Skip to content
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

Fix GH-16326: Memory management is broken for bad dictionaries #16335

Closed
wants to merge 3 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Oct 9, 2024

We must not efree() zend_strings, since they may have a refcount greater than one, and may even be interned.

We also must not confuse zend_string * with zend_string **.

And we should play it safe by using safe_emalloc() to avoid theoretical integer overflows.

We must not `efree()` `zend_string`s, since they may have a refcount
greater than one, and may even be interned.

We also must not confuse `zend_string *` with `zend_string **`.

And we should play it safe by using `safe_emalloc()` to avoid
theoretical integer overflows.
@cmb69 cmb69 linked an issue Oct 9, 2024 that may be closed by this pull request
@cmb69 cmb69 changed the base branch from master to PHP-8.2 October 9, 2024 22:52
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Changes / Fix LGTM.

ext/zlib/zlib.c Outdated Show resolved Hide resolved
ext/zlib/zlib.c Outdated
Comment on lines 818 to 823
if (*ptr) {
efree(*ptr);
zend_string_release(*ptr);
}
while (--ptr >= strings) {
efree(ptr);
zend_string_release(*ptr);
}
Copy link
Member

Choose a reason for hiding this comment

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

I believe this can be adjusted to a do-while loop for consistency with the NUL check case below. Or perhaps this can be unified into a single place with a goto.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, since zval_get_string() always returns a zend_string, we can simplify.

Regarding goto see https://xkcd.com/292/ ;)

Copy link
Member

Choose a reason for hiding this comment

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

I prefer a well-placed goto over duplicating error handling all over the place. In fact it's already a little messy with the EG(exception) check reusing the first if() for the error handling only.

Copy link
Member Author

Choose a reason for hiding this comment

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

In my opinion, it's much better than it was, and I'm fine with. Maybe we get a third opinion. :)

@@ -807,7 +807,7 @@ static bool zlib_create_dictionary_string(HashTable *options, char **dict, size_
if (zend_hash_num_elements(dictionary) > 0) {
char *dictptr;
zval *cur;
zend_string **strings = emalloc(sizeof(zend_string *) * zend_hash_num_elements(dictionary));
zend_string **strings = safe_emalloc(zend_hash_num_elements(dictionary), sizeof(zend_string *), 0);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
zend_string **strings = safe_emalloc(zend_hash_num_elements(dictionary), sizeof(zend_string *), 0);
zend_string **strings = safe_emalloc(zend_hash_num_elements(dictionary), sizeof(*strings), 0);

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I prefer using sizeof with an expression (in which case I also prefer to omit the parentheses), but I think I've seen it most of the time using a type in php-src. Do we have a style guideline about that?

Copy link
Member

Choose a reason for hiding this comment

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

Do we have a style guideline about that?

I am not aware of anything.

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Except possibly for the two unresolved discussion threads, but no super strong opinion there.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

LGTM

zend_string **end, **ptr = strings - 1;

ZEND_HASH_FOREACH_VAL(dictionary, cur) {
size_t i;

*++ptr = zval_get_string(cur);
Copy link
Member

Choose a reason for hiding this comment

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

In the future we should use the try API here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't make it better, I'm afraid. Now we have to check for EG(exception) but can be sure that *ptr is not NULL; with zval_try_get_string() we no longer have to check for EG(exception), but need to check that *ptr is not NULL before releasing it. Basically, what we did.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but it is easier to miss checking for EG(exception) and use some sort of default zend_string then not having one in the first place IMHO.

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.

Memory management is broken for bad dictionaries
3 participants