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

ExifProvider.php: handle unhandled exception #35932

Closed
wants to merge 2 commits into from
Closed

ExifProvider.php: handle unhandled exception #35932

wants to merge 2 commits into from

Conversation

luxifr
Copy link

@luxifr luxifr commented Dec 31, 2022

Signed-off-by: luxifr luxifer@luxifer.fyi

Summary

Handle unhandled exception to keep files:scan run from crashing when it encounters a file with invalid exif data.

Checklist

Signed-off-by: luxifr <luxifer@luxifer.fyi>
@luxifr luxifr requested a review from CarlSchwan as a code owner December 31, 2022 19:02
Signed-off-by: luxifr <luxifer@luxifer.fyi>
@szaimen szaimen added bug 3. to review Waiting for reviews labels Jan 1, 2023
@szaimen szaimen added this to the Nextcloud 26 milestone Jan 1, 2023
@szaimen szaimen requested review from a team, ArtificialOwl, icewind1991 and blizzz and removed request for a team January 1, 2023 11:29
@PVince81 PVince81 requested a review from artonge January 4, 2023 15:33
@blizzz blizzz mentioned this pull request Feb 1, 2023
$sizeResult = getimagesizefromstring($file->getContent());
try {
$sizeResult = getimagesizefromstring($file->getContent());
} catch (\Throwable $ex) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this will work:

php > try {getimagesize('foobar');} catch(\Throwable $e){}
PHP Warning:  getimagesize(foobar): Failed to open stream: No such file or directory in php shell code on line 1

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably same reason as in #36420 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

This would work from scan command because scan command converts warning into Exception, but that’s really ugly. It should be handled in Scan command as it is the one doing the conversion.

Comment on lines +72 to +74
$sizeResult = getimagesizefromstring($file->getContent());
} catch (\Throwable $ex) {
$this->logger->warning("Couldn't get image for ".$file->getId(), ['exception' => $ex]);
Copy link
Contributor

@szaimen szaimen Apr 15, 2023

Choose a reason for hiding this comment

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

Same as in https://github.com/nextcloud/server/pull/36420/files

Suggested change
$sizeResult = getimagesizefromstring($file->getContent());
} catch (\Throwable $ex) {
$this->logger->warning("Couldn't get image for ".$file->getId(), ['exception' => $ex]);
$sizeResult = @getimagesizefromstring($file->getContent());
} catch (\Exception $ex) {
$this->logger->info("Couldn't get image for ".$file->getId(), ['exception' => $ex]);

$sizeResult = getimagesizefromstring($file->getContent());
try {
$sizeResult = getimagesizefromstring($file->getContent());
} catch (\Throwable $ex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This would work from scan command because scan command converts warning into Exception, but that’s really ugly. It should be handled in Scan command as it is the one doing the conversion.

@come-nc
Copy link
Contributor

come-nc commented Apr 27, 2023

See #37944 instead

@szaimen szaimen closed this Apr 27, 2023
@digidax
Copy link

digidax commented May 26, 2023

Bug still present in 26.0.2 when using Android App Folder Sync:
grafik

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: unhandled exception in ExifProvider crashes scan run
8 participants