From a442938279290382c9e873b9d01d2da51378eaa0 Mon Sep 17 00:00:00 2001 From: Matt Rice <3780536+mattrice@users.noreply.github.com> Date: Fri, 18 Feb 2022 15:30:29 -0500 Subject: [PATCH 1/2] MDL-72582 core_files: Update readfile_accel error handling Correct the error handling in readfile_accel to properly log errors when working on a stored_file, rather than hiding the true error behind the incorrect "Object of class stored_file could not be converted to string." Co-authored-by: Simey Lameze Co-authored-by: Trisha Milan --- lib/filelib.php | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/lib/filelib.php b/lib/filelib.php index a5d62cc0aeb..edd075ed3b3 100644 --- a/lib/filelib.php +++ b/lib/filelib.php @@ -2226,10 +2226,11 @@ function readfile_accel($file, $mimetype, $accelerate) { header('Content-Type: '.$mimetype); } - $lastmodified = is_object($file) ? $file->get_timemodified() : filemtime($file); + $isfileobj = is_object($file); + $lastmodified = $isfileobj ? $file->get_timemodified() : filemtime($file); header('Last-Modified: '. gmdate('D, d M Y H:i:s', $lastmodified) .' GMT'); - if (is_object($file)) { + if ($isfileobj) { header('Etag: "' . $file->get_contenthash() . '"'); if (isset($_SERVER['HTTP_IF_NONE_MATCH']) and trim($_SERVER['HTTP_IF_NONE_MATCH'], '"') === $file->get_contenthash()) { header('HTTP/1.1 304 Not Modified'); @@ -2238,7 +2239,7 @@ function readfile_accel($file, $mimetype, $accelerate) { } // if etag present for stored file rely on it exclusively - if (!empty($_SERVER['HTTP_IF_MODIFIED_SINCE']) and (empty($_SERVER['HTTP_IF_NONE_MATCH']) or !is_object($file))) { + if (!empty($_SERVER['HTTP_IF_MODIFIED_SINCE']) && (empty($_SERVER['HTTP_IF_NONE_MATCH']) || !$isfileobj)) { // get unixtime of request header; clip extra junk off first $since = strtotime(preg_replace('/;.*$/', '', $_SERVER["HTTP_IF_MODIFIED_SINCE"])); if ($since && $since >= $lastmodified) { @@ -2254,7 +2255,7 @@ function readfile_accel($file, $mimetype, $accelerate) { } if ($accelerate) { - if (is_object($file)) { + if ($isfileobj) { $fs = get_file_storage(); if ($fs->supports_xsendfile()) { if ($fs->xsendfile_file($file)) { @@ -2271,7 +2272,8 @@ function readfile_accel($file, $mimetype, $accelerate) { } } - $filesize = is_object($file) ? $file->get_filesize() : filesize($file); + $filesize = $isfileobj ? $file->get_filesize() : filesize($file); + $filename = $isfileobj ? $file->get_filename() : $file; header('Last-Modified: '. gmdate('D, d M Y H:i:s', $lastmodified) .' GMT'); @@ -2305,10 +2307,10 @@ function readfile_accel($file, $mimetype, $accelerate) { $ranges = false; } if ($ranges) { - if (is_object($file)) { + if ($isfileobj) { $handle = $file->get_content_file_handle(); if ($handle === false) { - throw new file_exception('storedfilecannotreadfile', $file->get_filename()); + throw new file_exception('storedfilecannotreadfile', $filename); } } else { $handle = fopen($file, 'rb'); @@ -2334,7 +2336,12 @@ function readfile_accel($file, $mimetype, $accelerate) { // We do not expect any content in the buffer when we are serving files. $buffercontents = ob_get_clean(); if ($buffercontents !== '') { - error_log('Non-empty default output handler buffer detected while serving the file ' . $file); + // Include a preview of the first 20 characters of the output buffer to help identify + // what's causing it to be non-empty. This is useful for diagnosing unexpected output + // without exposing full content. + $buffercontentspreview = substr($buffercontents, 0, 20); + debugging("Non-empty default output handler buffer detected while serving the file {$filename}. " . + "Buffer contents (first 20 characters): {$buffercontentspreview}", DEBUG_DEVELOPER); } } else { // Some handlers such as zlib output compression may have file signature buffered - flush it. @@ -2343,7 +2350,7 @@ function readfile_accel($file, $mimetype, $accelerate) { } // send the whole file content - if (is_object($file)) { + if ($isfileobj) { $file->readfile(); } else { if (readfile_allow_large($file, $filesize) === false) { From 4e9c6b3f376fb135db980f5d5dac5ec08c266853 Mon Sep 17 00:00:00 2001 From: Trisha Milan Date: Thu, 17 Apr 2025 11:32:02 +1000 Subject: [PATCH 2/2] MDL-72582 core_files: Add unit test for readfile_accel debugging output Adds a unit test for the debugging message triggered when readfile_accel() detects a non-empty output buffer. The readfile_accel() function directly manipulates the output buffers, so calling it from within PHPUnit triggers an error like "Test code or tested code closed output buffers other than its own". As a workaround, the test runs a CLI script in a separate process, allowing buffer behaviour to be isolated and debugging output to be captured for assertions. --- lib/tests/filelib_test.php | 50 ++++++++ .../fixtures/readfile_accel_debug_cli.php | 112 ++++++++++++++++++ 2 files changed, 162 insertions(+) create mode 100644 lib/tests/fixtures/readfile_accel_debug_cli.php diff --git a/lib/tests/filelib_test.php b/lib/tests/filelib_test.php index 4bed342c6d6..f0414ad7473 100644 --- a/lib/tests/filelib_test.php +++ b/lib/tests/filelib_test.php @@ -2076,6 +2076,56 @@ EOF; ], ]; } + + /** + * Tests that readfile_accel() triggers the expected debugging message when a non-empty + * output buffer is detected, using both a file path and a stored_file input. + * + * This test runs a CLI script in a separate process to isolate buffer manipulation. + * This is necessary because readfile_accel() uses ob_get_clean() and ob_end_flush(), + * which interfere with PHPUnit's internal output buffer enforcement and cause risky + * test errors. + * + * The CLI script simulates a non-empty output buffer, calls the readfile_accel(), and + * prints any debugging output. The test then captures that output and asserts that the + * correct debugging message was generated. + * + * @covers ::readfile_accel + */ + public function test_readfile_accel_with_path_and_stored_file(): void { + $this->resetAfterTest(); + + // Construct the command to run the CLI script with a custom constant defined. + $scriptpath = __DIR__ . '/fixtures/readfile_accel_debug_cli.php'; + $cmd = 'php -r ' . escapeshellarg("define('PHPUNIT_READFILE_ACCEL_TEST', true); require '$scriptpath';"); + + $pipes = []; + $process = proc_open($cmd, [ + 1 => ['pipe', 'w'], + 2 => ['pipe', 'w'], + ], $pipes); + + $stdout = stream_get_contents($pipes[1]); + $stderr = stream_get_contents($pipes[2]); + + fclose($pipes[1]); + fclose($pipes[2]); + + $exitcode = proc_close($process); + + $output = $stdout . $stderr; + + // Debug just in case the subprocess fails. + $this->assertSame(0, $exitcode); + + // Validate that both path-based and stored_file debugging messages are present. + $filename = "readfile_accel.txt"; + $filepath = '/tmp/' . $filename; + $this->assertStringContainsString('Non-empty default output handler buffer detected while serving the file ' . + $filepath . '. Buffer contents (first 20 characters): test text', $output); + $this->assertStringContainsString('Non-empty default output handler buffer detected while serving the file ' . + $filename . '. Buffer contents (first 20 characters): test text', $output); + } } /** diff --git a/lib/tests/fixtures/readfile_accel_debug_cli.php b/lib/tests/fixtures/readfile_accel_debug_cli.php new file mode 100644 index 00000000000..4731bf3fa96 --- /dev/null +++ b/lib/tests/fixtures/readfile_accel_debug_cli.php @@ -0,0 +1,112 @@ +. + +/** + * Cli script for testing readfile_accel function. + * + * @package core + * @subpackage fixtures + * @copyright 2025 Catalyst IT + * @author Trisha Milan + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +define('CLI_SCRIPT', true); + +require_once(__DIR__ . '/../../../config.php'); +require_once($CFG->libdir . '/filelib.php'); + +if (!defined('PHPUNIT_READFILE_ACCEL_TEST')) { + echo 'This script is only intended to be run via PHPUnit.'; + exit(1); +} + +$testdb = moodle_database::get_driver_instance($CFG->dbtype, $CFG->dblibrary); +$testdb->connect($CFG->dbhost, $CFG->dbuser, $CFG->dbpass, $CFG->dbname, $CFG->phpunit_prefix); +$DB = $testdb; + +set_debugging(DEBUG_DEVELOPER, true); +$CFG->tempdir = '/tmp'; + +/** + * Runs readfile_accel() with a file path or a stored_file to trigger the buffer check. + * + * @param string|stored_file $input + * @param string $mimetype + * @param bool $accelerate + */ +function run_readfile_accel_test(string|stored_file $input, string $mimetype, bool $accelerate): void { + try { + ob_start(); + echo "test text"; + $_SERVER['REQUEST_METHOD'] = 'GET'; + readfile_accel($input, $mimetype, $accelerate); + } catch (Exception $e) { + echo $e->getMessage() . "\n"; + } +} + +try { + // Prepare test file. + $filename = "readfile_accel.txt"; + // Generate temporary local file for testing. + $path = "$CFG->tempdir/$filename"; + file_put_contents($path, "\nMoodle test data\n"); + + // Populate {files} table. + $fs = get_file_storage(); + $filerecord = [ + 'contextid' => context_system::instance()->id, + 'component' => 'test', + 'filearea' => 'readfile', + 'itemid' => 0, + 'filepath' => '/', + 'filename' => $filename, + ]; + $storedfile = null; + $filerecord['filename'] = $fs->get_unused_filename( + $filerecord['contextid'], + $filerecord['component'], + $filerecord['filearea'], + $filerecord['itemid'], + $filerecord['filepath'], + $filerecord['filename'] + ); + $storedfile = $fs->create_file_from_pathname($filerecord, $path); + $mimetype = get_mimetype_for_sending($storedfile->get_filename()); + $accelerate = true; + + // Run the test with direct path. + run_readfile_accel_test($path, $mimetype, $accelerate); + + // Run the test with direct stored_file. + run_readfile_accel_test($storedfile, $mimetype, $accelerate); +} finally { + // Clean up {files} table. + if (!is_null($fs)) { + @$fs->delete_area_files( + $filerecord['contextid'], + $filerecord['component'], + $filerecord['filearea'], + $filerecord['itemid'] + ); + } + + // Clean up testing file. + if ($path !== "") { + @unlink($path); + } +}