From 354e54a0aff6809aab6f197c97489112774df043 Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Sun, 1 Mar 2020 17:00:18 +0100 Subject: [PATCH 1/6] Limit album/tag downloading on a user basis. This removes the limit of downloading albums only; tags are fine, too. Now using UserFacingException for certain exceptions, as these are displayed to the user. Removing the inheritance of HTMLController, as we intend to output binary data only. --- controllers/Download.php | 36 ++++++++---------------------------- 1 file changed, 8 insertions(+), 28 deletions(-) diff --git a/controllers/Download.php b/controllers/Download.php index b6e2e6c2..b22b92b1 100644 --- a/controllers/Download.php +++ b/controllers/Download.php @@ -6,7 +6,7 @@ * Kabuki CMS (C) 2013-2019, Aaron van Geffen *****************************************************************************/ -class Download extends HTMLController +class Download { public function __construct() { @@ -15,38 +15,18 @@ class Download extends HTMLController if (!$user->isLoggedIn()) throw new NotAllowedException(); - if(!isset($_GET['tag'])) - throw new UnexpectedValueException('Must specify an album to download'); + if (!isset($_GET['tag'])) + throw new UserFacingException('No album or tag has been specified for download.'); $tag = (int)$_GET['tag']; $album = Tag::fromId($tag); - if($album->kind !== 'Album') - throw new UnexpectedValueException('Specified tag does not correspond to an album'); + if (isset($_SESSION['current_export'])) + throw new UserFacingException('An export of "' . $tag->tag . '" is ongoing. Please try again later.'); - //Yes TOCTOU but it does not need to be perfect. - $lock_file = join('/', [sys_get_temp_dir(), 'pics-export.lock']); - if(!file_exists($lock_file)) - { - try - { - $fp = fopen($lock_file, 'x'); - - if(!$fp) - throw new UnexpectedValueException('Could not open lock-file'); - - $this->exportAlbum($album); - } - finally - { - fclose($fp); - unlink($lock_file); - } - } - else - throw new UnexpectedValueException('Another export is busy, please try again later'); - - exit(); + // So far so good? + $this->exportAlbum($album); + exit; } private function exportAlbum($album) From 1b7e83e11ee27542c21104bf35e390f8872611be Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Sun, 1 Mar 2020 17:07:10 +0100 Subject: [PATCH 2/6] Let `tar` change working directory to assets directory. This prevents edge cases where files are not found, while ensuring the archive does not contain the system directory hierarchy. --- controllers/Download.php | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/controllers/Download.php b/controllers/Download.php index b22b92b1..8dfaa9a9 100644 --- a/controllers/Download.php +++ b/controllers/Download.php @@ -29,22 +29,16 @@ class Download exit; } - private function exportAlbum($album) + private function exportAlbum(Tag $album) { $files = []; $album_ids = array_merge([$album->id_tag], $this->getChildAlbumIds($album->id_tag)); foreach($album_ids as $album_id) { - $iterator = AssetIterator::getByOptions( - [ - 'id_tag' => $album_id - ] - ); - while($asset = $iterator->Next()) - { - $files[] = join(DIRECTORY_SEPARATOR, ['.', $asset->getSubdir(), $asset->getFilename()]); - } + $iterator = AssetIterator::getByOptions(['id_tag' => $album_id]); + while ($asset = $iterator->next()) + $files[] = join(DIRECTORY_SEPARATOR, [$asset->getSubdir(), $asset->getFilename()]); } $descriptorspec = [ @@ -52,7 +46,7 @@ class Download 1 => ['pipe', 'w'], ]; - $command = 'tar --null -cf - -T -'; + $command = 'tar -cf - -C ' . escapeshellarg(ASSETSDIR) . ' --null -T -'; $proc = proc_open($command, $descriptorspec, $pipes, ASSETSDIR); From 2bb29d7224dba3b1b2cc514d3dfabc4d59e34e3e Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Sun, 1 Mar 2020 17:09:58 +0100 Subject: [PATCH 3/6] Minor optimisation to Download::getChildAlbumIds. --- controllers/Download.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/controllers/Download.php b/controllers/Download.php index 8dfaa9a9..58ec832f 100644 --- a/controllers/Download.php +++ b/controllers/Download.php @@ -84,11 +84,11 @@ class Download { $ids = []; - $albums = Tag::getAlbums($parent_id, 0, PHP_INT_MAX, 'object'); - foreach($albums as $album) + $albums = Tag::getAlbums($parent_id, 0, PHP_INT_MAX); + foreach ($albums as $album) { - $ids[] = $album->id_tag; - $ids = array_merge($ids, $this->getChildAlbumIds($album->id_tag)); + $ids[] = $album['id_tag']; + $ids = array_merge($ids, $this->getChildAlbumIds($album['id_tag'])); } return $ids; From 5a51778a6a0d75c5e529637175c80d4b1bf39bf3 Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Wed, 11 Mar 2020 12:13:04 +0100 Subject: [PATCH 4/6] Clean up tar process handling; make stdout non-blocking. --- controllers/Download.php | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/controllers/Download.php b/controllers/Download.php index 58ec832f..f9b81ef7 100644 --- a/controllers/Download.php +++ b/controllers/Download.php @@ -34,7 +34,7 @@ class Download $files = []; $album_ids = array_merge([$album->id_tag], $this->getChildAlbumIds($album->id_tag)); - foreach($album_ids as $album_id) + foreach ($album_ids as $album_id) { $iterator = AssetIterator::getByOptions(['id_tag' => $album_id]); while ($asset = $iterator->next()) @@ -42,8 +42,8 @@ class Download } $descriptorspec = [ - 0 => ['pipe', 'r'], - 1 => ['pipe', 'w'], + 0 => ['pipe', 'r'], // STDIN + 1 => ['pipe', 'w'], // STDOUT ]; $command = 'tar -cf - -C ' . escapeshellarg(ASSETSDIR) . ' --null -T -'; @@ -59,22 +59,33 @@ class Download if(!$pipes[1]) throw new UnexpectedValueException('Could not open pipe for STDOUT'); - $album_name = $album->tag; + // STDOUT should not block. + stream_set_blocking($pipes[1], 0); header('Pragma: no-cache'); header('Content-Description: File Download'); - header('Content-disposition: attachment; filename="' . $album_name . '.tar"'); + header('Content-disposition: attachment; filename="' . $album->tag . '.tar"'); header('Content-Type: application/octet-stream'); header('Content-Transfer-Encoding: binary'); - foreach($files as $file) + // Write filenames to include to STDIN, separated by null bytes. + foreach ($files as $file) fwrite($pipes[0], $file . "\0"); + // Close STDIN pipe to start archiving. fclose($pipes[0]); - while($chunk = stream_get_contents($pipes[1], 4096)) - echo $chunk; + do + { + // Read STDOUT as `tar` is doing its work. + echo stream_get_contents($pipes[1], 4096); + // Are we still running? + $status = proc_get_status($proc); + } + while (!empty($status) && $status['running']); + + // Close STDOUT pipe and clean up process. fclose($pipes[1]); proc_close($proc); From c7d3b9c3d15f39dc56ca1f4ab80dcd91bcfa58d3 Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Wed, 11 Mar 2020 12:31:31 +0100 Subject: [PATCH 5/6] Disable output buffering so we can enjoy more than ~62MB of photos. --- controllers/Download.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/controllers/Download.php b/controllers/Download.php index f9b81ef7..94e10a59 100644 --- a/controllers/Download.php +++ b/controllers/Download.php @@ -75,6 +75,9 @@ class Download // Close STDIN pipe to start archiving. fclose($pipes[0]); + // At this point, end output buffering so we can enjoy more than ~62MB of photos. + ob_end_flush(); + do { // Read STDOUT as `tar` is doing its work. From bd1ca8d18c003ea5aec15d67bc294978ccc08bf8 Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Wed, 11 Mar 2020 18:57:31 +0100 Subject: [PATCH 6/6] Use $_SESSION['current_export'] to prevent simultaneous exports. --- controllers/Download.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/controllers/Download.php b/controllers/Download.php index 94e10a59..b5de5496 100644 --- a/controllers/Download.php +++ b/controllers/Download.php @@ -22,7 +22,7 @@ class Download $album = Tag::fromId($tag); if (isset($_SESSION['current_export'])) - throw new UserFacingException('An export of "' . $tag->tag . '" is ongoing. Please try again later.'); + throw new UserFacingException('You can only export one album at the same time. Please wait until the other download finishes, or try again later.'); // So far so good? $this->exportAlbum($album); @@ -46,6 +46,15 @@ class Download 1 => ['pipe', 'w'], // STDOUT ]; + // Prevent simultaneous exports. + $_SESSION['current_export'] = $album->id_tag; + + // Allow new exports if the connection is terminated unexpectedly (e.g. when a user aborts a download). + register_shutdown_function(function() { + if (isset($_SESSION['current_export'])) + unset($_SESSION['current_export']); + }); + $command = 'tar -cf - -C ' . escapeshellarg(ASSETSDIR) . ' --null -T -'; $proc = proc_open($command, $descriptorspec, $pipes, ASSETSDIR); @@ -92,6 +101,9 @@ class Download fclose($pipes[1]); proc_close($proc); + + // Allow new exports from this point onward. + unset($_SESSION['current_export']); } private function getChildAlbumIds($parent_id)