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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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)

Signed-off-by: Josh Richards <josh.t.richards@gmail.com>
@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...

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

No.

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!)
2 participants