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 test for bad chunk data to allow for 0-sample deep chunks #1185

Merged

Conversation

kdt3rd
Copy link
Contributor

@kdt3rd kdt3rd commented Oct 16, 2021

Signed-off-by: Kimball Thurston kdt3rd@gmail.com

Signed-off-by: Kimball Thurston <kdt3rd@gmail.com>
@lgritz
Copy link
Contributor

lgritz commented Oct 16, 2021

I don't understand this part of the code at all, but something about the structure of the change seems odd to me.

There are a number of tests that look like this

if (condition)
    error;

and this PR changes those to

if (condition || other_conditions)
    error;

Please help me understand how strictly expanding the set of circumstances that are considered an error will fix the bug wherein a correct file has been incorrectly flagged as an error? If you are just using || to catch more conditions, won't the same false positive still occur?

@kdt3rd
Copy link
Contributor Author

kdt3rd commented Oct 16, 2021

well, it was if value <= 0 before, I've changed it to if value < 0 OR (equal 0 AND the other value != 0) instead of just the <= test

@lgritz
Copy link
Contributor

lgritz commented Oct 16, 2021

Aha, of course. Two of the places you changed were < before and still after, and two changed from <= to <, and I did not notice that. OK, all looks good.

@kdt3rd
Copy link
Contributor Author

kdt3rd commented Oct 16, 2021

yeah, I basically added other guards for the other values - the first one for the packed size we had added was due to a fuzz report, the other cases I added would be exposed by other fuzz tests that hadn't yet been triggered - pre-fixing failures

@kdt3rd kdt3rd merged commit decd533 into AcademySoftwareFoundation:master Oct 16, 2021
@kdt3rd kdt3rd deleted the fix_zerosample_deep_reads branch October 16, 2021 08:07
@lgritz
Copy link
Contributor

lgritz commented Oct 16, 2021

Woo-hoo! OIIO bleeding edge tests passed last night!

cary-ilm pushed a commit to cary-ilm/openexr that referenced this pull request Oct 21, 2021
cary-ilm pushed a commit that referenced this pull request Oct 24, 2021
Signed-off-by: Kimball Thurston <kdt3rd@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants