Skip to content

Fix GH-13685: Unexpected null pointer in zend_string.h #13692

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix GH-13685: Unexpected null pointer in zend_string.h #13692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 28 additions & 18 deletions ext/spl/spl_directory.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1866,6 +1866,11 @@ zend_object_iterator *spl_filesystem_tree_get_iterator(zend_class_entry *ce, zva
}
/* }}} */

static ZEND_COLD void spl_filesystem_file_cannot_read(spl_filesystem_object *intern)
{
zend_throw_exception_ex(spl_ce_RuntimeException, 0, "Cannot read from file %s", ZSTR_VAL(intern->file_name));
}

static zend_result spl_filesystem_file_read_ex(spl_filesystem_object *intern, bool silent, zend_long line_add, bool csv)
{
char *buf;
Expand All @@ -1875,7 +1880,7 @@ static zend_result spl_filesystem_file_read_ex(spl_filesystem_object *intern, bo

if (php_stream_eof(intern->u.file.stream)) {
if (!silent) {
zend_throw_exception_ex(spl_ce_RuntimeException, 0, "Cannot read from file %s", ZSTR_VAL(intern->file_name));
spl_filesystem_file_cannot_read(intern);
}
return FAILURE;
}
Expand Down Expand Up @@ -1930,10 +1935,10 @@ static bool is_line_empty(spl_filesystem_object *intern)
|| (current_line_len == 2 && current_line[0] == '\r' && current_line[1] == '\n'))));
}

static zend_result spl_filesystem_file_read_csv(spl_filesystem_object *intern, char delimiter, char enclosure, int escape, zval *return_value) /* {{{ */
static zend_result spl_filesystem_file_read_csv(spl_filesystem_object *intern, char delimiter, char enclosure, int escape, zval *return_value, bool silent) /* {{{ */
{
do {
zend_result ret = spl_filesystem_file_read(intern, /* silent */ true, /* csv */ true);
zend_result ret = spl_filesystem_file_read(intern, silent, /* csv */ true);
if (ret != SUCCESS) {
return ret;
}
Expand All @@ -1959,19 +1964,21 @@ static zend_result spl_filesystem_file_read_csv(spl_filesystem_object *intern, c
}
/* }}} */

/* Call to this function reads a line in a "silent" fashion and does not throw an exception */
static zend_result spl_filesystem_file_read_line_ex(zval * this_ptr, spl_filesystem_object *intern) /* {{{ */
static zend_result spl_filesystem_file_read_line_ex(zval * this_ptr, spl_filesystem_object *intern, bool silent) /* {{{ */
{
zval retval;

/* 1) use fgetcsv? 2) overloaded call the function, 3) do it directly */
if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_CSV)) {
return spl_filesystem_file_read_csv(intern, intern->u.file.delimiter, intern->u.file.enclosure, intern->u.file.escape, NULL);
return spl_filesystem_file_read_csv(intern, intern->u.file.delimiter, intern->u.file.enclosure, intern->u.file.escape, NULL, silent);
}
if (intern->u.file.func_getCurr->common.scope != spl_ce_SplFileObject) {
spl_filesystem_file_free_line(intern);

if (php_stream_eof(intern->u.file.stream)) {
if (!silent) {
spl_filesystem_file_cannot_read(intern);
}
return FAILURE;
}
zend_call_method_with_0_params(Z_OBJ_P(this_ptr), Z_OBJCE_P(this_ptr), &intern->u.file.func_getCurr, "getCurrentLine", &retval);
Expand All @@ -1995,18 +2002,17 @@ static zend_result spl_filesystem_file_read_line_ex(zval * this_ptr, spl_filesys
zval_ptr_dtor(&retval);
return SUCCESS;
} else {
return spl_filesystem_file_read(intern, /* silent */ true, /* csv */ false);
return spl_filesystem_file_read(intern, silent, /* csv */ false);
}
} /* }}} */

/* Call to this function reads a line in a "silent" fashion and does not throw an exception */
static zend_result spl_filesystem_file_read_line(zval * this_ptr, spl_filesystem_object *intern) /* {{{ */
static zend_result spl_filesystem_file_read_line(zval * this_ptr, spl_filesystem_object *intern, bool silent) /* {{{ */
{
zend_result ret = spl_filesystem_file_read_line_ex(this_ptr, intern);
zend_result ret = spl_filesystem_file_read_line_ex(this_ptr, intern, silent);

while (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_SKIP_EMPTY) && ret == SUCCESS && is_line_empty(intern)) {
spl_filesystem_file_free_line(intern);
ret = spl_filesystem_file_read_line_ex(this_ptr, intern);
ret = spl_filesystem_file_read_line_ex(this_ptr, intern, silent);
}

return ret;
Expand All @@ -2028,7 +2034,7 @@ static void spl_filesystem_file_rewind(zval * this_ptr, spl_filesystem_object *i
intern->u.file.current_line_num = 0;

if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_AHEAD)) {
spl_filesystem_file_read_line(this_ptr, intern);
spl_filesystem_file_read_line(this_ptr, intern, true);
}
} /* }}} */

