Make zend_parse_parameters share fast zpp implementation where possible

[hikari-no-yume]

Since I don't like duplication, here's a patch that makes zend_parse_parameters use the fast zpp internal functions to do parameter parsing where possible, which so zpp and fast zpp share implementations. I think that'd be good as it'd mean that for most parameter types there's only one implementation of the parsing code.

This currently can't be safely merged, since if PHP's compiled with FAST_ZPP set to 0, then the fast zpp functions don't exist. Would it be reasonable to make the internal fast zpp functions always be available, and make FAST_ZPP only control the presence of the macros? :)

[hikari-no-yume]

By the way, this would actually fix a bug! 'z''s NULL check currently doesn't account for references, yet _z_param_zval does. This is an example of why having a shared implementation rather than two separate ones is a good thing: you can fix a bug in one place and it affects both zpp and fast zpp. :) (this isn't a bug actually, oops)

[hikari-no-yume]

Hey @dstogov, what do you think about this?

[hikari-no-yume]

This apparently broke some tests, need to look into that.

[dstogov]

I didn't verifiy all the code, but I like the idea

On Mon, Dec 15, 2014 at 5:47 AM, Andrea Faulds notifications@github.com
wrote:

This apparently broke some tests, need to look into that.

—
Reply to this email directly or view it on GitHub
#956 (comment).

[hikari-no-yume]

If we were to share code, it'd probably be better to rename these functions so they don't have the leading underscore. Would _z_param_long becoming zend_parse_parameter_long or something like that sound alright?

[hikari-no-yume]

Maybe, to go along with zend_parse_arg_impl, it could be zend_parse_arg_long?

[dstogov]

both look good.

Thanks. Dmitry.

On Mon, Dec 15, 2014 at 11:33 PM, Andrea Faulds notifications@github.com
wrote:

Maybe, to go along with zend_parse_arg_impl, it could be
zend_parse_arg_long?

—
Reply to this email directly or view it on GitHub
#956 (comment).

[hikari-no-yume]

Hey @dstogov, I'm confused about something: Why does fast zpp check if paths aren't zero-length, yet normal zpp doesn't? It seems pretty important, since me removing that check in fast zpp for symmetry with zpp made loads of stuff segfault.

[dstogov]

Hi Andrea,

Do you mean this line - zend_API.h:1190 - (check_null &&
UNEXPECTED(!(*dest)->val[0])) ||

It actually, was introduced by Anatol with commit 993d475.
I have no idea, why do we need it and what it was going to fix.

Anatol?

Thanks. Dmitry.

On Thu, Dec 18, 2014 at 5:49 AM, Andrea Faulds notifications@github.com
wrote:

Hey @dstogov https://github.com/dstogov, I'm confused about something:
Why does fast zpp check if paths aren't zero-length, yet normal zpp
doesn't? It seems pretty important, since me removing that check in fast
zpp for symmetry with zpp made loads of stuff segfault.

—
Reply to this email directly or view it on GitHub
#956 (comment).

[weltling]

Andrea, Dmitry,

zend_string's member to hold the actualy string is defined as val[1], not *val. Consequently check like

if (str->val) {...}

is always true, hence the change. I mean if that check is needed, which what it was looking like at the time of changing, so it should be

if (str->val[0])

to dereference the first char from the string. The flow shows that the passed string is at least allocated to that time.

Cheers

anatol

[weltling]

Hm, but i think Andrea was asking why that check is present at all. @TazeTSchnitzel in general it looks like to be making sense, as a zero length path could cause bad situations. Except you're moving it somewhere else appropriately.

[dstogov]

I didn't get it anyway.

"empty string" is not the same as "NULL". why empty string should behave as
IS_NULL in FAST_ZPP.

Thanks. Dmitry.

On Thu, Dec 18, 2014 at 12:42 PM, Anatol Belski notifications@github.com
wrote:

Andrea, Dmitry,

zend_string's member to hold the actualy string is defined as val[1], not
*val. Consequently check like

if (str->val) {...}

is always true, hence the change. I mean if that check is needed, which
what it was looking like at the time of changing, so it should be

if (str->val[0])

to dereference the first char from the string. The flow shows that the
passed string is at least allocated to that time.

Cheers

anatol

—
Reply to this email directly or view it on GitHub
#956 (comment).

[weltling]

Ah, now i see, the check_null argument regards to the subsequent check in _z_param_str(). The particular condition i was fixing (but actually not introduced) was looking like one wanted to check whether the str->val itself were NULL. Actually there are some places i see like Zend/zend_API.h:1177

if (check_null && UNEXPECTED(!str)) {

but despite it has nothing to do with the zval's IS_NULL, it is technically correct. I don't think "empty string" should be the same as "NULL", just it seemed to be logic to fix the condition that way. The subsequent check in that condition line 1191 does

UNEXPECTED(CHECK_NULL_PATH((_dest)->val, (_dest)->len))) {

and that's using strlen(str->val) . So this place, if check_null should be used at all, should rather check zend_string pointer as whole, so !str, not str->val.

[laruence]

@weltling why not if (!str->len) ?

[weltling]

@laruence yes, principally it were the same as !str->val[0], so check for an empty string. But on the next line it does

CHECK_NULL_PATH((*dest)->val, (*dest)->len)

so possibly it would dereference NULL which could come back from _z_param_str(). So imho it would be correct to do

(check_null && UNEXPECTED(!(*dest)))

on that line.

[hikari-no-yume]

I don't understand: You realise that val is an array of one element because strings are variable length, and the space beyond the struct is the data? Checking if the first character of the string is null and erroring if so makes no sense. Why is this check performed only if check_null is chosen - that doesn't make it fail on NULL, it makes it succeed on NULL but set the destination to NULL.

[hikari-no-yume]

More importantly, why does removing this check for fast zpp cause segfaults, yet zpp gets away fine without doing this check at all?

[weltling]

@TazeTSchnitzel the story from the start:

• first variant, wrong as !(*dest)->val is always false

(check_null && UNEXPECTED(!(*dest)->val))

• second variant, wrong as it checks for "zero length string", not for NULL. Or actually it might be both.

(check_null && UNEXPECTED(!(*dest)->val[0]))

• third variant, is how it maybe should be (if applicable to your patch)

(check_null && UNEXPECTED(!(*dest)))

The check_null is currently passed always as 0, so this part of code was never really used till now (Well, i tested it at the time of doing the second variant), see the Z_PARAM_PATH_STR macro. Btw. in that macro is also to see that it should fail when check_null is in effect.

To the third variant - _z_param_path_str() calls _z_param_str() which can ocassionally set *dest to NULL. That's why i think thi third variant is right and should be.

[hikari-no-yume]

If check_null is set and it's NULL, it should set the destination to NULL and succeed, however.

[hikari-no-yume]

Unless check_null has the opposite meaning to everywhere else.

[hikari-no-yume]

For other similar parameters, we error on NULL unless check_null is set. I think that's actually what we want here.

[weltling]

Yes, it seems correct. Say passing check_null you explicitly request to fail. Say placing Z_PARAM_PATH_STR_EX(str, 1, 0) would explicitly check the path correctness.

[hikari-no-yume]

But check_null makes it not fail on NULL. Also, quite importantly, that check should be carried out before conversion to string, not after.

[weltling]

yeah, i see now what you mean ... but for _z_param_long for example it's kinda different situation. For a path i could imagine some ext dev using explicitly Z_PARAM_PATH_STR_EX for example to ensure the correct path and that would be actually correlate with 'p' or 'P'. No?

[weltling]

why it makes it not fail on NULL, if it were

static zend_always_inline int _z_param_path_str(zval _arg, zend_string *_dest, int check_null)
{
if (!_z_param_str(arg, dest, check_null) ||
(check_null && UNEXPECTED(!(_dest))) || /_return 0 when the previous set _dest to NULL */
UNEXPECTED(CHECK_NULL_PATH((_dest)->val, (*dest)->len))) {
return 0;
}
return 1;
}

[weltling]

pah, when do i learn that md

[hikari-no-yume]

Alright, AIUI, check_null checks whether the argument is IS_NULL. If it's not set, the argument will be converted. If it is set, dest will be NULL. So what _z_param_path(_str) should be doing is failing when _z_param_str resulted in NULL and is_null is not set (paths can't be NULL), but if is_null is set, do nothing.

[hikari-no-yume]

So the condition should actually be (!check_null && UNEXPECTED(!(*dest))), I think.

[hikari-no-yume]

Wait, no, argh... I'm confused.

[hikari-no-yume]

Aha, I finally fixed the patch! This can be merged, then :)

