From 36cfe9f85774e037b4eeaebc19f9e78d149f7507 Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Fri, 15 Jul 2016 12:52:01 +0200 Subject: [PATCH 01/11] Delete successfully generated thumbnail (temporary sources) too. --- lib/imagefile.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/imagefile.php b/lib/imagefile.php index b0cd13089f..626221d9a8 100644 --- a/lib/imagefile.php +++ b/lib/imagefile.php @@ -151,10 +151,14 @@ class ImageFile try { $image = new ImageFile($file->getID(), $imgPath); - } catch (UnsupportedMediaException $e) { + } catch (Exception $e) { + common_debug(sprintf('Exception caught when creating ImageFile for File id==%s and imgPath==', _ve($file->id), _ve($imgPath))); + throw $e; + } finally { // Avoid deleting the original try { - if ($imgPath !== $file->getPath()) { + if (strlen($imgPath) > 0 && $imgPath !== $file->getPath()) { + common_debug(__METHOD__.': Deleting temporary file that was created as image file thumbnail source: '._ve($imgPath)); @unlink($imgPath); } } catch (FileNotFoundException $e) { @@ -162,7 +166,6 @@ class ImageFile // doesn't exist anyway, so it's safe to delete $imgPath @unlink($imgPath); } - throw $e; } return $image; } From 46c227bf3a14e98b15c4935390f2e009ed904f8b Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Fri, 15 Jul 2016 13:19:16 +0200 Subject: [PATCH 02/11] FileNotFoundException is more proper here --- lib/imagefile.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/imagefile.php b/lib/imagefile.php index 626221d9a8..ab59e16b25 100644 --- a/lib/imagefile.php +++ b/lib/imagefile.php @@ -146,7 +146,7 @@ class ImageFile } if (!file_exists($imgPath)) { - throw new ServerException(sprintf('Image not available locally: %s', $imgPath)); + throw new FileNotFoundException($imgPath); } try { From fc440ba7e767803fb57644fc9af53c14706055de Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Wed, 20 Jul 2016 22:51:38 +0200 Subject: [PATCH 03/11] Easier debugging of VideoThumbnails plugin --- plugins/VideoThumbnails/VideoThumbnailsPlugin.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/plugins/VideoThumbnails/VideoThumbnailsPlugin.php b/plugins/VideoThumbnails/VideoThumbnailsPlugin.php index e8cf7e31f2..fc3925375f 100644 --- a/plugins/VideoThumbnails/VideoThumbnailsPlugin.php +++ b/plugins/VideoThumbnails/VideoThumbnailsPlugin.php @@ -67,7 +67,9 @@ class VideoThumbnailsPlugin extends Plugin common_log(LOG_ERR, 'Neither ffmpeg nor avconv was found in your PATH. Cannot create video thumbnail.'); return true; } - $result = exec($cmd.' -y -i '.escapeshellarg($file->getPath()).' -vcodec mjpeg -vframes 1 -f image2 -an '.escapeshellarg($tmp_imgPath)); + $fullcmd = $cmd.' -y -i '.escapeshellarg($file->getPath()).' -vcodec mjpeg -vframes 1 -f image2 -an '.escapeshellarg($tmp_imgPath); + common_debug(__METHOD__ . ' executing: '._ve($fullcmd)); + $result = exec($fullcmd); if (!getimagesize($tmp_imgPath)) { common_debug('exec of "avconv" produced a bad/nonexisting image it seems'); From e8e996182fce9979d9380243944b5bea06db65eb Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Thu, 21 Jul 2016 00:23:27 +0200 Subject: [PATCH 04/11] Delete file on class destruction or we do it too quickly Source image was removed when trying to use it for resizeTo --- lib/imagefile.php | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/imagefile.php b/lib/imagefile.php index ab59e16b25..0d711ad986 100644 --- a/lib/imagefile.php +++ b/lib/imagefile.php @@ -120,6 +120,14 @@ class ImageFile Event::handle('FillImageFileMetadata', array($this)); } + public function __destruct() + { + if (strlen($this->filepath) > 0 && (!$this->fileRecord instanceof File || $this->filepath !== $this->fileRecord->getPath())) { + common_debug(__METHOD__.': Deleting temporary file that was created as image file thumbnail source: '._ve($this->filepath)); + @unlink($this->filepath); + } + } + public static function fromFileObject(File $file) { $imgPath = null; @@ -152,9 +160,6 @@ class ImageFile try { $image = new ImageFile($file->getID(), $imgPath); } catch (Exception $e) { - common_debug(sprintf('Exception caught when creating ImageFile for File id==%s and imgPath==', _ve($file->id), _ve($imgPath))); - throw $e; - } finally { // Avoid deleting the original try { if (strlen($imgPath) > 0 && $imgPath !== $file->getPath()) { @@ -166,6 +171,8 @@ class ImageFile // doesn't exist anyway, so it's safe to delete $imgPath @unlink($imgPath); } + common_debug(sprintf('Exception caught when creating ImageFile for File id==%s and imgPath==', _ve($file->id), _ve($imgPath))); + throw $e; } return $image; } @@ -587,7 +594,7 @@ class ImageFile list($width, $height, $x, $y, $w, $h) = $this->scaleToFit($width, $height, $crop); $thumb = File_thumbnail::pkeyGet(array( - 'file_id'=> $this->fileRecord->id, + 'file_id'=> $this->fileRecord->getID(), 'width' => $width, 'height' => $height, )); @@ -597,7 +604,7 @@ class ImageFile $filename = $this->fileRecord->filehash ?: $this->filename; // Remote files don't have $this->filehash $extension = File::guessMimeExtension($this->mimetype); - $outname = "thumb-{$this->fileRecord->id}-{$width}x{$height}-{$filename}." . $extension; + $outname = "thumb-{$this->fileRecord->getID()}-{$width}x{$height}-{$filename}." . $extension; $outpath = File_thumbnail::path($outname); // The boundary box for our resizing @@ -615,7 +622,7 @@ class ImageFile throw new ServerException('Bad thumbnail size parameters.'); } - common_debug(sprintf('Generating a thumbnail of File id==%u of size %ux%u', $this->fileRecord->id, $width, $height)); + common_debug(sprintf('Generating a thumbnail of File id==%u of size %ux%u', $this->fileRecord->getID(), $width, $height)); // Perform resize and store into file $this->resizeTo($outpath, $box); From 13e1f0a56190cb6c602c2cbced06b706f07006de Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Thu, 21 Jul 2016 00:24:05 +0200 Subject: [PATCH 05/11] VideoThumbnails shouldn't have to recreate the thumbnail all the time --- plugins/VideoThumbnails/VideoThumbnailsPlugin.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/plugins/VideoThumbnails/VideoThumbnailsPlugin.php b/plugins/VideoThumbnails/VideoThumbnailsPlugin.php index fc3925375f..30b62677ee 100644 --- a/plugins/VideoThumbnails/VideoThumbnailsPlugin.php +++ b/plugins/VideoThumbnails/VideoThumbnailsPlugin.php @@ -55,6 +55,21 @@ class VideoThumbnailsPlugin extends Plugin return true; } + try { + // Exception thrown if no thumbnail found + $thumb = File_thumbnail::byFile($file, false); + // If getPath doesn't throw an exception, we have a working locally stored thumbnail + return $thumb->getPath(); + } catch (NoResultException $e) { + // Alright, no thumbnail found, so let's create one. + } catch (InvalidFilenameException $e) { + // I guess this means $thumb->filename is null? Shouldn't happen because $file->filename is not null, so delete it + $thumb->delete(); + } catch (FileNotFoundException $e) { + // Thumb file was not found, let's delete it. + $thumb->delete(); + } + // Let's save our frame to a temporary file. If we fail, remove it. $tmp_imgPath = tempnam(sys_get_temp_dir(), 'socialthumb-'); From d230d332cf392edfda823d29c93489e2ef739a96 Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Thu, 21 Jul 2016 00:27:22 +0200 Subject: [PATCH 06/11] return false to exit event, imgPath holds the path --- plugins/VideoThumbnails/VideoThumbnailsPlugin.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/plugins/VideoThumbnails/VideoThumbnailsPlugin.php b/plugins/VideoThumbnails/VideoThumbnailsPlugin.php index 30b62677ee..5b96653db1 100644 --- a/plugins/VideoThumbnails/VideoThumbnailsPlugin.php +++ b/plugins/VideoThumbnails/VideoThumbnailsPlugin.php @@ -58,8 +58,9 @@ class VideoThumbnailsPlugin extends Plugin try { // Exception thrown if no thumbnail found $thumb = File_thumbnail::byFile($file, false); - // If getPath doesn't throw an exception, we have a working locally stored thumbnail - return $thumb->getPath(); + $imgPath = $thumb->getPath(); + // If getPath didn't throw an exception, we have a working locally stored thumbnail + return false; } catch (NoResultException $e) { // Alright, no thumbnail found, so let's create one. } catch (InvalidFilenameException $e) { From d5c733919b3f80b4fe8f37f99e63e57e3b76ba47 Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Thu, 21 Jul 2016 00:34:40 +0200 Subject: [PATCH 07/11] Because the other part of the code works now, this is unnecessary --- lib/imagefile.php | 8 -------- 1 file changed, 8 deletions(-) diff --git a/lib/imagefile.php b/lib/imagefile.php index 0d711ad986..22fdda9d30 100644 --- a/lib/imagefile.php +++ b/lib/imagefile.php @@ -120,14 +120,6 @@ class ImageFile Event::handle('FillImageFileMetadata', array($this)); } - public function __destruct() - { - if (strlen($this->filepath) > 0 && (!$this->fileRecord instanceof File || $this->filepath !== $this->fileRecord->getPath())) { - common_debug(__METHOD__.': Deleting temporary file that was created as image file thumbnail source: '._ve($this->filepath)); - @unlink($this->filepath); - } - } - public static function fromFileObject(File $file) { $imgPath = null; From 1981cb76622977ed3a0a480bfcb427b09ca3c688 Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Thu, 21 Jul 2016 00:37:43 +0200 Subject: [PATCH 08/11] Select the first generated thumbnail, which should be the proper size ...most of the time. If all works well. --- classes/File_thumbnail.php | 1 + 1 file changed, 1 insertion(+) diff --git a/classes/File_thumbnail.php b/classes/File_thumbnail.php index 968883f2b8..91ba653744 100644 --- a/classes/File_thumbnail.php +++ b/classes/File_thumbnail.php @@ -98,6 +98,7 @@ class File_thumbnail extends Managed_DataObject if ($notNullUrl) { $thumb->whereAdd('url IS NOT NULL'); } + $thumb->orderBy('modified ASC'); // the first created, a somewhat ugly hack $thumb->limit(1); if (!$thumb->find(true)) { throw new NoResultException($thumb); From e52275e37f8c1593ef2214fcdf40bdc226f8758b Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Thu, 21 Jul 2016 01:38:31 +0200 Subject: [PATCH 09/11] Some comparisons were incorrect (text/html;charset=utf-8 etc.) --- lib/attachmentlistitem.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/attachmentlistitem.php b/lib/attachmentlistitem.php index 133756147c..934c5f74dd 100644 --- a/lib/attachmentlistitem.php +++ b/lib/attachmentlistitem.php @@ -174,7 +174,7 @@ class AttachmentListItem extends Widget default: unset($thumb); // there's no need carrying this along - switch ($this->attachment->mimetype) { + switch (common_bare_mime($this->attachment->mimetype)) { case 'text/plain': $this->element('div', ['class'=>'e-content plaintext'], file_get_contents($this->attachment->getPath())); break; From 809e2f6d075402b3fdf81f6168e5841ac69f0718 Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Thu, 21 Jul 2016 01:38:48 +0200 Subject: [PATCH 10/11] Use File->getID() --- plugins/Oembed/OembedPlugin.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/plugins/Oembed/OembedPlugin.php b/plugins/Oembed/OembedPlugin.php index 6950fb0a9a..652da64ffd 100644 --- a/plugins/Oembed/OembedPlugin.php +++ b/plugins/Oembed/OembedPlugin.php @@ -102,7 +102,7 @@ class OembedPlugin extends Plugin array(), array('format'=>'json', 'url'=> common_local_url('attachment', - array('attachment' => $action->attachment->id)))), + array('attachment' => $action->attachment->getID())))), 'title'=>'oEmbed'),null); $action->element('link',array('rel'=>'alternate', 'type'=>'text/xml+oembed', @@ -111,7 +111,7 @@ class OembedPlugin extends Plugin array(), array('format'=>'xml','url'=> common_local_url('attachment', - array('attachment' => $action->attachment->id)))), + array('attachment' => $action->attachment->getID())))), 'title'=>'oEmbed'),null); break; case 'shownotice': @@ -159,9 +159,9 @@ class OembedPlugin extends Plugin */ public function onEndFileSaveNew(File $file) { - $fo = File_oembed::getKV('file_id', $file->id); + $fo = File_oembed::getKV('file_id', $file->getID()); if ($fo instanceof File_oembed) { - common_log(LOG_WARNING, "Strangely, a File_oembed object exists for new file {$file->id}", __FILE__); + common_log(LOG_WARNING, "Strangely, a File_oembed object exists for new file {$file->getID()}", __FILE__); return true; } @@ -178,14 +178,14 @@ class OembedPlugin extends Plugin return true; } - File_oembed::saveNew($oembed_data, $file->id); + File_oembed::saveNew($oembed_data, $file->getID()); } return true; } public function onEndShowAttachmentLink(HTMLOutputter $out, File $file) { - $oembed = File_oembed::getKV('file_id', $file->id); + $oembed = File_oembed::getKV('file_id', $file->getID()); if (empty($oembed->author_name) && empty($oembed->provider)) { return true; } @@ -217,7 +217,7 @@ class OembedPlugin extends Plugin { // Never treat generic HTML links as an enclosure type! // But if we have oEmbed info, we'll consider it golden. - $oembed = File_oembed::getKV('file_id', $file->id); + $oembed = File_oembed::getKV('file_id', $file->getID()); if (!$oembed instanceof File_oembed || !in_array($oembed->type, array('photo', 'video'))) { return true; } From 1b3d5834183230180efb11d43365e68cb557b2cd Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Thu, 21 Jul 2016 03:19:05 +0200 Subject: [PATCH 11/11] file_quota for OembedPlugin too Don't download huge files that might kill memory limits. --- classes/File_thumbnail.php | 4 +++ plugins/Oembed/OembedPlugin.php | 32 +++++++++++++++---- .../StoreRemoteMediaPlugin.php | 4 +-- 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/classes/File_thumbnail.php b/classes/File_thumbnail.php index 91ba653744..68cb2a737f 100644 --- a/classes/File_thumbnail.php +++ b/classes/File_thumbnail.php @@ -273,6 +273,10 @@ class File_thumbnail extends Managed_DataObject return File::getByID($this->file_id); } + public function getFileId() + { + return $this->file_id; + } static public function hashurl($url) { diff --git a/plugins/Oembed/OembedPlugin.php b/plugins/Oembed/OembedPlugin.php index 652da64ffd..716649c68c 100644 --- a/plugins/Oembed/OembedPlugin.php +++ b/plugins/Oembed/OembedPlugin.php @@ -377,16 +377,36 @@ class OembedPlugin extends Plugin protected function storeRemoteFileThumbnail(File_thumbnail $thumbnail) { if (!empty($thumbnail->filename) && file_exists($thumbnail->getPath())) { - throw new AlreadyFulfilledException(sprintf('A thumbnail seems to already exist for remote file with id==%u', $thumbnail->file_id)); + throw new AlreadyFulfilledException(sprintf('A thumbnail seems to already exist for remote file with id==%u', $thumbnail->getFileId())); } - $url = $thumbnail->getUrl(); - $this->checkWhitelist($url); + $remoteUrl = $thumbnail->getUrl(); + $this->checkWhitelist($remoteUrl); - // First we download the file to memory and test whether it's actually an image file + $http = new HTTPClient(); + // First see if it's too large for us + common_debug(__METHOD__ . ': '.sprintf('Performing HEAD request for remote file id==%u to avoid unnecessarily downloading too large files. URL: %s', $thumbnail->getFileId(), $remoteUrl)); + $head = $http->head($remoteUrl); + $remoteUrl = $head->getEffectiveUrl(); // to avoid going through redirects again + + $headers = $head->getHeader(); + $filesize = isset($headers['content-length']) ? $headers['content-length'] : null; + + // FIXME: I just copied some checks from StoreRemoteMedia, maybe we should have other checks for thumbnails? Or at least embed into some class somewhere. + if (empty($filesize)) { + // file size not specified on remote server + common_debug(sprintf('%s: Ignoring remote thumbnail because we did not get a content length for thumbnail for file id==%u', __CLASS__, $thumbnail->getFileId())); + return true; + } elseif ($filesize > common_config('attachments', 'file_quota')) { + // file too big according to site configuration + common_debug(sprintf('%s: Skip downloading remote thumbnail because content length (%u) is larger than file_quota (%u) for file id==%u', __CLASS__, intval($filesize), common_config('attachments', 'file_quota'), $thumbnail->getFileId())); + return true; + } + + // Then we download the file to memory and test whether it's actually an image file // FIXME: To support remote video/whatever files, this needs reworking. - common_debug(sprintf('Downloading remote thumbnail for file id==%u with thumbnail URL: %s', $thumbnail->file_id, $url)); - $imgData = HTTPClient::quickGet($url); + common_debug(sprintf('Downloading remote thumbnail for file id==%u (should be size %u) with effective URL: %s', $thumbnail->getFileId(), $filesize, _ve($remoteUrl))); + $imgData = HTTPClient::quickGet($remoteUrl); $info = @getimagesizefromstring($imgData); if ($info === false) { throw new UnsupportedMediaException(_('Remote file format was not identified as an image.'), $url); diff --git a/plugins/StoreRemoteMedia/StoreRemoteMediaPlugin.php b/plugins/StoreRemoteMedia/StoreRemoteMediaPlugin.php index 2e42802724..e171218f79 100644 --- a/plugins/StoreRemoteMedia/StoreRemoteMediaPlugin.php +++ b/plugins/StoreRemoteMedia/StoreRemoteMediaPlugin.php @@ -91,7 +91,7 @@ class StoreRemoteMediaPlugin extends Plugin $http = new HTTPClient(); common_debug(sprintf('Performing HEAD request for remote file id==%u to avoid unnecessarily downloading too large files. URL: %s', $file->getID(), $remoteUrl)); $head = $http->head($remoteUrl); - $remoteUrl = $head->effectiveUrl; // to avoid going through redirects again + $remoteUrl = $head->getEffectiveUrl(); // to avoid going through redirects again if (!$this->checkBlackList($remoteUrl)) { common_log(LOG_WARN, sprintf('%s: Non-blacklisted URL %s redirected to blacklisted URL %s', __CLASS__, $file->getUrl(), $remoteUrl)); return true; @@ -120,7 +120,7 @@ class StoreRemoteMediaPlugin extends Plugin common_debug(sprintf('Downloading remote file id==%u (should be size %u) with effective URL: %s', $file->getID(), $filesize, _ve($remoteUrl))); $imgData = HTTPClient::quickGet($remoteUrl); } catch (HTTP_Request2_ConnectionException $e) { - common_log(LOG_ERR, __CLASS__.': quickGet on URL: '._ve($file->getUrl()).' threw exception: '.$e->getMessage()); + common_log(LOG_ERR, __CLASS__.': '._ve(get_class($e)).' on URL: '._ve($file->getUrl()).' threw exception: '.$e->getMessage()); return true; } $info = @getimagesizefromstring($imgData);