From ac2b4ffc8c62aa95ad2eccc1d38832e132cb8849 Mon Sep 17 00:00:00 2001 From: Damyon Wiese Date: Fri, 24 Jan 2014 13:52:53 +0800 Subject: [PATCH] MDL-43593 Assign editpdf: More robust handling of errors from TCPDF. This includes a check to see if there are 0 pages in the combined pdf, catching exceptions and suppressing php warnings and errors from bad pdf files. Also - Use TCPDF directly to check if pdfs are compatible. The previous check was letting dodgy PDFs through which then failed at generation time. This way dodgy pdfs will get run through ghostscript early and cleaned up. --- lib/pdflib.php | 3 ++ .../editpdf/classes/document_services.php | 23 +++++++- mod/assign/feedback/editpdf/classes/pdf.php | 53 ++++++++++++------- 3 files changed, 58 insertions(+), 21 deletions(-) diff --git a/lib/pdflib.php b/lib/pdflib.php index 3b72e6e9e77..309698e3a76 100644 --- a/lib/pdflib.php +++ b/lib/pdflib.php @@ -81,6 +81,9 @@ define('K_CELL_HEIGHT_RATIO', 1.25); /** reduction factor for small font */ define('K_SMALL_RATIO', 2/3); +/** Throw exceptions from errors so they can be caught and recovered from. */ +define('K_TCPDF_THROW_EXCEPTION_ERROR', true); + require_once(dirname(__FILE__).'/tcpdf/tcpdf.php'); /** diff --git a/mod/assign/feedback/editpdf/classes/document_services.php b/mod/assign/feedback/editpdf/classes/document_services.php index 3117df09663..468b1032921 100644 --- a/mod/assign/feedback/editpdf/classes/document_services.php +++ b/mod/assign/feedback/editpdf/classes/document_services.php @@ -207,9 +207,17 @@ class document_services { $tmpfile = $tmpdir . '/' . self::COMBINED_PDF_FILENAME; @unlink($tmpfile); - $pagecount = $pdf->combine_pdfs($compatiblepdfs, $tmpfile); + try { + $pagecount = $pdf->combine_pdfs($compatiblepdfs, $tmpfile); + } catch (\Exception $e) { + debugging('TCPDF could not process the pdf files:' . $e->getMessage(), DEBUG_DEVELOPER); + // TCPDF does not recover from errors so we need to re-initialise the class. + $pagecount = 0; + } if ($pagecount == 0) { // We at least want a single blank page. + debugging('TCPDF did not produce a valid pdf:' . $tmpfile . '. Replacing with a blank pdf.', DEBUG_DEVELOPER); + $pdf = new pdf(); $pdf->AddPage(); @unlink($tmpfile); $files = false; @@ -229,14 +237,27 @@ class document_services { $fs->delete_area_files($record->contextid, $record->component, $record->filearea, $record->itemid); + // Detect corrupt generated pdfs and replace with a blank one. + if ($files) { + $pagecount = $pdf->load_pdf($tmpfile); + if ($pagecount <= 0) { + $files = false; + } + } + if (!$files) { // This was a blank pdf. + unset($pdf); + $pdf = new pdf(); $content = $pdf->Output(self::COMBINED_PDF_FILENAME, 'S'); $file = $fs->create_file_from_string($record, $content); } else { // This was a combined pdf. $file = $fs->create_file_from_pathname($record, $tmpfile); @unlink($tmpfile); + + // Test the generated file for correctness. + $compatiblepdf = pdf::ensure_pdf_compatible($file); } return $file; diff --git a/mod/assign/feedback/editpdf/classes/pdf.php b/mod/assign/feedback/editpdf/classes/pdf.php index d11f0778d73..c3c5f0079cf 100644 --- a/mod/assign/feedback/editpdf/classes/pdf.php +++ b/mod/assign/feedback/editpdf/classes/pdf.php @@ -78,6 +78,7 @@ class pdf extends \FPDI { public function combine_pdfs($pdflist, $outfilename) { raise_memory_limit(MEMORY_EXTRA); + $olddebug = error_reporting(0); $this->setPageUnit('pt'); $this->setPrintHeader(false); @@ -97,6 +98,7 @@ class pdf extends \FPDI { } $this->save_pdf($outfilename); + error_reporting($olddebug); return $totalpagecount; } @@ -125,6 +127,7 @@ class pdf extends \FPDI { */ public function load_pdf($filename) { raise_memory_limit(MEMORY_EXTRA); + $olddebug = error_reporting(0); $this->setPageUnit('pt'); $this->scale = 72.0 / 100.0; @@ -138,6 +141,7 @@ class pdf extends \FPDI { $this->pagecount = $this->setSourceFile($filename); $this->filename = $filename; + error_reporting($olddebug); return $this->pagecount; } @@ -389,7 +393,9 @@ class pdf extends \FPDI { * @param string $filename the filename for the PDF (including the full path) */ public function save_pdf($filename) { + $olddebug = error_reporting(0); $this->Output($filename, 'F'); + error_reporting($olddebug); } /** @@ -461,33 +467,26 @@ class pdf extends \FPDI { * @return string path to copy or converted pdf (false == fail) */ public static function ensure_pdf_compatible(\stored_file $file) { - global $CFG; - - $fp = $file->get_content_file_handle(); - $ident = fread($fp, 10); - if (substr_compare('%PDF-', $ident, 0, 5) !== 0) { - return false; // This is not a PDF file at all. - } - $ident = substr($ident, 5); // Remove the '%PDF-' part. - $ident = explode('\x0A', $ident); // Truncate to first '0a' character. - list($major, $minor) = explode('.', $ident[0]); // Split the major / minor version. - $major = intval($major); - $minor = intval($minor); - if ($major == 0 || $minor == 0) { - return false; // Not a valid PDF version number. - } $temparea = \make_temp_directory('assignfeedback_editpdf'); $hash = $file->get_contenthash(); // Use the contenthash to make sure the temp files have unique names. $tempsrc = $temparea . "/src-$hash.pdf"; $tempdst = $temparea . "/dst-$hash.pdf"; + $file->copy_content_to($tempsrc); // Copy the file. - if ($major = 1 && $minor<=4) { - // PDF is valid version - just create a copy we can use. - $file->copy_content_to($tempdst); // Copy the file. - return $tempdst; + $pdf = new pdf(); + $pagecount = 0; + try { + $pagecount = $pdf->load_pdf($tempsrc); + } catch (\Exception $e) { + // PDF was not valid - try running it through ghostscript to clean it up. + $pagecount = 0; + } + + if ($pagecount > 0) { + // Page is valid and can be read by tcpdf. + return $tempsrc; } - $file->copy_content_to($tempsrc); // Copy the file. $gsexec = \escapeshellarg(\get_config('assignfeedback_editpdf', 'gspath')); $tempdstarg = \escapeshellarg($tempdst); @@ -500,6 +499,20 @@ class pdf extends \FPDI { return false; } + $pdf = new pdf(); + $pagecount = 0; + try { + $pagecount = $pdf->load_pdf($tempdst); + } catch (\Exception $e) { + // PDF was not valid - try running it through ghostscript to clean it up. + $pagecount = 0; + } + if ($pagecount <= 0) { + @unlink($tempdst); + // Could not parse the converted pdf. + return false; + } + return $tempdst; }