Will do if no objections.

[hikari-no-yume]

I think I figured out what the segfault issue was with the NULL string check. If the string is NULL, then dereferencing it to do a nul-byte path check will segfault. It shouldn't cause zpp failure, though, as we never do this for NULL.

So I've rewritten it like this:

(*dest && UNEXPECTED(CHECK_NULL_PATH((*dest)->val, (*dest)->len)))

This way, if the string is NULL, it won't do the null path check and hence won't segfault.

The string isn't usually NULL. By default, we convert IS_NULL to an empty string. However, with is_null set to 1, we don't convert if the argument was IS_NULL and instead set the string pointer to NULL. The check in FAST_ZPP was probably to make sure that in this case, where parsing succeeds but the string was NULL, we error out before CHECK_NULL_PATH segfaults trying to dereference a NULL pointer.

I'd looked at ZPP to see what it did, and was confused at why it lacked this check. I see now that this wasn't a problem in ZPP, because it'd break; immediately if is_null was set to 1 and the argument was IS_NULL. However, in FAST_ZPP, the path-parsing functions call the string-parsing functions, so they will end up with NULL pointers when the string-parsing functions produce them, thus the need for the check.

So, yeah, I think I've solved this. ^^

[hikari-no-yume]

Aha, yes, it's green on Travis. It works. ^^

[hikari-no-yume]

@dstogov any objections to merging? I feel I double-check with you since you're the FAST_ZPP expert.

[dstogov]

go forward. everything looks fine.

Thanks. Dmitry.

On Sun, Dec 28, 2014 at 9:51 PM, Andrea Faulds notifications@github.com
wrote:

@dstogov https://github.com/dstogov any objections to merging? I feel I
double-check with you since you're the FAST_ZPP expert.

—
Reply to this email directly or view it on GitHub
#956 (comment).