From 61d02bc0f1f44fec3ffcb26ee6b7f3e19e314174 Mon Sep 17 00:00:00 2001
From: Niels Dossche <7771979+nielsdos@users.noreply.github.com>
Date: Sat, 30 Nov 2024 12:22:42 +0100
Subject: [PATCH] Fix GH-16998: UBSAN warning in rfc1867

The "else branch" of `next_line` can reset the `buf_begin` field to
NULL, causing the next invocation to pass NULL to `memchr` with a 0
length. When UBSAN is enabled this causes an UBSAN abort. Real world
impact is likely none because of the 0 length.

To fix this, don't set the pointer to NULL, which means that the
`memchr` will return NULL and since
`self->bytes_in_buffer < self->bufsize` we return NULL and request more
data through `fill_buffer`. That function will reset `buf_begin` and
`bytes_in_buffer` so that the next invocation works fine.

I chose this solution so we have an invariant that `buf_begin` is never
NULL, which makes reasoning easier. An alternative solution is keeping
the NULLing of `buf_begin` and add an extra check at the top of
`next_line`, but I didn't like special casing this.
---
 main/rfc1867.c           |  2 +-
 tests/basic/gh16998.phpt | 49 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100644 tests/basic/gh16998.phpt

diff --git a/main/rfc1867.c b/main/rfc1867.c
index 12794c414b342..fbfd6e78f9994 100644
--- a/main/rfc1867.c
+++ b/main/rfc1867.c
@@ -341,8 +341,8 @@ static char *next_line(multipart_buffer *self)
 		}
 		/* return entire buffer as a partial line */
 		line[self->bufsize] = 0;
-		self->buf_begin = ptr;
 		self->bytes_in_buffer = 0;
+		/* Let fill_buffer() handle the reset of self->buf_begin */
 	}
 
 	return line;
diff --git a/tests/basic/gh16998.phpt b/tests/basic/gh16998.phpt
new file mode 100644
index 0000000000000..8bfcbbda99dd0
--- /dev/null
+++ b/tests/basic/gh16998.phpt
@@ -0,0 +1,49 @@
+--TEST--
+GH-16998 (UBSAN warning in rfc1867)
+--SKIPIF--
+<?php
+if (!getenv('TEST_PHP_CGI_EXECUTABLE')) {
+    die("skip php-cgi not available");
+}
+?>
+--FILE--
+<?php
+const FILLUNIT = 5 * 1024;
+$cmd = [
+    getenv('TEST_PHP_CGI_EXECUTABLE'),
+    '-C',
+    '-n',
+    __DIR__ . '/GHSA-9pqp-7h25-4f32.inc',
+];
+$boundary = str_repeat('A', FILLUNIT);
+$body = ""
+    . "--$boundary\r\n"
+    . "Content-Disposition: form-data; name=\"koko\"\r\n"
+    . "\r\n"
+    . "BBB\r\n--" . substr($boundary, 0, -1) . "CCC\r\n"
+    . "--$boundary--\r\n"
+    ;
+$env = array_merge($_ENV, [
+    'REDIRECT_STATUS' => '1',
+    'CONTENT_TYPE' => "multipart/form-data; boundary=",
+    'CONTENT_LENGTH' => strlen($body),
+    'REQUEST_METHOD' => 'POST',
+    'SCRIPT_FILENAME' => __DIR__ . '/GHSA-9pqp-7h25-4f32.inc',
+]);
+$spec = [
+    0 => ['pipe', 'r'],
+    1 => STDOUT,
+    2 => STDOUT,
+];
+$pipes = [];
+$handle = proc_open($cmd, $spec, $pipes, getcwd(), $env);
+fwrite($pipes[0], $body);
+proc_close($handle);
+?>
+--EXPECTF--
+X-Powered-By: PHP/%s
+Content-type: text/html; charset=UTF-8
+
+Hello world
+array(0) {
+}