Expand Down Expand Up @@ -2182,7 +2188,7 @@ PHP_METHOD(SplFileObject, current)
CHECK_SPL_FILE_OBJECT_IS_INITIALIZED(intern);

if (!intern->u.file.current_line && Z_ISUNDEF(intern->u.file.current_zval)) {
spl_filesystem_file_read_line(ZEND_THIS, intern);
spl_filesystem_file_read_line(ZEND_THIS, intern, true);
}
if (intern->u.file.current_line && (!SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_CSV) || Z_ISUNDEF(intern->u.file.current_zval))) {
RETURN_STRINGL(intern->u.file.current_line, intern->u.file.current_line_len);
Expand Down Expand Up @@ -2221,7 +2227,7 @@ PHP_METHOD(SplFileObject, next)

spl_filesystem_file_free_line(intern);
if (SPL_HAS_FLAG(intern->flags, SPL_FILE_OBJECT_READ_AHEAD)) {
spl_filesystem_file_read_line(ZEND_THIS, intern);
spl_filesystem_file_read_line(ZEND_THIS, intern, true);
}
intern->u.file.current_line_num++;
} /* }}} */
Expand Down Expand Up @@ -2339,7 +2345,7 @@ PHP_METHOD(SplFileObject, fgetcsv)
}
}

if (spl_filesystem_file_read_csv(intern, delimiter, enclosure, escape, return_value) == FAILURE) {
if (spl_filesystem_file_read_csv(intern, delimiter, enclosure, escape, return_value, true) == FAILURE) {
RETURN_FALSE;
}
}
Expand Down Expand Up @@ -2720,7 +2726,7 @@ PHP_METHOD(SplFileObject, seek)
spl_filesystem_file_rewind(ZEND_THIS, intern);

for (i = 0; i < line_pos; i++) {
if (spl_filesystem_file_read_line(ZEND_THIS, intern) == FAILURE) {
if (spl_filesystem_file_read_line(ZEND_THIS, intern, true) == FAILURE) {
return;
}
}
Expand All @@ -2740,8 +2746,12 @@ PHP_METHOD(SplFileObject, __toString)

CHECK_SPL_FILE_OBJECT_IS_INITIALIZED(intern);

if (!intern->u.file.current_line && Z_ISUNDEF(intern->u.file.current_zval)) {
spl_filesystem_file_read_line(ZEND_THIS, intern);
if (!intern->u.file.current_line) {
ZEND_ASSERT(Z_ISUNDEF(intern->u.file.current_zval));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that confused me is the original check: !intern->u.file.current_line && Z_ISUNDEF(intern->u.file.current_zval). I think that if one of these conditions in the && is false, the other one is as well.
If that is not the case then it will still be buggy (in other places too) because there could be a potential NULL deref if it is possible that Z_ISUNDEF(intern->u.file.current_zval) is false while !intern->u.file.current_line is true.
That's why I changed it to an assert.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it possible if one does some rewind or something like that? But yeah that does seem lightly weird otherwise

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm I don't think so, rewind calls spl_filesystem_file_free_line which (confusingly) also clears out current_zval...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah than this should be fine

zend_result result = spl_filesystem_file_read_line(ZEND_THIS, intern, false);
if (UNEXPECTED(result != SUCCESS)) {
RETURN_THROWS();
}
}

RETURN_STRINGL(intern->u.file.current_line, intern->u.file.current_line_len);
Expand Down
52 changes: 52 additions & 0 deletions ext/spl/tests/gh13685.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
--TEST--
GH-13685 (Unexpected null pointer in zend_string.h)
--FILE--
<?php

$contents = <<<EOS
"A", "B", "C"
"D", "E", "F"
EOS;

echo "--- Directly call fgetcsv ---\n";

$file = new SplTempFileObject;
$file->fwrite($contents);
$file->rewind();
while (($data = $file->fgetcsv(',', '"', ''))) {
var_dump((string) $file);
}
try {
var_dump((string) $file);
} catch (Exception $e) {
echo $e->getMessage(), "\n";
}

echo "--- Use csv control ---\n";

$file = new SplTempFileObject;
$file->fwrite($contents);
$file->rewind();
$file->setFlags(SplFileObject::READ_CSV);
$file->setCsvControl(',', '"', '');
foreach ($file as $row) {
var_dump((string) $file);
}
try {
var_dump((string) $file);
} catch (Exception $e) {
echo $e->getMessage(), "\n";
}

?>
--EXPECT--
--- Directly call fgetcsv ---
string(14) ""A", "B", "C"
"
string(13) ""D", "E", "F""
Cannot read from file php://temp
--- Use csv control ---
string(14) ""A", "B", "C"
"
string(13) ""D", "E", "F""
Cannot read from file php://temp