-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[fuzz] fix h1 end to end fuzz test issue #15318
Conversation
Signed-off-by: Asra Ali <asraa@google.com>
Just found a very similar issue with h2_capture that I will add for this PR. Will ping when ready for review. |
Signed-off-by: Asra Ali <asraa@google.com>
Ready, sorry about that! Would love help for a better solution btw. I cannot actually test this without it being in the OSS-Fuzz environment, I have one concern where this might not work, and that is if the canonical path is not
but I can't check that in the logs... all I know is the visible path. I'm not sure if /mnt will be the output of realpath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange bug...
I'm guessing that because the test was able to write the file upon initialization without a failure, and because it only failed on the 33rd iteration (although the previous may have been wrong input) then it might be something external that deletes temporary files, but that's just a guess.
I wonder if using /tmp is the right solution (https://google.github.io/oss-fuzz/further-reading/fuzzer-environment/#file-system).
Seems that the fix for the h1 and h2 fuzzer is inconsistent (h1 uses inlined bytes, and h2 uses illegal path). Should it be that way?
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION | ||
// Allow temporary files in OSS-Fuzz to prevent failures running (h1/h2)_capture_fuzz_test. | ||
// Ideally this would be TestEnvironment::temporaryPath() but this avoids depending on test | ||
// libraries. | ||
if (absl::StartsWith(canonical_path.rc_, "/mnt")) { | ||
return true; | ||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that the fuzzer code won't be able to access files in /mnt?
Could this impact fuzz tests that may need to access external files under the /mnt directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OMG thanks for pointing that out. The fix was meant to do the opposite.
The best solution is probably to make
Ideally I want to avoid using the filesystem anyway. |
Signed-off-by: Asra Ali <asraa@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this!
/assign @htuch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Signed-off-by: Asra Ali asraa@google.com
Commit Message: Removes temporary files used for direct responses in h1 and h2 end to end fuzz test.
Additional Description: This was causing a flakey issue where OSS-Fuzz's temporary path used for direct responses looked illegal to Envoy. It seems unlikely that it was one of the deny listed paths in
illegalPath
because the path is always/mnt
. It may be flakey on arealpath
failure when getting the canonical path. Replace with inline bytes since it's better not to mess around with the filesystem anyway.and bootstrap path:
Risk Level: Low
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=31617