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

fix: decrease a call limit on the toml fuzzer #698

Merged
merged 1 commit into from Aug 19, 2022

Conversation

tomtau
Copy link
Contributor

@tomtau tomtau commented Aug 17, 2022

closes #697

@tomtau tomtau requested a review from a team August 17, 2022 14:05
@tomtau tomtau requested a review from a team as a code owner August 17, 2022 14:05
@tomtau tomtau requested review from NoahTheDuke and removed request for a team August 17, 2022 14:05
@NoahTheDuke
Copy link
Member

Weird that it's failing now but wasn't failing earlier. Looks like this sample is truly a worst case scenario. Should the limit be lowered even more?

@CAD97
Copy link
Contributor

CAD97 commented Aug 17, 2022

Ideally we'd determine what the actual limit is to avoid an OOM. And ideally max usage should be linear on the limit, so there shouldn't be a case where one input hits the limit but another OOMs...

@CAD97
Copy link
Contributor

CAD97 commented Aug 17, 2022

(Meta note: I don't know why the maintainers review request is being converted to an individual but the team isn't.. perhaps it's the fact that the requested maintainer is also on the triage team? But then why do I have a requested review? I think what's happened is that GH has routed the triage request to me but not updated that in the reviewers list?)

@tomtau tomtau force-pushed the fix/fuzz-tomlcrash2 branch 3 times, most recently from 02fcd2e to 528b66b Compare August 17, 2022 16:10
@tomtau
Copy link
Contributor Author

tomtau commented Aug 17, 2022

@NoahTheDuke this one is weird that I managed to reproduce it locally (the sample crashes even VSCode's parser sometimes when I try to open it)... the limit I set passes on my machine locally, but fails on CI for some reason

@tomtau
Copy link
Contributor Author

tomtau commented Aug 17, 2022

@NoahTheDuke when running the tests with --release on CI, it seems to work fine: https://github.com/pest-parser/pest/actions/runs/2876875272

@CAD97
Copy link
Contributor

CAD97 commented Aug 17, 2022

--release

Ah yes, definitely, anything dealing with resource exhaustion will change thresholds in release and debug mode, because debug has zero optimization. Notably, if you have any bindings which are not live for the entire function, stack space for all of them to be live simultaneously is reserved in debug mode and bindings which are not live in the same control flow are only overlapped once optimizations kick in. This can in a case with a bunch of match arms make the difference of multiple orders of magnitude of stack space. And it doesn't have to be named bindings, either; just temporaries (e.g. calling a method, doing basically anything other than a literal) is enough to generate locals which debug mode pessimistically assumes could be live for the entire function.

There's two reasonable resolutions AIUI:

  • Mark resource-exhaustion tests as #[ignore]; test them with --include-ignored in --release mode
  • Make the resource limits depend on cfg(opt_level) (which needs to be sniffed in the buildscript, or debug_assertions as a proxy)

@tomtau
Copy link
Contributor Author

tomtau commented Aug 18, 2022

@CAD97 I'd prefer the #[ignore] option, because I wasn't able to reliable reproduce the CI behaviour locally in the debug mode (i.e. tests didn't crash with the set limit, unlike CI), so there may be a few more runtime and toolchain variables than just the optimization level. Unfortunately, --include-ignored fails due to a handful of ignored illustratory doc examples that don't build (one alternative for them may be to change them to text instead of ignore, but that would lose syntax highlighting).

Anyway, let's go with the second option for now (I feature-guarded that problematic sample execution with #[cfg(not(debug_assertions))]).

@CAD97
Copy link
Contributor

CAD97 commented Aug 18, 2022

Unfortunately, --include-ignored fails due to a handful of ignored illustratory doc examples that don't build

And unfortunately the rustdoc team isn't very open to fixing this for some reason. The interim solution is to specify which tests to include manually, using e.g. smth like test -- --ignored exhaustion_limits.

@tomtau
Copy link
Contributor Author

tomtau commented Aug 18, 2022

@CAD97 ok, thanks for the context. Yes, we can then keep it ignored with a separate step to run -- --ignored ... for the moment then.

@tomtau
Copy link
Contributor Author

tomtau commented Aug 19, 2022

@NoahTheDuke @CAD97 ok to merge this?

Copy link
Contributor

@CAD97 CAD97 left a comment

Choose a reason for hiding this comment

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

Lgtm now

@CAD97 CAD97 merged commit 3a1f167 into pest-parser:master Aug 19, 2022
@tomtau tomtau deleted the fix/fuzz-tomlcrash2 branch August 19, 2022 04:51
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.

oss-fuzz: toml stackoverflow crash
3 participants