Skip to content

Cleanup exif checks #10402

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

Closed
wants to merge 2 commits into from
Closed

Cleanup exif checks #10402

wants to merge 2 commits into from

Conversation

nielsdos
Copy link
Member

Gets rid of two redundant checks.

This condition will never trigger because otherwise the do-while loop
wouldn't have exited.
@devnexen
Copy link
Member

nice catches !

@@ -3810,12 +3805,6 @@ static bool exif_scan_JPEG_header(image_info_type *ImageInfo)

fpos = php_stream_tell(ImageInfo->infile);

if (marker == 0xff) {
Copy link
Member

Choose a reason for hiding this comment

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

could it be still somewhat useful (e.g. fuzzing) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand your question. This check can never be hit because of the do-while loop: if we get here marker must be !=0xff. Unless this code was meant to check in a different way?

Copy link
Member

Choose a reason for hiding this comment

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

nevermind I misread the code it s clearer when unfolded.

Copy link
Member

Choose a reason for hiding this comment

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

It might be a good idea to replace that with a ZEND_ASSERT(), so it's easier to understand (and to also have a safeguard just in case the code above would be modified).

Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

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

LCTM

@devnexen devnexen closed this in f4dd35e Jan 22, 2023
@devnexen
Copy link
Member

Thanks!

devnexen added a commit to devnexen/php-src that referenced this pull request Jan 22, 2023
 more in a context of a possible text change.
follow-up on phpGH-10402.
devnexen added a commit that referenced this pull request Jan 23, 2023
… a context of a possible text change. follow-up on GH-10402.

Close GH-10416.
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.

None yet

4 participants