Stop trying to assess the image dimensions of large files and file chunks

Summary:
Depends on D18828. Ref T7789. See <https://discourse.phabricator-community.org/t/git-lfs-fails-with-large-images/584>.

Currently, when you upload a large (>4MB) image, we may try to assess the dimensions for the image and for each individual chunk.

At best, this is slow and not useful. At worst, it fatals or consumes a ton of memory and I/O we don't need to be using.

Instead:

- Don't try to assess dimensions for chunked files.
- Don't try to assess dimensions for the chunks themselves.
- Squelch errors for bad data, etc., that `gd` can't actually read, since we recover sensibly.

Test Plan:
- Created a 2048x2048 PNG in Photoshop using the "Random Noise" filter which weighs 8.5MB.
- Uploaded it.
- Before patch: got complaints in log about `imagecreatefromstring()` failing, although the actual upload went OK in my environment.
- After patch: clean log, no attempt to detect the size of a big image.
- Also uploaded a small image, got dimensions detected properly still.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7789

Differential Revision: https://secure.phabricator.com/D18830

epriestley 8 years ago e1d6bad8 5295840a

+31 -2

3 changed files

expand all

src

applications

files

conduit

FileUploadChunkConduitAPIMethod.php

storage

PhabricatorFile.php

uploadsource

PhabricatorFileUploadSource.php

src/applications/files/conduit/FileUploadChunkConduitAPIMethod.php

··· 64 64 $params = array( 65 65 'name' => $file->getMonogram().'.chunk-'.$chunk->getID(), 66 66 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, 67 + 'chunk' => true, 67 68 ); 68 69 69 70 if ($mime_type !== null) {

+29 -2

src/applications/files/storage/PhabricatorFile.php

··· 40 40 const METADATA_PROFILE = 'profile'; 41 41 const METADATA_STORAGE = 'storage'; 42 42 const METADATA_INTEGRITY = 'integrity'; 43 + const METADATA_CHUNK = 'chunk'; 43 44 44 45 const STATUS_ACTIVE = 'active'; 45 46 const STATUS_DELETED = 'deleted'; ··· 410 411 try { 411 412 $file->updateDimensions(false); 412 413 } catch (Exception $ex) { 413 - // Do nothing 414 + // Do nothing. 414 415 } 415 416 416 417 $file->saveAndIndex(); ··· 1057 1058 throw new Exception(pht('Cannot retrieve image information.')); 1058 1059 } 1059 1060 1061 + if ($this->getIsChunk()) { 1062 + throw new Exception( 1063 + pht('Refusing to assess image dimensions of file chunk.')); 1064 + } 1065 + 1066 + $engine = $this->instantiateStorageEngine(); 1067 + if ($engine->isChunkEngine()) { 1068 + throw new Exception( 1069 + pht('Refusing to assess image dimensions of chunked file.')); 1070 + } 1071 + 1060 1072 $data = $this->loadFileData(); 1061 1073 1062 - $img = imagecreatefromstring($data); 1074 + $img = @imagecreatefromstring($data); 1063 1075 if ($img === false) { 1064 1076 throw new Exception(pht('Error when decoding image.')); 1065 1077 } ··· 1255 1267 return $this; 1256 1268 } 1257 1269 1270 + public function getIsChunk() { 1271 + return idx($this->metadata, self::METADATA_CHUNK); 1272 + } 1273 + 1274 + public function setIsChunk($value) { 1275 + $this->metadata[self::METADATA_CHUNK] = $value; 1276 + return $this; 1277 + } 1278 + 1258 1279 public function setIntegrityHash($integrity_hash) { 1259 1280 $this->metadata[self::METADATA_INTEGRITY] = $integrity_hash; 1260 1281 return $this; ··· 1339 1360 'mime-type' => 'optional string', 1340 1361 'builtin' => 'optional string', 1341 1362 'storageEngines' => 'optional list<PhabricatorFileStorageEngine>', 1363 + 'chunk' => 'optional bool', 1342 1364 )); 1343 1365 1344 1366 $file_name = idx($params, 'name'); ··· 1414 1436 $mime_type = idx($params, 'mime-type'); 1415 1437 if ($mime_type) { 1416 1438 $this->setMimeType($mime_type); 1439 + } 1440 + 1441 + $is_chunk = idx($params, 'chunk'); 1442 + if ($is_chunk) { 1443 + $this->setIsChunk(true); 1417 1444 } 1418 1445 1419 1446 return $this;

src/applications/files/uploadsource/PhabricatorFileUploadSource.php

··· 189 189 $params = array( 190 190 'name' => $file->getMonogram().'.chunk-'.$offset, 191 191 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, 192 + 'chunk' => true, 192 193 ); 193 194 194 195 // If this isn't the initial chunk, provide a dummy MIME type so we do not

Configure Feed

Configure Feed