Page MenuHomePhabricator

Memes use older image transform code without modern error handling
Closed, ResolvedPublic

Description

The following exception occurs when trying to render the thumbnail images for two macros:

[03-Jun-2014 23:01:17 UTC] [2014-06-04 09:01:17] EXCEPTION: (CommandException) Command failed with error #137!
COMMAND
convert '/tmp/8it37r5s2jok0kkg/11351-vbdrNb' -coalesce -resize '280'X'186.375''!' '/tmp/2855ast2qn28csoc/11351-GxEHPy'

STDOUT
(empty)

STDERR
(empty) at [/usr/src/libphutil/src/future/exec/ExecFuture.php:398]
[03-Jun-2014 23:01:17 UTC]   #0 ExecFuture::resolvex() called at [/usr/src/phabricator/src/applications/files/PhabricatorImageTransformer.php:438]
[03-Jun-2014 23:01:17 UTC]   #1 PhabricatorImageTransformer::applyScaleWithImagemagick(Object PhabricatorFile, 280, 210) called at [/usr/src/phabricator/src/applications/files/PhabricatorImageTransformer.php:140]
[03-Jun-2014 23:01:17 UTC]   #2 PhabricatorImageTransformer::crudelyScaleTo(Object PhabricatorFile, 280, 210) called at [/usr/src/phabricator/src/applications/files/PhabricatorImageTransformer.php:27]
[03-Jun-2014 23:01:17 UTC]   #3 PhabricatorImageTransformer::executeThumbTransform(Object PhabricatorFile, 280, 210) called at [/usr/src/phabricator/src/applications/files/controller/PhabricatorFileTransformController.php:156]
[03-Jun-2014 23:01:17 UTC]   #4 PhabricatorFileTransformController::executeThumbTransform(Object PhabricatorFile, 280, 210) called at [/usr/src/phabricator/src/applications/files/controller/PhabricatorFileTransformController.php:63]
[03-Jun-2014 23:01:17 UTC]   #5 PhabricatorFileTransformController::processRequest() called at [/usr/src/phabricator/webroot/index.php:95]

Related Objects

Event Timeline

joshuaspence assigned this task to epriestley.
joshuaspence raised the priority of this task from to Needs Triage.
joshuaspence updated the task description. (Show Details)
joshuaspence added projects: Macros, Files.
joshuaspence added a subscriber: joshuaspence.

Running the convert command manually and it just seems to hang.

This may also be of use:

> convert -version
Version: ImageMagick 6.6.9-7 2014-03-06 Q16 http://www.imagemagick.org
Copyright: Copyright (C) 1999-2011 ImageMagick Studio LLC
Features: OpenMP

The 137 means we killed it after 15 seconds, primarily to defuse a hilarious attack that "researchers" keep reporting where you upload a 40,000 frame GIF which doesn't actually have any image content and convert hangs.

So probably the source image is large-ish and/or has a lot of frames-ish? We should raise a better error in this case, and maybe let it run longer (30s?) and such, but fundamentally this is working as intended, approximately.

