From 4dfd90abc423ae34add34a2293c24de8b398246c Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 1 May 2017 14:03:00 +0200 Subject: [PATCH 1/4] better handling of preview generation errors Signed-off-by: Robin Appelman --- lib/private/Preview/Generator.php | 5 +++++ lib/private/legacy/image.php | 8 ++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/private/Preview/Generator.php b/lib/private/Preview/Generator.php index fd75e51b63..0fb284259a 100644 --- a/lib/private/Preview/Generator.php +++ b/lib/private/Preview/Generator.php @@ -303,6 +303,10 @@ class Generator { private function generatePreview(ISimpleFolder $previewFolder, ISimpleFile $maxPreview, $width, $height, $crop, $maxWidth, $maxHeight) { $preview = $this->helper->getImage($maxPreview); + if (!$preview->valid()) { + throw new \InvalidArgumentException('Failed to generate preview, failed to load image'); + } + if ($crop) { if ($height !== $preview->height() && $width !== $preview->width()) { //Resize @@ -325,6 +329,7 @@ class Generator { $preview->resize(max($width, $height)); } + $path = $this->generatePath($width, $height, $crop); try { $file = $previewFolder->newFile($path); diff --git a/lib/private/legacy/image.php b/lib/private/legacy/image.php index e26148bdf1..120b19d1cf 100644 --- a/lib/private/legacy/image.php +++ b/lib/private/legacy/image.php @@ -563,7 +563,7 @@ class OC_Image implements \OCP\IImage { case IMAGETYPE_JPEG: if (imagetypes() & IMG_JPG) { if (getimagesize($imagePath) !== false) { - $this->resource = imagecreatefromjpeg($imagePath); + $this->resource = @imagecreatefromjpeg($imagePath); } else { $this->logger->debug('OC_Image->loadFromFile, JPG image not valid: ' . $imagePath, array('app' => 'core')); } @@ -573,7 +573,7 @@ class OC_Image implements \OCP\IImage { break; case IMAGETYPE_PNG: if (imagetypes() & IMG_PNG) { - $this->resource = imagecreatefrompng($imagePath); + $this->resource = @imagecreatefrompng($imagePath); // Preserve transparency imagealphablending($this->resource, true); imagesavealpha($this->resource, true); @@ -583,14 +583,14 @@ class OC_Image implements \OCP\IImage { break; case IMAGETYPE_XBM: if (imagetypes() & IMG_XPM) { - $this->resource = imagecreatefromxbm($imagePath); + $this->resource = @imagecreatefromxbm($imagePath); } else { $this->logger->debug('OC_Image->loadFromFile, XBM/XPM images not supported: ' . $imagePath, array('app' => 'core')); } break; case IMAGETYPE_WBMP: if (imagetypes() & IMG_WBMP) { - $this->resource = imagecreatefromwbmp($imagePath); + $this->resource = @imagecreatefromwbmp($imagePath); } else { $this->logger->debug('OC_Image->loadFromFile, WBMP images not supported: ' . $imagePath, array('app' => 'core')); } From 6aac094091076d503d6376ca74d2bedadff63de7 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Mon, 1 May 2017 18:35:47 -0300 Subject: [PATCH 2/4] Add PHPDoc Signed-off-by: Morris Jobke --- lib/private/Preview/Generator.php | 2 ++ lib/private/PreviewManager.php | 3 ++- lib/public/IPreview.php | 3 ++- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/private/Preview/Generator.php b/lib/private/Preview/Generator.php index 0fb284259a..5a264c2bbd 100644 --- a/lib/private/Preview/Generator.php +++ b/lib/private/Preview/Generator.php @@ -84,6 +84,7 @@ class Generator { * @param string $mimeType * @return ISimpleFile * @throws NotFoundException + * @throws \InvalidArgumentException if the preview would be invalid (in case the original image is invalid) */ public function getPreview(File $file, $width = -1, $height = -1, $crop = false, $mode = IPreview::MODE_FILL, $mimeType = null) { $this->eventDispatcher->dispatch( @@ -299,6 +300,7 @@ class Generator { * @param int $maxHeight * @return ISimpleFile * @throws NotFoundException + * @throws \InvalidArgumentException if the preview would be invalid (in case the original image is invalid) */ private function generatePreview(ISimpleFolder $previewFolder, ISimpleFile $maxPreview, $width, $height, $crop, $maxWidth, $maxHeight) { $preview = $this->helper->getImage($maxPreview); diff --git a/lib/private/PreviewManager.php b/lib/private/PreviewManager.php index 8c5a7ad29f..12fcc292c6 100644 --- a/lib/private/PreviewManager.php +++ b/lib/private/PreviewManager.php @@ -182,7 +182,8 @@ class PreviewManager implements IPreview { * @param string $mimeType * @return ISimpleFile * @throws NotFoundException - * @since 11.0.0 + * @throws \InvalidArgumentException if the preview would be invalid (in case the original image is invalid) + * @since 11.0.0 - \InvalidArgumentException was added in 12.0.0 */ public function getPreview(File $file, $width = -1, $height = -1, $crop = false, $mode = IPreview::MODE_FILL, $mimeType = null) { if ($this->generator === null) { diff --git a/lib/public/IPreview.php b/lib/public/IPreview.php index 207539b117..7705df61a1 100644 --- a/lib/public/IPreview.php +++ b/lib/public/IPreview.php @@ -104,7 +104,8 @@ interface IPreview { * @param string $mimeType To force a given mimetype for the file (files_versions needs this) * @return ISimpleFile * @throws NotFoundException - * @since 11.0.0 + * @throws \InvalidArgumentException if the preview would be invalid (in case the original image is invalid) + * @since 11.0.0 - \InvalidArgumentException was added in 12.0.0 */ public function getPreview(File $file, $width = -1, $height = -1, $crop = false, $mode = IPreview::MODE_FILL, $mimeType = null); From 23cc309606443003c3478d96452998b109ea9c09 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Mon, 1 May 2017 18:41:37 -0300 Subject: [PATCH 3/4] Handle more error cases Signed-off-by: Morris Jobke --- apps/files_sharing/lib/Controller/PublicPreviewController.php | 2 ++ apps/files_trashbin/lib/Controller/PreviewController.php | 2 ++ apps/files_versions/lib/Controller/PreviewController.php | 2 ++ core/Controller/PreviewController.php | 2 ++ 4 files changed, 8 insertions(+) diff --git a/apps/files_sharing/lib/Controller/PublicPreviewController.php b/apps/files_sharing/lib/Controller/PublicPreviewController.php index b91b84c9c3..49e48993f5 100644 --- a/apps/files_sharing/lib/Controller/PublicPreviewController.php +++ b/apps/files_sharing/lib/Controller/PublicPreviewController.php @@ -97,6 +97,8 @@ class PublicPreviewController extends Controller { return new FileDisplayResponse($f, Http::STATUS_OK, ['Content-Type' => $f->getMimeType()]); } catch (NotFoundException $e) { return new DataResponse([], Http::STATUS_NOT_FOUND); + } catch (\InvalidArgumentException $e) { + return new DataResponse([], Http::STATUS_BAD_REQUEST); } } } diff --git a/apps/files_trashbin/lib/Controller/PreviewController.php b/apps/files_trashbin/lib/Controller/PreviewController.php index c73b1c17c1..ae3a106d62 100644 --- a/apps/files_trashbin/lib/Controller/PreviewController.php +++ b/apps/files_trashbin/lib/Controller/PreviewController.php @@ -114,6 +114,8 @@ class PreviewController extends Controller { return new Http\FileDisplayResponse($f, Http::STATUS_OK, ['Content-Type' => $f->getMimeType()]); } catch (NotFoundException $e) { return new DataResponse([], Http::STATUS_NOT_FOUND); + } catch (\InvalidArgumentException $e) { + return new DataResponse([], Http::STATUS_BAD_REQUEST); } } } diff --git a/apps/files_versions/lib/Controller/PreviewController.php b/apps/files_versions/lib/Controller/PreviewController.php index 8d961f47ee..7a7c024fb5 100644 --- a/apps/files_versions/lib/Controller/PreviewController.php +++ b/apps/files_versions/lib/Controller/PreviewController.php @@ -94,6 +94,8 @@ class PreviewController extends Controller { return new FileDisplayResponse($f, Http::STATUS_OK, ['Content-Type' => $f->getMimeType()]); } catch (NotFoundException $e) { return new DataResponse([], Http::STATUS_NOT_FOUND); + } catch (\InvalidArgumentException $e) { + return new DataResponse([], Http::STATUS_BAD_REQUEST); } } } diff --git a/core/Controller/PreviewController.php b/core/Controller/PreviewController.php index 4b9ba8b60d..4e2b2f0287 100644 --- a/core/Controller/PreviewController.php +++ b/core/Controller/PreviewController.php @@ -126,6 +126,8 @@ class PreviewController extends Controller { return $response; } catch (NotFoundException $e) { return new DataResponse([], Http::STATUS_NOT_FOUND); + } catch (\InvalidArgumentException $e) { + return new DataResponse([], Http::STATUS_BAD_REQUEST); } } From 2847e9f2e3e5f338ba1b727ea207b2c549d38e92 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 2 May 2017 13:23:02 +0200 Subject: [PATCH 4/4] fix preview tests Signed-off-by: Robin Appelman --- tests/lib/Preview/GeneratorTest.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/lib/Preview/GeneratorTest.php b/tests/lib/Preview/GeneratorTest.php index 50d46ae932..f1383b0691 100644 --- a/tests/lib/Preview/GeneratorTest.php +++ b/tests/lib/Preview/GeneratorTest.php @@ -169,6 +169,7 @@ class GeneratorTest extends \Test\TestCase { $image = $this->createMock(IImage::class); $image->method('width')->willReturn(2048); $image->method('height')->willReturn(2048); + $image->method('valid')->willReturn(true); $this->helper->method('getThumbnail') ->will($this->returnCallback(function ($provider, $file, $x, $y) use ($invalidProvider, $validProvider, $image) { @@ -217,6 +218,7 @@ class GeneratorTest extends \Test\TestCase { ->with(128); $image->method('data') ->willReturn('my resized data'); + $image->method('valid')->willReturn(true); $previewFile->expects($this->once()) ->method('putContent') @@ -379,6 +381,7 @@ class GeneratorTest extends \Test\TestCase { ->willReturn($image); $image->method('height')->willReturn($maxY); $image->method('width')->willReturn($maxX); + $image->method('valid')->willReturn(true); $preview = $this->createMock(ISimpleFile::class); $previewFolder->method('newFile')