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

[fuzz] fix h1 end to end fuzz test issue #15318

Merged
merged 4 commits into from
Mar 8, 2021
Merged

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Mar 4, 2021

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 a realpath failure when getting the canonical path. Replace with inline bytes since it's better not to mess around with the filesystem anyway.

[2021-02-17 14:24:17.258][49492][critical][assert] [test/integration/base_integration_test.cc:322] assert failure: rejected_counter == nullptr \|\| rejected_counter->value() == 0. Details: Lds update failed. Details
failed_configuration:
[...]
details: "Invalid path: /mnt/scratch0/clusterfuzz/bot/tmp/test_envoy"

and bootstrap path:

[2021-02-17 20:55:28.894][82532][critical][main] [source/server/server.cc:109] error initializing configuration '/mnt/scratch0/clusterfuzz/bot/tmp/bootstrap.pb': Invalid path: /mnt/scratch0/clusterfuzz/bot/tmp/bootstrap.pb'

Risk Level: Low
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=31617

Signed-off-by: Asra Ali <asraa@google.com>
@asraa
Copy link
Contributor Author

asraa commented Mar 4, 2021

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>
@asraa
Copy link
Contributor Author

asraa commented Mar 4, 2021

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 /mnt/...

const Api::SysCallStringResult canonical_path = canonicalPath(path);

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

Signed-off-by: Asra Ali <asraa@google.com>
Copy link
Contributor

@adisuissa adisuissa left a 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?

Comment on lines 163 to 170
#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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@asraa
Copy link
Contributor Author

asraa commented Mar 5, 2021

I wonder if using /tmp is the right solution (https://google.github.io/oss-fuzz/further-reading/fuzzer-environment/#file-system).

The best solution is probably to make temporaryPath() return /tmp for OSS-Fuzz. As long as the realpath of /tmp looks legal, things will work.

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?

Ideally I want to avoid using the filesystem anyway.

Signed-off-by: Asra Ali <asraa@google.com>
Copy link
Contributor

@adisuissa adisuissa left a 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!

@dio
Copy link
Member

dio commented Mar 8, 2021

/assign @htuch

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@asraa asraa merged commit 9541572 into envoyproxy:main Mar 8, 2021
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.

4 participants