(Or convert is hanging or something, in which case there's a real issue here, but "the thing needs more than 15s to process" is an expected way to end up here.)

Yeah, ok... I let the command run for as long as it needed to. It took 1min 18sec.

Interestingly, the macro is one that I stole from here.

dancingpeople

How are thumbnails generated? Surely not on-demand per request?

They're on-demand, but cached after the first request.

I think there are a couple of issues with the {meme, src=...} form specifically -- like if you don't specify any text, we don't reuse the raw file and instead split out all the frames, put "no text" on them, and then covert them back to a GIF. This is pointless and silly.

They're on-demand, but cached after the first request.
I think there are a couple of issues with the {meme, src=...} form specifically -- like if you don't specify any text, we don't reuse the raw file and instead split out all the frames, put "no text" on them, and then covert them back to a GIF. This is pointless and silly.

Sorry, I'm probably missing something, but how is that relevant to thumbnails?

Oh, sorry, it's not directly related -- there was a similar issue somewhere else with meme text. The indirect relationship is that in both cases we attempt to process files with convert, and don't recover as gracefully as we could if the process takes too long.

Gotcha.

Perhaps that 30sec timeout should/can be made configurable?

I think the primary issue is that when this happens, it's not clear why it happened. We should surface the root cause of the error ("this image is too large") clearly. See also T2479. In some cases, we don't even try to generate the thumbnail (for very large images) and we're similarly unhelpful about this (when the user experience should be a "This image is larger than 4000x4000 pixels, so we didn't generate a thumbnail.").

Once the pipeline handles errors properly we can tweak the parameters, but if we bump this to 30 or some config value we're just going to see 10 more copies of this task in the future as users upload larger and larger GIFs.

What is the expected behavior when a user uploads an image that is "too large" for a thumbnail to be generated? I feel as if users should be free to upload any image (so long as its size is less than storage.upload-size-limit or whatever). My understanding, now, is that there are two competing limits here:

  1. The size of the original size (configurable).
  2. The amount of time required to create a thumbnail for the original image (not configurable).

They should be able to upload it, we just won't thumbnail it. In cases where we need a thumbnail we would probably substitute a generic thumbnail ("this enormous image has too much beauty to contain, click to view it" rendered over a pastoral scene or something).

The specific cases where we really need to do this are attacks: 100,000x100,000 white PNGs (which are like 30KB when compressed, so we can not reasonably limit them by setting a file upload limit), 40K frame 1x1 white GIFs (which are also very small when compressed), etc. We can and should set the limits high enough that normal users rarely encounter them (15s is probably too low), but removing them entirely means you can upload images which convert spends days processing.

(Particularly, we've seen a lot of HackerOne researchers reporting that Phabricator is "vulnerable" to these attacks, and testing them on this install. The 15s limit means I don't need to go log into the box and kill all the convert processes after we get a report. Since adding the limit, we still get these reports, but now they are "Phabricator encounters an unexpected error processing my clever attack GIF".)

They should be able to upload it, we just won't thumbnail it. In cases where we need a thumbnail we would probably substitute a generic thumbnail ("this enormous image has too much beauty to contain, click to view it" rendered over a pastoral scene or something).

This sounds reasonably actually. I hadn't considered this.

joshuaspence renamed this task from `CommandException` when trying to create a thumbnail. to `CommandException` when trying to create a thumbnail.Jul 10 2014, 7:37 PM
epriestley renamed this task from `CommandException` when trying to create a thumbnail to Memes use older image transform code without modern error handling.May 13 2015, 6:45 PM

This is now resolved for non-meme cases.

On a possibly related note, when messing with increasing the timeout to get a meme to work (memes are very important for delivering business value) I once got:

[Wed May 20 11:18:54 2015] [error] [client 10.15.15.194] [2015-05-20 11:18:54] EXCEPTION: (AphrontDuplicateKeyQueryException) #1062: Duplicate entry 'PHID-FILE-3wzck3h3xe2qkeapqsvb-meme3014bf6dfef23870bb4bbb4a4f444' for key '
originalPHID' at [<phutil>/src/aphront/storage/connection/mysql/AphrontBaseMySQLDatabaseConnection.php:300], referer: http://phabricator.clearspring.local/T54295

but don't have a consistent repro case.

I'll file that separately.

enko added a subscriber: enko.May 27 2015, 10:45 PM

I do not know if this is related:

https://phablab.krautspace.de/macro/meme/?macro=ohno

Stacktrace looks like this:

May 28 00:44:16 web ool www: [2015-05-28 00:44:16] EXCEPTION: (CommandException) Command failed with error #1!#012COMMAND#012convert '/tmp/7proc72l3ns40sk4/22782-rOBjQy' info:#012#012STDOUT#012(empty)#012#012STDERR#012convert convert: Unable to open file () [No such file or directory].#012 at [<phutil>/src/future/exec/ExecFuture.php:416]
May 28 00:44:16 web ool www: arcanist(head=master, ref.master=f4aadb96048b), phabricator(head=master, ref.master=50240eda03e1), phutil(head=master, ref.master=c2cd90ee7aec)
May 28 00:44:16 web ool www:   #0 phlog(CommandException) called at [<phabricator>/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php:226]
May 28 00:44:16 web ool www:   #1 AphrontDefaultApplicationConfiguration::handleException(CommandException) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:230]
May 28 00:44:16 web ool www:   #2 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:140]
May 28 00:44:16 web ool www:   #3 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phabricator>/webroot/index.php:19]
epriestley moved this task from Backlog to vNext on the Files board.Nov 24 2015, 4:58 PM
eadler added a project: Restricted Project.Aug 5 2016, 5:24 PM