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

Improvements of OSS-Fuzz integration #33724

Closed
DavidKorczynski opened this issue Jun 4, 2020 · 2 comments
Closed

Improvements of OSS-Fuzz integration #33724

DavidKorczynski opened this issue Jun 4, 2020 · 2 comments
Labels
build Issues and PRs related to build files or the CI. feature request Issues that request new features to be added to Node.js.

Comments

@DavidKorczynski
Copy link
Contributor

DavidKorczynski commented Jun 4, 2020

Is your feature request related to a problem? Please describe.
Please describe the problem you are trying to solve.

This feature is not related to a problem.

Describe the solution you'd like
Please describe the desired behavior.

This feature asks for tighter integration with continuous fuzzing via OSS-Fuzz. In this PR google/oss-fuzz#3860 (comment) I integrated NodeJS with fuzzing and so far it was used to capture this bug #33640
However, the current integration could be improved and it would be desirable to cover more of NodeJS with fuzzers, as briefly discussed with @bnoordhuis in the above PR. Specifically, there are two core parts where the integration with OSS-Fuzz can improve: (1) integrating the build procedure with the OSS-Fuzz environment more closely with the NodeJS environment and (2) building more fuzzers.

Regarding part 1 then the current strategy (build.sh here https://github.com/google/oss-fuzz/pull/3860/files) compiles the NodeJS core in an awkward manner by first running make without any proper OSS-Fuzz flags and then re-comiling the .cc files of node/src with the proper OSS-Fuzz flags, in order to create a static archive.
The OSS-Fuzz environment sets the following environment variables when compiling the fuzzers to something similar to this:

export CC=clang
export CXX=clang++
export CFLAGS="-O1 -fno-omit-frame-pointer -gline-tables-only -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=address -fsanitize-address-use-after-scope -fsanitize=fuzzer-no-link"
export CXXFLAGS="-O1 -fno-omit-frame-pointer -gline-tables-only -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=address -fsanitize-address-use-after-scope -fsanitize=fuzzer-no-link -stdlib=libc++"
export LIB_FUZZING_ENGINE="-fsanitize=fuzzer"

It would be nice if the build process of NodeJS can integrate a fuzzing part which enables us to compile with the OSS-Fuzz variables (CFLAGS, CXXFLAGS and LIB_FUZZING_ENGINE) above. The LIB_FUZZING_ENGINE variable is only used for linking the final fuzzer and should not be used on any of the compiled libraries. Also note that to get the fuzzers compiled properly they should be compiled against static libraries. As I see the desired goal is, therefore, to have the files in node/src be compiled with the CFLAGS and CXXFLAGS variables above.

Regarding part 2 then I can certainly start writing more fuzzers and covering more of the NodeJS code, but if you have any suggestions of good APIs for fuzzing then here would be a good place to write I think.

@DavidKorczynski DavidKorczynski changed the title Integration with OSS-Fuzz Improvements of OSS-Fuzz integration Jun 4, 2020
@bnoordhuis
Copy link
Member

W.r.t. part 1: it should be fairly straightforward to add a flag to configure.py that gets forwarded to the gyp build files, where the appropriate defines and compiler flags are then added.

a7ae7aa is a recent example. It also demonstrates how you can remove already defined flags with 'cflags!': [ '-fomit-frame-pointer' ] (note the exclamation mark.)

Use cflags_cc for C++-only flags and defines for, er, defines.

LIB_FUZZING_ENGINE is set by Google's fuzzing build service? You probably want to pass that on as a flag from configure.py to the gyp files in the same way.

Do you want to open a pull request? If you can't get it to work, don't worry - just open the pull request and we'll step through it.

@bnoordhuis bnoordhuis added the build Issues and PRs related to build files or the CI. label Jun 5, 2020
@DavidKorczynski
Copy link
Contributor Author

DavidKorczynski commented Jun 5, 2020

W.r.t. part 1: it should be fairly straightforward to add a flag to configure.py that gets forwarded to the gyp build files, where the appropriate defines and compiler flags are then added.

a7ae7aa is a recent example. It also demonstrates how you can remove already defined flags with 'cflags!': [ '-fomit-frame-pointer' ] (note the exclamation mark.)

Use cflags_cc for C++-only flags and defines for, er, defines.

Perfect!

LIB_FUZZING_ENGINE is set by Google's fuzzing build service? You probably want to pass that on as a flag from configure.py to the gyp files in the same way.

Yes it's set by Google's service.

Do you want to open a pull request? If you can't get it to work, don't worry - just open the pull request and we'll step through it.

Okay awesome and yup sounds good - I will get the above up and running and then open a PR! Thanks for the detailed assistance.

@jasnell jasnell added the feature request Issues that request new features to be added to Node.js. label Jun 5, 2020
@Trott Trott closed this as completed in 375b859 Aug 15, 2020
MylesBorins pushed a commit that referenced this issue Aug 17, 2020
Refs: google/oss-fuzz#3860
Fixes: #33724

PR-URL: #34761
Fixes: #33724
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
DavidKorczynski added a commit to DavidKorczynski/node that referenced this issue Aug 19, 2020
BethGriggs pushed a commit that referenced this issue Aug 20, 2020
Refs: google/oss-fuzz#3860
Fixes: #33724

PR-URL: #34761
Fixes: #33724
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
addaleax pushed a commit that referenced this issue Sep 22, 2020
Refs: google/oss-fuzz#3860
Fixes: #33724

PR-URL: #34761
Fixes: #33724
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
mhdawson pushed a commit that referenced this issue Oct 9, 2020
Refs: #34761
Refs: #33724

PR-URL: #34844
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this issue Oct 14, 2020
Refs: #34761
Refs: #33724

PR-URL: #34844
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 3, 2020
Refs: #34761
Refs: #33724

PR-URL: #34844
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 16, 2020
Refs: #34761
Refs: #33724

PR-URL: #34844
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this issue Jan 8, 2021
Refs: nodejs#34761
Refs: nodejs#33724

PR-URL: nodejs#34844
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants