Skip to content

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Sep 6, 2025

No description provided.

@nielsdos nielsdos force-pushed the exif-fixes2 branch 2 times, most recently from 6f75e1f to 7dcdc1a Compare September 7, 2025 09:16
@nielsdos nielsdos marked this pull request as ready for review September 7, 2025 10:48
@nielsdos nielsdos requested a review from bukka September 12, 2025 07:34
@nielsdos
Copy link
Member Author

Rebased.
And this now also fixes https://issues.oss-fuzz.com/issues/444479893

@nielsdos nielsdos requested a review from arnaud-lb September 13, 2025 09:48
@nielsdos nielsdos requested a review from arnaud-lb September 13, 2025 22:01
@nielsdos
Copy link
Member Author

Nice catches in the review, thanks! I updated it with the issues (hopefully) fixed.

Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

Looks good to me!

It is illegal to construct out-of-bound pointers, even if they are not
dereferenced. The current bound checks rely on undefined behaviour.
Fix this by introducing convenience macros that check the remaining
length.
The loop checks against `p` but increases `p2`. I don't see the point of
having 2 separate variables, so use `p` instead to correct the bounds
check and simplify the code in the process.
…ile size

We change the order of operations such that the file size check cannot
overflow in the for loop. This prevents infinite loops.
We also add an overflow check at the end of the loop body to prevent the
addition of offset and box.size from overflowing.
@nielsdos
Copy link
Member Author

Rebased and cleaned up the history. Will merge on green CI.

@nielsdos nielsdos merged commit fb3d4be into php:master Sep 14, 2025
9 checks passed
@bukka
Copy link
Member

bukka commented Oct 2, 2025

I just doubled checked this and it all looks good. Nice work. It was really buggy - I didn't do a good job in review apparently - need to do more checking next time.

@bukka
Copy link
Member

bukka commented Oct 2, 2025

Well I found few but apparently also missed few as well but good that fuzzer is running on it as some of this would be security issues most likely...

@nielsdos
Copy link
Member Author

nielsdos commented Oct 2, 2025

Parsing code tends to be tricky. It's always good to look with a few different people as it's very easy to miss issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants