Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fuzzing Coverage Expansion #866

Merged
merged 15 commits into from
Oct 4, 2023
Merged

Fuzzing Coverage Expansion #866

merged 15 commits into from
Oct 4, 2023

Conversation

viktoriia-lsg
Copy link
Contributor

This pull requests expands fuzzing coverage for OSS-fuzz by adding 4 more fuzz test files and 5 corpora files. All fuzz tests are written in native go fuzzing style. Corpora files are compatible with OSS-fuzz format.

New fuzzers and corpora were added for OSS-Fuzz.
fse/fse_fuzz_test.go Outdated Show resolved Hide resolved
fse/fse_fuzz_test.go Outdated Show resolved Hide resolved
huff0/huff0_fuzz_test.go Outdated Show resolved Hide resolved
s2/s2_fuzz_test.go Outdated Show resolved Hide resolved
s2/s2_fuzz_test.go Outdated Show resolved Hide resolved
s2/s2_fuzz_test.go Outdated Show resolved Hide resolved
snappy/xerial/xerial_encode_fuzz_test.go Outdated Show resolved Hide resolved
huff0/huff0_fuzz_test.go Outdated Show resolved Hide resolved
@klauspost
Copy link
Owner

klauspost commented Sep 26, 2023

Please see #289 for reference.

A) I would like to move seed files off-repo, since it is making the repo too large. I have an old repo that was used for the old fuzz tests, but can be repurposed for this.

B) I do not want to end up like with #289 - where a third party controls scripts that I cannot modify freely. This means I have been unable to modify/add my fuzz tests for half a year now. I have asked @AdamKorcz for assistance in this, but no response. If I am not in control of the setup, I will not accept this.

Needless to say I am not very happy about the current setup and would like that fixed as a primary thing, which TBH is more important than adding coverage.

If you need assistance in making CI pass, let me know.

Renaming from x_fuzz_test.go to fuzz_test.go
klauspost added a commit that referenced this pull request Sep 26, 2023
Fixes `panic: runtime error: index out of range [7] with length 5` when providing short input to EstimateBlockSize.

Found via #866
klauspost added a commit that referenced this pull request Sep 26, 2023
Fixes `panic: runtime error: index out of range [7] with length 5` when providing short input to EstimateBlockSize.

Found via #866
@viktoriia-lsg
Copy link
Contributor Author

Regarding fuzzing setup:
A) I think it can be done easily. I just need to rewrite configuration file for it to take files from the github repository. So it's not a problem.
B) As the configuration file (build.sh) is located on oss-fuzz repo, I still need to make PR to update it. To satisfy your needs, we can change build.sh to the following format, so the oss-fuzz will take build script from your repo:

$SRC/compress/ossfuzz.sh

Does it make sense?

@klauspost
Copy link
Owner

Seems a file is missing:

--- FAIL: FuzzEncoder (0.00s)
    helpers.go:36: open testdata/fuzz/block-corpus-raw.zip: no such file or directory
FAIL

workaround in s2 is removed; snappy fuzzer is moved to fuzz_test; missing file is added.
@viktoriia-lsg
Copy link
Contributor Author

Thanks! I've added a missing file and slightly changed a path to it.

@klauspost
Copy link
Owner

Run diff <(gofmt -d .) <(printf "")
1,13d0
< diff fse/fuzz_test.go.orig fse/fuzz_test.go
< --- fse/fuzz_test.go.orig
< +++ fse/fuzz_test.go
< @@ -3,[8](https://github.com/klauspost/compress/actions/runs/6313933219/job/17143098604?pr=866#step:4:9) +3,8 @@
<  import (
<  	"bytes"
<  	"fmt"
< -	"testing"
<  	"github.com/klauspost/compress/internal/fuzz"
< +	"testing"
<  )
<

@viktoriia-lsg
Copy link
Contributor Author

  1. Does it mean that fse/fuzz_test.go needs a newline after "testing" package?
  2. Regarding s2_fuzz_test.go file, is everything fine, so I can merge it with s2/fuzz_test.go? Fuzzing Coverage Expansion #866 (comment)
  3. What do you think about this setup? Fuzzing Coverage Expansion #866 (comment)

@klauspost
Copy link
Owner

  1. Does it mean that fse/fuzz_test.go needs a newline after "testing" package?

Imports are always sorted.

  1. Regarding s2_fuzz_test.go file, is everything fine, so I can merge it with s2/fuzz_test.go? Fuzzing Coverage Expansion #866 (comment)

Yes, please do that.

  1. What do you think about this setup? Fuzzing Coverage Expansion #866 (comment)

Sounds like a reasonable approach.

viktoriia-lsg and others added 4 commits September 29, 2023 14:24
Fix import issue in fse;
Merge s2_fuzz_test with s2/fuzz_test.
Moving setup_dicts.go from oss-fuzz
@viktoriia-lsg
Copy link
Contributor Author

Alright, we started working on it.

Setup_dicts is moved to ossfuzz dir;
Configuration file ossfuzz.sh is moved to repo.
Adding a comment
@viktoriia-lsg
Copy link
Contributor Author

Hi @klauspost! We've made changes as discussed earlier. We've also opened a pull request to compress-fuzz with seed corpuses klauspost/compress-fuzz#2.

Please let us know, if we need to change anything.

@klauspost
Copy link
Owner

Thanks! I will run the tests and give it a final review.

Copy link
Owner

@klauspost klauspost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one change and a question.

snappy/xerial/testdata/block-corpus-raw.zip Outdated Show resolved Hide resolved
ossfuzz/ossfuzz.sh Outdated Show resolved Hide resolved
@viktoriia-lsg
Copy link
Contributor Author

I have edited ossfuzz.sh file and decreased the seed corpus in size.

@klauspost
Copy link
Owner

Looks good!

@viktoriia-lsg
Copy link
Contributor Author

Thanks! Please also review klauspost/compress-fuzz#2 , so I can make a PR to oss-fuzz in order to start fuzzing.

@klauspost
Copy link
Owner

Thanks! Please also review klauspost/compress-fuzz#2 , so I can make a PR to oss-fuzz in order to start fuzzing.

Ah. Somehow I missed that. Will take a look - probably tomorrow.

@klauspost klauspost merged commit 0e1030c into klauspost:master Oct 4, 2023
18 checks passed
kodiakhq bot referenced this pull request in cloudquery/filetypes Nov 1, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github.com/klauspost/compress](https://github.com/klauspost/compress) | indirect | patch | `v1.17.0` -> `v1.17.2` |

---

### Release Notes

<details>
<summary>klauspost/compress (github.com/klauspost/compress)</summary>

### [`v1.17.2`](https://github.com/klauspost/compress/releases/tag/v1.17.2)

[Compare Source](https://github.com/klauspost/compress/compare/v1.17.1...v1.17.2)

#### What's Changed

-   zstd: Fix corrupted output in "best" by [@&#8203;klauspost](https://github.com/klauspost) in [https://github.com/klauspost/compress/pull/876](https://github.com/klauspost/compress/pull/876)

**Full Changelog**: klauspost/compress@v1.17.1...v1.17.2

### [`v1.17.1`](https://github.com/klauspost/compress/releases/tag/v1.17.1)

[Compare Source](https://github.com/klauspost/compress/compare/v1.17.0...v1.17.1)

#### What's Changed

-   s2: Fix S2 "best" dictionary wrong encoding by [@&#8203;klauspost](https://github.com/klauspost) in [https://github.com/klauspost/compress/pull/871](https://github.com/klauspost/compress/pull/871)
-   flate: Reduce allocations in decompressor and minor code improvements by [@&#8203;fakefloordiv](https://github.com/fakefloordiv) in [https://github.com/klauspost/compress/pull/869](https://github.com/klauspost/compress/pull/869)
-   s2: Fix EstimateBlockSize on 6&7 length input by [@&#8203;klauspost](https://github.com/klauspost) in [https://github.com/klauspost/compress/pull/867](https://github.com/klauspost/compress/pull/867)
-   tests: Fuzzing Coverage Expansion by [@&#8203;viktoriia-lsg](https://github.com/viktoriia-lsg) in [https://github.com/klauspost/compress/pull/866](https://github.com/klauspost/compress/pull/866)
-   tests: Set FSE decompress fuzzer max limit by [@&#8203;klauspost](https://github.com/klauspost) in [https://github.com/klauspost/compress/pull/868](https://github.com/klauspost/compress/pull/868)
-   tests: Fuzzing Coverage Expansion ([#&#8203;2](https://github.com/klauspost/compress/issues/2)) by [@&#8203;viktoriia-lsg](https://github.com/viktoriia-lsg) in [https://github.com/klauspost/compress/pull/870](https://github.com/klauspost/compress/pull/870)

#### New Contributors

-   [@&#8203;viktoriia-lsg](https://github.com/viktoriia-lsg) made their first contribution in [https://github.com/klauspost/compress/pull/866](https://github.com/klauspost/compress/pull/866)
-   [@&#8203;fakefloordiv](https://github.com/fakefloordiv) made their first contribution in [https://github.com/klauspost/compress/pull/869](https://github.com/klauspost/compress/pull/869)

**Full Changelog**: klauspost/compress@v1.17.0...v1.17.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 4am on the first day of the month" (UTC), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMDkuNCIsInVwZGF0ZWRJblZlciI6IjM2LjEwOS40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants