Skip to content

Commit

Permalink
Fix GH-16998: UBSAN warning in rfc1867
Browse files Browse the repository at this point in the history
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.

Closes GH-17000.
  • Loading branch information
nielsdos committed Dec 1, 2024
1 parent 94fa2a4 commit aab7842
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 1 deletion.
3 changes: 3 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ PHP NEWS
. Fixed bug GH-15208 (Segfault with breakpoint map and phpdbg_clear()).
(nielsdos)

- SAPI:
. Fixed bug GH-16998 (UBSAN warning in rfc1867). (nielsdos)

- SimpleXML:
. Fixed bug GH-16808 (Segmentation fault in RecursiveIteratorIterator
->current() with a xml element input). (nielsdos)
Expand Down
2 changes: 1 addition & 1 deletion main/rfc1867.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
49 changes: 49 additions & 0 deletions tests/basic/gh16998.phpt
Original file line number Diff line number Diff line change
@@ -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) {
}

0 comments on commit aab7842

Please sign in to comment.