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.

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

Conversation

nielsdos
Copy link
Member

Regressed in 6fbf81c.

There is a missing error check on spl_filesystem_file_read_line(), which means that if the line could not be read (e.g. because we're at the end of the file), it will not set intern->u.file.current_line, which will cause a NULL pointer deref later on.

Fix it by adding a check, and reintroducing the silent flag partially to be able to throw an exception like it did in the past.

Regressed in 6fbf81c.

There is a missing error check on spl_filesystem_file_read_line(), which
means that if the line could not be read (e.g. because we're at the end
of the file), it will not set intern->u.file.current_line, which will
cause a NULL pointer deref later on.

Fix it by adding a check, and reintroducing the silent flag partially to
be able to throw an exception like it did in the past.
@nielsdos nielsdos requested a review from Girgias as a code owner March 12, 2024 20:40
@nielsdos nielsdos linked an issue Mar 12, 2024 that may be closed by this pull request
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

@nielsdos nielsdos closed this in aa34e0a Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected null pointer in zend_string.h
2 participants