Skip to content

fix: make sure ConsumeIntegralInRange is always in range [min; max] #945

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

Conversation

oetr
Copy link
Contributor

@oetr oetr commented Aug 13, 2025

No description provided.

@oetr oetr marked this pull request as ready for review August 13, 2025 06:38
Copy link
Contributor

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

Can this change the result in any case in which it previously fell within the given bounds? If possible, we should avoid invalidating existing crashing inputs that behaved as expected.

@oetr
Copy link
Contributor Author

oetr commented Aug 13, 2025

Can this change the result in any case in which it previously fell within the given bounds? If possible, we should avoid invalidating existing crashing inputs that behaved as expected.

Ahh, I see what you mean, good point. And yes, it can and it will!
Obviously, this will still invalidate the crashes due to the bug addressed in this PR.
For ranges [0; N] (the most frequent in my opinion!) my change does nothing different.
For all other ranges it might invalidate the crashing input.

My motivation for leaving the number as is, if it's already within the requested range [min; max], was to make the fuzzer more efficiently use the table of recent compares.
The way I understand it, libfuzzer keeps recently compared numbers in a table and randomly inserts them during mutation steps.
If these numbers are reduced modulo some range, ToRC becomes mostly useless for ranges other than [0;N].
The further away the range deviates from [0; N], the more useless the ToRC becomes (IMO).

So, my claim is that this change makes fuzzing more efficient, but I don't have the data!

A compromise solution that considers your point and fixes the bug at the same time would be to always squeeze (aka remove the if-statement)

// if (range != std::numeric_limits<T>::max()) 
// We accept modulo bias in favor of reading a dynamic number of bytes as
// this would make it harder for the fuzzer to mutate towards values from
// the table of recent compares.
result = result % (range + 1);

return static_cast<T>(min + result);

If possible, we should avoid invalidating existing crashing inputs that behaved as expected.

It's inconvenient, but can be addressed by fixing the issues before upgrading to the new Jazzer release, or by running the fuzzer and letting it find the issue again.

WDYT?

@oetr oetr force-pushed the 931-bug-fuzzeddataproviderconsume-returns-wrong-results-when-max---min-max_value branch 2 times, most recently from 507ecf6 to 07d24c4 Compare August 14, 2025 21:58
@oetr oetr requested a review from Copilot August 15, 2025 13:31
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug in the ConsumeIntegralInRange function to ensure the returned value is always within the specified range [min, max]. The fix optimizes for fuzzer effectiveness by preserving values that are already in range to maintain compatibility with the table of recent compares.

Key changes:

  • Adds a check to return values already within range without modification
  • Consolidates the modulo operation and range adjustment into a single expression
  • Removes the conditional check for std::numeric_limits<T>::max()

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@oetr oetr force-pushed the 931-bug-fuzzeddataproviderconsume-returns-wrong-results-when-max---min-max_value branch from 07d24c4 to b072ead Compare August 15, 2025 14:46
For cases where FuzzedDataProvider.consume<Byte,Short,Char,Integer,Long>(from,
to) sets the range [from; to] to be exactly the maximum value for the type, this
methods could return values outside the requested range. This PR makes sure that
the returned values are always in range.

In addition, this PR boost the performance of these methods by making less use
of the modulo operator to reduce fuzzer values into the range.  Keeping the
number "as is" allows the fuzzer to use the table of recent compares more
efficiently. At the same time, this change might invalidate existing crash files
and some corpus entries.  However, given overall performance boost this change
brings, the potential corpus invalidation is considered less important.
@oetr oetr force-pushed the 931-bug-fuzzeddataproviderconsume-returns-wrong-results-when-max---min-max_value branch from b072ead to ebf207a Compare August 18, 2025 10:48
@oetr oetr enabled auto-merge (rebase) August 18, 2025 10:52
@oetr oetr merged commit 47fef8f into main Aug 18, 2025
8 checks passed
@oetr oetr deleted the 931-bug-fuzzeddataproviderconsume-returns-wrong-results-when-max---min-max_value branch August 18, 2025 11:46
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.

Bug: FuzzedDataProvider#consume... returns wrong results when max - min == MAX_VALUE
4 participants