So I've started the refactor to change the stored string size from int to
I've got it compiling and the tests mostly passing (not all), when run with
--disable-all and --disable-cgi.
There are definitely still issues with the patch (there are some weird
segfaults in certain times, which are caught by the tests), but it's
progressing really nicely.
Here's what I did:
I created a new build option: --enable-zstrlen. This turns off the new
match, and type-defs and defines everything back to how it was before. This
is really useful for testing changes to ensure that they still work.
Then, I defined a series of new types:
#define zend_str_size_int int
#define zend_str_size_uint unsigned int
#define zend_str_size_size_t size_t
#define zend_str_size_long long
typedef int zend_str_size;
#define zend_str_size_int zend_str_size
#define zend_str_size_uint zend_str_size
#define zend_str_size_size_t zend_str_size
#define zend_str_size_long zend_str_size
typedef size_t zend_str_size;
Any API that accepted a string size parameter, I replace with one of the
zend_str_size_* definitions. I chose to do this instead of just changing it
directly to zend_str_size, as it should make extension developer's lives
easier by supporting the intermediate types (with their own define lines
for older versions of the API).
These are intended to be removed after 1 or 2 releases, replacing
everything with just zend_str_size.
Due to a problem with zend_parse_parameters, I added two new identifiers: S
and P. They act just like s and p, except that they return zend_str_size
instead of int.
When `--enable-zstrlen` is not enabled, I disable s and p, and changed ZPP
to rase an E_ERROR on unknown parameter types. The E_ERROR change is not
intended to go into production, but instead just makes life A LOT easier
refactoring modules one at a time.
Here's what's left to do:
I've only really got the basic system working (for limited definitions of
working). There's a ton of extensions that need migrating, and tons of
parts of the core that i haven't fully migrated yet.
I've migrated php_pcre.c over, but pcrelib still uses int for string sizes.
This is going to be a much larger refactor, and I wanted to see people's
thoughts prior to digging into it.
Substr needs to be refactored to use size_t. Right now, I just raise an
error if Z_STRSIZE > INT_MAX (or an overflow would happen). I'd love to see
that cleaned up more.
My general process has been to enable an extension, fix the compile errors
(typically due to removing Z_STRLEN*). Then run through the extension,
searching for int and replacing where appropriate with zend_str_size
(within a function) or zend_str_size_* in an API. Then run the tests for
that extension, and fix the issues as they come up. Finally, recompile with
-Werror and fix all of the warnings (yay!)...
Lessons Learned So Far
How this system is working today, I have no idea. There are SOOO many
issues in string handling just due to types. I've seen int, unsigned int,
size_t, long, unsigned long and others, silently cast back and forth
(implicit casts too). Some really weird things going on...
Here's the branch:
And the diff:
If you want to help out, please let me know and let's try to coordinate so
we don't step on each other's toes...
[PHP-INTERNALS] String Size Refactor Progress
- Michael Wallner:
- Christopher Jones: Can you elaborate on the "problem"? Ideally the return type of s & p would be toggled by ZEND_USE_LEGACY_STRING_TYPES. Chris -- [email protected] http://twitter.com/ghrd Free PHP & Oracle book: http://www.oracle.com/technetwork/topics/php/underground-php-oracle-manual-098250.html
- Anthony Ferrara: Chris, When `--enable-zstrlen` is not enabled, I disable s and p, and changed ZPP The problem is that changing 's' from int to size_t can (and does) cause segfaults. We can have the discussion if 's' and 'p' should remain there (and if the compile time option should be there) at merge time, but for now the switch just makes life easier making the change. It wasn't designed for a long-term switch... Anthony