fix(syslogs): bound memory in LogTimestampParser::readLastLines (#1021)

Alexey Portnov · Alexey Portnov · commit b6ff441c3a82 · 2026-04-13T18:49:12.000+08:00
Sentry #27765: fatal OOM (`Allowed memory size of 268435456 bytes
exhausted`) in WorkerApiCommands when the syslog:getLogTimeRange API
encountered a log file with no newline characters. The backward chunk
reader accumulated 4 KiB per iteration into $chunk forever because
explode("\n", $chunk) returned a single-element array on every pass.

Rewrite readLastLines() to:
- Cap accumulated $chunk at READ_LAST_LINES_MAX_BUFFER (1 MiB) and bail
  out cleanly on binary / single-line files.
- Short-circuit files &lt;= 64 KiB through file(FILE_IGNORE_NEW_LINES),
  which is faster and avoids the seek state machine entirely.
- Track position via a local integer and SEEK_SET instead of
  fseek(-mb_strlen($chunk), SEEK_CUR), which was a pre-existing bug:
  on iterations after the first it sent the read pointer past the
  start of the previously-read region, causing partial-line overlap
  and skipped bytes between iterations.
- Virtually clip a single trailing '\n' so explode() does not yield
  a spurious empty element that would consume one of the requested
  line slots.
- Drop the never-firing `fseek(-1, SEEK_END); fread(1); fgets()`
  branch (after fread the position was at EOF, so fgets always
  returned false).

Add tests/PBXCoreREST/SysLogs/LogTimestampParserTest.php with 13
PHPUnit cases (reflection-based unit tests for the private method
plus end-to-end via getLogTimeRange):
- empty/missing file, zero lines requested
- small file with/without trailing newline, tail-limit
- large file (&gt;64 KiB) backward-seek path
- large file with 5000-byte lines crossing the 4 KiB internal buffer
  (regression for the seek arithmetic bug)
- 4 MiB file with no newlines must not exhaust memory: peak delta
  asserted &lt; 6 MiB after memory_reset_peak_usage(), measured ~2 MiB
- large file with multiple trailing blank lines preserves blanks
  matching file(FILE_IGNORE_NEW_LINES) semantics
- getLogTimeRange end-to-end on normal asterisk format and on
  garbage binary file (returns null start/end without crashing)
diff --git a/src/PBXCoreREST/Lib/SysLogs/LogTimestampParser.php b/src/PBXCoreREST/Lib/SysLogs/LogTimestampParser.php
@@ -207,6 +207,19 @@ private static function getLastTimestamp(string $filename): ?int
         return null;
     }
 
+    /**
+     * Maximum buffered chunk size for backward reading.
+     * Files without newlines (binary, single-line JSON, corrupted logs) would otherwise
+     * grow $chunk unbounded and OOM the worker. 1 MiB is far above any plausible log line.
+     */
+    private const READ_LAST_LINES_MAX_BUFFER = 1048576;
+
+    /**
+     * Files at or below this size are read whole instead of doing backward chunk seeks —
+     * simpler, faster, and avoids the entire failure mode of the seeking algorithm.
+     */
+    private const READ_LAST_LINES_SMALL_FILE_THRESHOLD = 65536;
+
     /**
      * Read last N lines from file efficiently
      *
@@ -216,43 +229,75 @@ private static function getLastTimestamp(string $filename): ?int
      */
     private static function readLastLines(string $filename, int $lines): array
     {
-        $handle = fopen($filename, 'r');
-        if ($handle === false) {
+        $size = @filesize($filename);
+        if ($size === false || $size === 0 || $lines <= 0) {
             return [];
         }
 
-        $buffer = 4096;
-        $output = [];
+        // Small file: just read it whole. file() handles trailing newlines correctly
+        // and avoids the backward-seek state machine entirely.
+        if ($size <= self::READ_LAST_LINES_SMALL_FILE_THRESHOLD) {
+            $allLines = @file($filename, FILE_IGNORE_NEW_LINES);
+            if ($allLines === false) {
+                return [];
+            }
+            return array_slice($allLines, -$lines);
+        }
 
-        fseek($handle, -1, SEEK_END);
+        $handle = @fopen($filename, 'rb');
+        if ($handle === false) {
+            return [];
+        }
 
-        if (fread($handle, 1) !== "\n") {
-            $line = fgets($handle);
-            if ($line !== false) {
-                $output[] = $line;
-            }
+        // If the file ends with '\n', virtually clip it so that explode() won't produce
+        // a spurious empty trailing element that would consume one of the requested slots.
+        // Matches file()/FILE_IGNORE_NEW_LINES semantics for normal log files.
+        fseek($handle, -1, SEEK_END);
+        if (fread($handle, 1) === "\n") {
+            $size--;
         }
 
+        $output = [];
         $chunk = '';
+        $pos = $size;
+        $bufferSize = 4096;
 
-        while (ftell($handle) > 0 && count($output) < $lines) {
-            $seek = min(ftell($handle), $buffer);
-            fseek($handle, -$seek, SEEK_CUR);
-            $chunk = fread($handle, $seek) . $chunk;
-            fseek($handle, -mb_strlen($chunk, '8bit'), SEEK_CUR);
+        while ($pos > 0 && count($output) < $lines) {
+            // Bail out on files without newlines (binary data, single huge JSON, etc.)
+            // before $chunk grows large enough to exhaust memory_limit. See issue #1021.
+            if (strlen($chunk) > self::READ_LAST_LINES_MAX_BUFFER) {
+                break;
+            }
 
+            $readSize = (int)min($pos, $bufferSize);
+            $pos -= $readSize;
+            if (fseek($handle, $pos, SEEK_SET) !== 0) {
+                break;
+            }
+            $data = fread($handle, $readSize);
+            if ($data === false) {
+                break;
+            }
+
+            $chunk = $data . $chunk;
             $linesArray = explode("\n", $chunk);
+            // First element may be a partial line whose start lives in the not-yet-read part
+            // of the file. Stash it for the next iteration; the rest are complete lines.
             $chunk = array_shift($linesArray);
-
             $output = array_merge($linesArray, $output);
         }
 
-        if (count($output) >= $lines) {
-            $output = array_slice($output, -$lines);
+        // Reached the start of the file: $chunk now holds the very first line.
+        if ($pos === 0 && $chunk !== '') {
+            array_unshift($output, $chunk);
         }
 
         fclose($handle);
 
+        if (count($output) > $lines) {
+            $output = array_slice($output, -$lines);
+        }
+
         return $output;
     }
 
diff --git a/tests/PBXCoreREST/SysLogs/LogTimestampParserTest.php b/tests/PBXCoreREST/SysLogs/LogTimestampParserTest.php
@@ -0,0 +1,232 @@
+<?php
+/*
+ * MikoPBX - free phone system for small business
+ * Copyright © 2017-2026 Alexey Portnov and Nikolay Beketov
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along with this program.
+ * If not, see <https://www.gnu.org/licenses/>.
+ */
+
+declare(strict_types=1);
+
+namespace MikoPBX\Tests\PBXCoreREST\SysLogs;
+
+use MikoPBX\PBXCoreREST\Lib\SysLogs\LogTimestampParser;
+use PHPUnit\Framework\TestCase;
+use ReflectionClass;
+
+/**
+ * Unit tests for LogTimestampParser, in particular the regression for issue #1021
+ * (OOM in readLastLines when the file contains no newline characters).
+ *
+ * Tests use reflection to call the private readLastLines() method directly so the
+ * algorithm can be exercised without standing up a full DI container.
+ */
+class LogTimestampParserTest extends TestCase
+{
+    private string $tmpDir;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        $this->tmpDir = sys_get_temp_dir() . '/log_ts_parser_' . bin2hex(random_bytes(4));
+        mkdir($this->tmpDir);
+    }
+
+    protected function tearDown(): void
+    {
+        foreach (glob($this->tmpDir . '/*') ?: [] as $file) {
+            @unlink($file);
+        }
+        @rmdir($this->tmpDir);
+        parent::tearDown();
+    }
+
+    /**
+     * Invoke the private readLastLines method via reflection.
+     *
+     * @return array<int, string>
+     */
+    private function callReadLastLines(string $filename, int $lines): array
+    {
+        $reflection = new ReflectionClass(LogTimestampParser::class);
+        $method = $reflection->getMethod('readLastLines');
+        $method->setAccessible(true);
+        return $method->invoke(null, $filename, $lines);
+    }
+
+    private function writeFile(string $name, string $content): string
+    {
+        $path = $this->tmpDir . '/' . $name;
+        file_put_contents($path, $content);
+        return $path;
+    }
+
+    public function testMissingFileReturnsEmpty(): void
+    {
+        self::assertSame([], $this->callReadLastLines($this->tmpDir . '/nope.log', 10));
+    }
+
+    public function testEmptyFileReturnsEmpty(): void
+    {
+        $path = $this->writeFile('empty.log', '');
+        self::assertSame([], $this->callReadLastLines($path, 10));
+    }
+
+    public function testZeroLinesRequestedReturnsEmpty(): void
+    {
+        $path = $this->writeFile('basic.log', "a\nb\nc\n");
+        self::assertSame([], $this->callReadLastLines($path, 0));
+    }
+
+    public function testSmallFileWithTrailingNewline(): void
+    {
+        $path = $this->writeFile('small.log', "line1\nline2\nline3\n");
+        self::assertSame(['line1', 'line2', 'line3'], $this->callReadLastLines($path, 10));
+    }
+
+    public function testSmallFileWithoutTrailingNewline(): void
+    {
+        $path = $this->writeFile('small.log', "line1\nline2\nline3");
+        self::assertSame(['line1', 'line2', 'line3'], $this->callReadLastLines($path, 10));
+    }
+
+    public function testSmallFileLimitsToRequestedTail(): void
+    {
+        $content = '';
+        for ($i = 1; $i <= 50; $i++) {
+            $content .= "line{$i}\n";
+        }
+        $path = $this->writeFile('tail.log', $content);
+
+        $result = $this->callReadLastLines($path, 5);
+        self::assertSame(['line46', 'line47', 'line48', 'line49', 'line50'], $result);
+    }
+
+    public function testLargeFileBackwardSeekPath(): void
+    {
+        // Build a file safely above the 64 KiB small-file threshold so the backward
+        // seeking path is exercised. 5000 short lines ≈ 50 KiB per 1k lines, so 5k lines ≈ 250 KiB.
+        $content = '';
+        for ($i = 1; $i <= 5000; $i++) {
+            $content .= sprintf("[2025-10-09 08:21:%02d] line%05d payload\n", $i % 60, $i);
+        }
+        $path = $this->writeFile('big.log', $content);
+        self::assertGreaterThan(65536, filesize($path));
+
+        $result = $this->callReadLastLines($path, 100);
+        self::assertCount(100, $result);
+        self::assertStringContainsString('line04901', $result[0]);
+        self::assertStringContainsString('line05000', $result[99]);
+    }
+
+    public function testLargeFileWithLongLinesCrossingBufferBoundary(): void
+    {
+        // Lines longer than the 4 KiB internal buffer to verify the partial-line
+        // accumulation across iterations works correctly after the seek fix.
+        $longPayload = str_repeat('x', 5000);
+        $content = '';
+        for ($i = 1; $i <= 50; $i++) {
+            $content .= "tag{$i}|{$longPayload}\n";
+        }
+        $path = $this->writeFile('long_lines.log', $content);
+        self::assertGreaterThan(65536, filesize($path));
+
+        $result = $this->callReadLastLines($path, 3);
+        self::assertCount(3, $result);
+        self::assertStringStartsWith('tag48|', $result[0]);
+        self::assertStringStartsWith('tag49|', $result[1]);
+        self::assertStringStartsWith('tag50|', $result[2]);
+        self::assertSame(strlen("tag50|") + 5000, strlen($result[2]));
+    }
+
+    /**
+     * Regression for issue #1021: a multi-megabyte file containing no '\n' must NOT
+     * exhaust memory_limit. Before the fix, $chunk grew by 4 KiB per iteration until
+     * it consumed the entire file. The guard caps it at READ_LAST_LINES_MAX_BUFFER (1 MiB).
+     */
+    public function testFileWithoutNewlinesDoesNotExhaustMemory(): void
+    {
+        // 4 MiB of 'A' with zero newlines — the worst case from the Sentry crash.
+        $payload = str_repeat('A', 4 * 1024 * 1024);
+        $path = $this->writeFile('binaryish.log', $payload);
+
+        // Reset the process-wide peak so prior fixtures in this run can't mask the leak.
+        // PHP 8.2+ — the project requires 8.3.
+        memory_reset_peak_usage();
+        $memBefore = memory_get_usage();
+        $result = $this->callReadLastLines($path, 100);
+        $memPeak = memory_get_peak_usage();
+
+        // Should bail cleanly without OOM, not crash. Result content is best-effort
+        // (algorithm cannot recover lines from a file with no separators).
+        self::assertIsArray($result);
+
+        // Critically: peak memory growth must stay under a few MiB, not climb to 4+ MiB
+        // (the file size). The 1 MiB chunk cap plus the 4 KiB read buffer plus PHP's
+        // per-string overhead leaves ~3 MiB of headroom; we assert 6 MiB to absorb
+        // explode()'s element-array allocations on small fixtures.
+        self::assertLessThan(
+            6 * 1024 * 1024,
+            $memPeak - $memBefore,
+            'readLastLines leaked memory while scanning a newline-free file'
+        );
+    }
+
+    /**
+     * file()/FILE_IGNORE_NEW_LINES preserves blank lines in the middle and (importantly)
+     * blank lines at the end. Our backward-seek implementation should match.
+     */
+    public function testLargeFileWithMultipleTrailingBlankLines(): void
+    {
+        // 18 KiB filler so the file crosses the 64 KiB threshold and the backward
+        // seek path is exercised.
+        $filler = str_repeat("filler line keeps file above the small-file threshold\n", 1500);
+        $content = $filler . "alpha\nbeta\n\n\n";
+        $path = $this->writeFile('trailing_blanks.log', $content);
+        self::assertGreaterThan(65536, filesize($path));
+
+        $result = $this->callReadLastLines($path, 5);
+
+        // The trailing single \n is the file terminator (not a content line).
+        // The two preceding \n produce two empty content lines, matching file().
+        self::assertSame(['filler line keeps file above the small-file threshold', 'alpha', 'beta', '', ''], $result);
+    }
+
+    public function testGetLogTimeRangeOnNormalFile(): void
+    {
+        $content = <<<LOG
+[2025-10-09 08:00:00] start of log
+[2025-10-09 08:15:32] some entry
+[2025-10-09 09:30:45] another entry
+[2025-10-09 10:00:00] end of log
+
+LOG;
+        $path = $this->writeFile('asterisk.log', $content);
+
+        $range = LogTimestampParser::getLogTimeRange($path);
+        self::assertSame(strtotime('2025-10-09 08:00:00'), $range['start']);
+        self::assertSame(strtotime('2025-10-09 10:00:00'), $range['end']);
+    }
+
+    public function testGetLogTimeRangeOnFileWithoutNewlinesReturnsNullsCleanly(): void
+    {
+        // 2 MiB of garbage without newlines — must not crash even though
+        // no timestamp can be extracted.
+        $path = $this->writeFile('garbage.log', str_repeat("\x01\x02\x03\x04", 512 * 1024));
+
+        $range = LogTimestampParser::getLogTimeRange($path);
+        self::assertNull($range['start']);
+        self::assertNull($range['end']);
+    }
+}
