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(previews): Stop returning true when getimagesize() fails #46342

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

joshtrichards
Copy link
Member

Summary

Our checkImageSize() & checkImageDataSize() methods always return true if an image is corrupt/invalid1 (or unreadable). However, when getimagesize() returns false so should checkImage*() IMO.

This current behavior causes already detected invalid images to not only bypass the checkImageMemory() check (as intended), but to continue to be processed in other ways even though they'll fail.

This impacts all formats, not just jpeg (though most are covered by suppressors at this point).

One could argue the current situation is valid behavior since the intention of the checkImage* methods isn't to detect other types of image problems, so it's a bit of an overloaded use. But in all cases I can conceive of, other image manipulations are going to fail anyhow if getimagesize() does. And we're immediately calling getimagesize() once again in many cases.

P.S. With the fix to checkImageSize(), the line with the suppressor shouldn't even be executed any longer. The suppressor added here is just for good measure and to be consistent with the other image formats. If the behavior of the checkImage*() methods is desired to be retained as-is, I can eliminate those changes and keep only the suppressor.

TODO

  • ...

Checklist

Footnotes

  1. (or at least corrupt enough their image data can't be retrieved by gd)

@joshtrichards
Copy link
Member Author

/backport to stable29

@joshtrichards
Copy link
Member Author

Thinking more about this, we should probably just use the suppressor and not over-use these other methods...

3rdparty Outdated Show resolved Hide resolved
@joshtrichards

This comment was marked as outdated.

Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
@artonge artonge requested review from skjnldsv and come-nc July 16, 2024 08:12
@skjnldsv skjnldsv merged commit d237fd0 into master Jul 16, 2024
166 checks passed
@skjnldsv skjnldsv deleted the fix-getimagesize branch July 16, 2024 19:47
@blizzz blizzz mentioned this pull request Jul 24, 2024
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.

[Bug]: Exaggerated presence of errors into the log viewer during syncing of a new macOS device (uplod ndr!)
3 participants