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

Use Go 1.18's testing.F on simple fuzzers #7056

Merged
merged 3 commits into from
Jun 15, 2022
Merged

Conversation

kzys
Copy link
Member

@kzys kzys commented Jun 13, 2022

These two fuzzers are simple enough to use Go 1.18's testing.F

@k8s-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kzys kzys marked this pull request as ready for review June 13, 2022 17:20
@kzys kzys force-pushed the go118-fuzz branch 2 times, most recently from c64378f to 086db70 Compare June 13, 2022 17:44
Copy link
Member

@mxpv mxpv left a comment

Choose a reason for hiding this comment

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

Should we enable these on CI?

go test -fuzz FuzzXYZ

@kzys
Copy link
Member Author

kzys commented Jun 13, 2022

Should we enable these on CI?

go test -fuzz FuzzXYZ

@AdamKorcz - Would it be covered by #7052? I don't want to maintain the list of the fuzzing tests if possible.

@AdamKorcz
Copy link
Contributor

AdamKorcz commented Jun 13, 2022

Should we enable these on CI?
go test -fuzz FuzzXYZ

@AdamKorcz - Would it be covered by #7052?

No

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

kzys added 2 commits June 15, 2022 14:56
Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
@kzys kzys force-pushed the go118-fuzz branch 2 times, most recently from 106201b to bf88348 Compare June 15, 2022 16:15
In addition to oss-fuzz's CIFuzz (see containerd#7052), this commit adds a small
shell script that run all fuzzing tests with go test -fuzz.

While running for 30 seconds would be too short to acutally find issues,
we want to make sure that these fuzzing tests are not fundamentally
broken.

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
@kzys
Copy link
Member Author

kzys commented Jun 15, 2022

The last commit is adding simple go test -fuzz. While these tests are used by CI Fuzz, I'd like to make sure simple go test -fuzz is working as well.

@kzys
Copy link
Member Author

kzys commented Jun 15, 2022

@mxpv Can you take a look again? Your comment should be addressed by the last commit.

Copy link
Member

@mxpv mxpv left a comment

Choose a reason for hiding this comment

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

Thanks!

@mxpv mxpv merged commit 8aa3459 into containerd:main Jun 15, 2022
"github.com/containerd/containerd/pkg/cap"
)

func FuzzParseProcPIDStatus(data []byte) int {
Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed? Is it not needed for the oss-fuzz integration?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is moved under pkg/cap.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but the signature is now func (*testing.F) rather than func ([]byte) int and is moved into a _test.go file. Does the external OSS-fuzz runner that consumes these functions (from #4841) know how to invoke a func (*testing.F)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the external OSS-fuzz runner that consumes these functions (from #4841) know how to invoke a func (*testing.F)?

Yes. Some other changes might be needed in the build script, though. For example, utilities from _test.go files are currently not available to OSS-Fuzz, so these files need to be non _test.go files.

Copy link
Contributor

Choose a reason for hiding this comment

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

the signature is now func (*testing.F) rather than func ([]byte) int

Both can be used by OSS-Fuzz.

"github.com/containerd/containerd/platforms"
)

func FuzzPlatformsParse(data []byte) int {
Copy link
Member

Choose a reason for hiding this comment

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

Same question on this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is moved under platforms/. Since Go's fuzzing support is part of testing package. I'm moving fuzzing from contrib/fuzz to each package if possible.

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.

6 participants