Skip to content

Conversation

@FrankYFTang
Copy link
Contributor

@FrankYFTang FrankYFTang commented Oct 22, 2025

Avoid timeout on meaningless test

  • Required: Issue filed: ICU-23239
  • Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
  • Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@FrankYFTang FrankYFTang requested a review from roubert October 22, 2025 20:24
Copy link
Member

@roubert roubert left a comment

Choose a reason for hiding this comment

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

This doesn't look right to me, there must surely be some way to configure the fuzzer to never generate more data than what is wished for, so that all that's needed here is a simple assertion that the data isn't larger than that, instead of having the fuzzer generate too much data and then throwing away the excess.

@FrankYFTang
Copy link
Contributor Author

sorry, I am not ready for review yet. click on the wrong bug

@FrankYFTang FrankYFTang changed the title ICU-23239 Limit fuzzer test data size to 32K ICU-23239 Limit fuzzer test data size to 64K bytes Oct 25, 2025
@FrankYFTang FrankYFTang force-pushed the ICU-23239-limit-pattern-length branch from ef68537 to eba7115 Compare October 25, 2025 17:56
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@roubert roubert marked this pull request as draft October 27, 2025 15:08
@roubert
Copy link
Member

roubert commented Oct 27, 2025

sorry, I am not ready for review yet. click on the wrong bug

OK, I've now marked this PR as draft for you, so that you can click Ready for review when it is.

@FrankYFTang FrankYFTang marked this pull request as ready for review October 27, 2025 20:15
@FrankYFTang
Copy link
Contributor Author

FrankYFTang commented Oct 27, 2025

This doesn't look right to me, there must surely be some way to configure the fuzzer to never generate more data than what is wished for, so that all that's needed here is a simple assertion that the data isn't larger than that, instead of having the fuzzer generate too much data and then throwing away the excess.

There is one: use option file *fuzzer.option file with max_len option
However, as stated in
https://github.com/google/oss-fuzz/blob/master/docs/getting-started/new_project_guide.md#input-size
It is recommend to hard code that limit in .cpp file because some fuzzer engine won't hand that
(see google/oss-fuzz#1846 )

I change the PR to use the option file and that should work in libfuzz but won't work with other fuzzer.
Do you prefer we use this way or rollback to the recommendated way I wrote before?

@roubert
Copy link
Member

roubert commented Oct 27, 2025

I change the PR to use the option file and that should work in libfuzz but won't work with other fuzzer.
Do you prefer we use this way or rollback to the recommendated way I wrote before?

This seems like a much better solution to me, but to be safe you might want to combine it with an assertion in the code that verifies that it really doesn't get more data than it should.

@FrankYFTang
Copy link
Contributor Author

I change the PR to use the option file and that should work in libfuzz but won't work with other fuzzer.
Do you prefer we use this way or rollback to the recommendated way I wrote before?

This seems like a much better solution to me, but to be safe you might want to combine it with an assertion in the code that verifies that it really doesn't get more data than it should.

much better? but the documentation said it won't work!

I don't think it is a good idea to put the limit into two places . It is a known issue that other fuzzer will get more data and trigger the assertion. I do not think it is reasonable to add an assertion for a known issue which we know it will break.

@FrankYFTang
Copy link
Contributor Author

@roubert ping

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.

2 participants