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

Explicitly disable Go modules via GO111MODULE=off for go-fuzz corpus #2878

Closed
thepudds opened this issue Sep 22, 2019 · 7 comments
Closed

Explicitly disable Go modules via GO111MODULE=off for go-fuzz corpus #2878

thepudds opened this issue Sep 22, 2019 · 7 comments

Comments

@thepudds
Copy link

thepudds commented Sep 22, 2019

Background

Go modules are currently an opt-in feature of the go command. The GO111MODULE env var controls when module-mode is enabled for the go command.

go-fuzz does not currently support module-mode, but that might change soon.

Suggested change

It would be helpful to explicitly set GO111MODULE=off within oss-fuzz for projects/golang.

The oss-fuzz change could look something like this.

The history is a bit confusing, but in short, it is likely better for oss-fuzz to be explicitly in control over whether or not module-mode is enabled by setting GO111MODULE explicitly, rather than being surprised on a random day by one or more upstream changes that are likely to happen.

Additional details

This might be more detail than is interesting, but a bit more about the history here, as well as more on the rationale for this change:

  • The go command's default value for GO111MODULE did change twice during Go 1.13 dev cycle — first the default was changed from auto to on, and then later that was reversed. The first time it changed to default to on, that caused breakage for oss-fuzz (for example in Golang internal library fuzzers #2188 (comment)).
  • When that breakage happened for oss-fuzz during the Go 1.13 dev cycle, there was a temporary workaround introduced in go-fuzz of setting GO111MODULE=off within go-fuzz itself.
  • Although the details are still being worked out (e.g., go-fuzz-build doesn't work with go modules (go 1.11.2) dvyukov/go-fuzz#195), most likely as part of adding support for fuzzing modules, go-fuzz will be removing that temporary workaround of always setting GO111MODULE=off (and most likely go-fuzz will start respecting the user's setting for GO111MODULE at least to some degree).
  • The go command's default value for GO111MODULE is planned to be changed again to default to on during the Go 1.14 development cycle (which is a second try at the same change that broke oss-fuzz in Golang internal library fuzzers #2188 (comment)).
  • If both of those things happen without any change on the oss-fuzz side, then the fuzzing of projects/golang on oss-fuzz would likely break. Rather than that happening on a random day, it is better to have more explicit control within oss-fuzz itself by disabling GO111MODULE explicitly as suggested in this issue.
  • Right now, none of the packages at dvyukov/go-fuzz-corpus/... are modules, which means no one is relying on module mode being enabled (and in fact, if someone was relying on module-mode being enabled, it wouldn't work for them currently because go-fuzz does not yet support modules).

Alternatives

Two alternatives to consider:

  1. Rather than setting GO111MODULE=off in the build.sh, set it in the docker file. That could be a reasonable choice as well. My guess is it makes more sense in build.sh, but the right answer here is probably dependent on the oss-fuzz philosophy.
  2. Rather than setting GO111MODULE=off, set GO111MODULE=auto. That could be a reasonable choice currently, but I think from oss-fuzz perspective, it is probably a more conservative change to do GO111MODULE=off, including because go-fuzz currently sets GO111MODULE=off (and not GO111MODULE=auto), and also the meaning of auto changed during the Go 1.13 dev cycle and some small-ish chance the meaning of auto might change again on a random day during the Go 1.14 dev cycle, and hence slightly more future proof to not use auto.

CC @dvyukov @josharian @Dor1s @guidovranken @mvdan

@josharian
Copy link
Contributor

I’m +1 on explicitly setting GO111MODULE=off somewhere. I have a very weak preference for the docker file because I think a new contributor is more likely to notice it there.

@Dor1s
Copy link
Contributor

Dor1s commented Sep 23, 2019

Should we set it inside the base-builder image (it is used for building all OSS-Fuzz projects), so that every project will have it automatically?

@josharian
Copy link
Contributor

Should we set it inside the base-builder image (it is used for building all OSS-Fuzz projects), so that every project will have it automatically?

Probably not. go-fuzz-corpus is unusual in that it is fuzzing the standard library. For regular Go projects, the right approach is to leave it unset. IIUC, @thepudds is in the process of making go-fuzz work correctly by default for normal projects...which will break go-fuzz-corpus without the explicit override, thus this issue.

@thepudds
Copy link
Author

FWIW, I agree with @josharian's comments.

One other data point is the cryptofuzz project on oss-fuzz sets GO111MODULE=off in its build.sh:

export GO111MODULE=off

I mentioned that mainly in support of explicitly setting GO111MODULE=off somewhere on the oss-fuzz side for the golang stdlib fuzzers being discussed here.

(And I don't really have any opinion on using the build.sh vs. the docker file within the project).

@thepudds
Copy link
Author

thepudds commented Oct 3, 2019

Hello @Dor1s, wanted to briefly check if this generally makes sense or not, and whether or not you have any open questions?

The short version:

@josharian
Copy link
Contributor

@thepudds I think you should just send a PR, and we can discuss there. I think @Dor1s is likely to just merge if we are happy with it, which (AFAICT) we are.

yevgenypats pushed a commit to fuzzitdev/oss-fuzz that referenced this issue Oct 3, 2019
This will prevent breaking go-fuzz when it will support go modules
dvyukov/go-fuzz#195

and addressing this issue:
google#2878
Dor1s pushed a commit that referenced this issue Oct 3, 2019
This will prevent breaking go-fuzz when it will support go modules
dvyukov/go-fuzz#195

and addressing this issue:
#2878
@thepudds
Copy link
Author

thepudds commented Oct 4, 2019

Fixed by #2914.

Thanks @yevgenypats!

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

No branches or pull requests

3 participants