Skip to content

Conversation

@nielsdos
Copy link
Member

@nielsdos nielsdos commented Jan 1, 2023

As suggested by cmb69, this is fixed by changing zend_gcvt() and verifying whether the callers have enough buffer space.
The only place that did not allocate enough buffer space was in php_encoding.c. However, it actually uses safe_emalloc with an offset of MAX_LENGTH_OF_DOUBLE + 1 so it wouldn't have overflowed in practice I guess, but I think it's still better to not rely on the overflow protection and do it properly.

As suggested by cmb69, this is fixed by changing zend_gcvt() and
verifying whether the callers have enough buffer space.
@cmb69
Copy link
Member

cmb69 commented Jan 2, 2023

The only place that did not allocate enough buffer space was in php_encoding.c.

Unfortunately, we can't be sure about that, because zend_strtod.h is public, and zend_gcvt() is exported (and as such can be used by external extensions and SAPIs). Therefore we cannot apply this patch to any stable version. It might be okay to apply to "master", document the change in UPGRADING, and generally ZEND_ASSERT() that the buffer is at least 5 bytes long.

@nielsdos
Copy link
Member Author

nielsdos commented Jan 2, 2023

Ah right... that's unfortunate.

and generally ZEND_ASSERT() that the buffer is at least 5 bytes long.

You mean at the function entry right?

@nielsdos
Copy link
Member Author

nielsdos commented Jan 2, 2023

Wait how would we even assert for enough buffer space? We can't really use ndigit because it is actually used for precision and having it less than 5 is legal afaik?

@cmb69
Copy link
Member

cmb69 commented Jan 2, 2023

Wait how would we even assert for enough buffer space? We can't really use ndigit because it is actually used for precision and having it less than 5 is legal afaik?

Oh, right! This function is a foot-gun, what's probably the reason it has been removed from the POSIX specs. It may make sense to introduce a new function which also requires the buffer size to be passed (like _gcvt_s).

nielsdos added a commit to nielsdos/php-src that referenced this pull request Jan 12, 2023
…sses if the appropriate methods are present

This introduces the class flag ZEND_ACC_SUBCLASS_SERIALIZABLE, which
allows classes which are not serializable / deserializable to become
that if a subclass implements the appropriate methods.
Setting this flag through the stubs can be done using
@subclass-serializable.
Introducing this through behaviour a new flag allows for opt-in
behaviour, which is backwards compatible.

Fixes phpGH-10201
@iluuu1994
Copy link
Member

Also worth noting that you can already "overflow" ndigit. https://3v4l.org/RQ9iS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants