Fix bug #40531 mb_substr optional parameters

[mytskine]

The fix is in 3 commits. First, I enable a test that was skipped. Then I add a small feature to zend_verify_arg_type(). The last commit modifies the 2 functions that were buggy, mb_substr() and mb_strcut(). The last commit message details the changes.

I based my work on the branch PHP-5.4, but maybe this fix should be applied to an older version.

[nikic]

This seems like a rather strange way to fix the issue. Changing the Zend Engine should not be required. What would be the problem with fetching the value as z, then checking for IS_NULL and otherwise converting it to a long using convert_to_long?

[mytskine]

I first did like you suggested. But then there is no parameter validation in
the function. I couldn't find any function that checks the parameter is an
integer without converting it to an int. So instead of duplicating code, I added
this type check to the Zend Engine.

This is my first look at PHP's code, so of course I may be completly wrong.

[mytskine]

Le 2012-03-20, nikic
[email protected]
a écrit :

This seems like a rather strange way to fix the issue. Changing the Zend
Engine should not be required. What would be the problem with fetching the
value as z, then checking for IS_NULL and otherwise converting it to a
long using convert_to_long?

---

Reply to this email directly or view it on GitHub:
#13 (comment)

I've just seen that my second commit re-introduces something that was removed
by commit 0e24a7c on 2010-10-19 by D. Rethans:
"Strip out the typehint checks only".

It seems my proposed patch has just landed in the hot topic of scalar type
hinting. I do think my patch is simple and logical, and I can't see a simpler
solution. But I don't have enough background to understand the consequences it
would bare on type hinting in PHP.

[smalyshev]

I don't think changes to how the engine handles arguments can be introduced without RFC and discussion.

[php-pulls]

Comment on behalf of johannes at php.net:

test (please ignore)

[ircmaxell]

This actually has a direct impact on the scalar type hinting discussion as it actually implements the type-hints differently for the core functions which as-of-yet is not supported...

Why not just change the arg type in zend_parse_parameters to expect an int in that case (like every other core function)...?

Anthony

[cataphract]

As you can't use the ! modifier with l, the solution is simply to call zend_get_parameters_ex or the like before.

For instance, I have this still uncommitted code for the intl extension:

U_CFUNC PHP_FUNCTION(intlcal_clear)
{
    zval    **args_a[2] = {0},
            ***args     = &args_a[0];
    long    field;
    int     variant;
    CALENDAR_METHOD_INIT_VARS;

    if (ZEND_NUM_ARGS() > (getThis() ? 1 : 2) ||
            zend_get_parameters_array_ex(getThis() ? 1 : 2, args) == FAILURE) {
        intl_error_set(NULL, U_ILLEGAL_ARGUMENT_ERROR,
            "intlcal_clear: too many arguments", 0 TSRMLS_CC);
        RETURN_FALSE;
    }
    if (!getThis()) {
        args++;
    }
    if (args[0] == NULL || Z_TYPE_PP(args[0]) == IS_NULL) {
        zval *dummy; /* we know it's null */
        if (zend_parse_method_parameters(ZEND_NUM_ARGS() TSRMLS_CC,
                getThis(), "O|z", &object, Calendar_ce_ptr, &dummy) == FAILURE) {
            intl_error_set(NULL, U_ILLEGAL_ARGUMENT_ERROR,
                "intlcal_clear: bad arguments", 0 TSRMLS_CC);
            RETURN_FALSE;
        }
        variant = 0;
    } else if (zend_parse_method_parameters(ZEND_NUM_ARGS() TSRMLS_CC,
            getThis(), "Ol", &object, Calendar_ce_ptr, &field) == FAILURE) {
        intl_error_set(NULL, U_ILLEGAL_ARGUMENT_ERROR,
            "intlcal_clear: bad arguments", 0 TSRMLS_CC);
        RETURN_FALSE;
    } else if (field < 0 || field >= UCAL_FIELD_COUNT) {
        intl_error_set(NULL, U_ILLEGAL_ARGUMENT_ERROR,
            "intlcal_clear: invalid field", 0 TSRMLS_CC);
        RETURN_FALSE;
    } else {
        variant = 1;
    }

[mytskine]

A few comments about ctaphract's suggestion:

• zend_get_parameters_ex() is not documented. Not on php.net, but neither
  in the code (except for a cryptic comment). The variant functions are the
  same (no doc, no comments).
• It is declared as ZEND_API ZEND_ATTRIBUTE_DEPRECATED. Deprecated?
• I don't understand how this function work, so I won't try to use it.
  It is almost never used in PHP-5.4, this lack of examples doesn't help.

More globally, here are my "naïve" thoughts:

• I think it is strange that a php internal function can't declare that one of
  its arguments is integer or null.
• If I understood well, the suggested solution is to parse every argument "by
  hand" in this case, including the error messages when validating each
  argument.
• If I didn't misunderstand, this would lead to a heavy rewrite of the parsing
  of the arguments in the functions mb_substr() and mb_strcut(). It would
  add dozens of lines of tedious code.
• I feel that it is wiser to wait for a way to define more completely the
  prototype of the functions (through zend_parse_parameters() or scalar type
  hinting or anything else). But I'm not qualified enough to defend my point of
  view.

Finally, I believe I should have posted in the internals list instead of
posting a pull request. I now understand the patch was too intrusive to be
applied, and a discussion was much needed, at least for me!

[php-pulls]

Hi!

• I think it is strange that a php internal function can't declare that one of
  its arguments is integer or null.

I think it's because there never was a use case for it. You actually
stepped on a much bigger problem - how to omit optional arguments if
it's not the last one. Unfortunately, PHP does not provide proper
solution to it - and null is not a very good solution since it makes
substr() and mb_substr() work differently.
I plan to propose better solution soon but of course it won't happen for
5.4.

Right now I don't see a clean way to return null in 'l' field from
zend_parse_arg_impl(), so I'm afraid you'd have to deal with 'z' and do
your own parsing. If you really want it (see above).

• I feel that it is wiser to wait for a way to define more completely the
  prototype of the functions (through zend_parse_parameters() or scalar type
  hinting or anything else). But I'm not qualified enough to defend my point of
  view.

zend_parse_parameters() does define type, but type "long or null" is not
expressible in its language right now. 'l' can not return null since it
accepts int* and thus returns int. We could have some type that returns
"is it null?" flag, but that would be very limited use case, I'm not
sure it's worth it.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

[mytskine]

Thanks for this reply. I'm glad to know there are plans for a better way to deal with optional arguments.

I still think that the type checks for the arguments of internal functions are too limited. Maybe type checks shouldn't be added to zend_verify_arg_type() (please note it does not enable type-hinting for user functions, though the subject is connected). If not, I feel the API lacks a helper function to validate individual arguments. I've wandered a bit in the code of several extensions, and I've often seen a function call several times zend_parse_parameters() just because one of its arguments is "int or string". I don't like this kind of code (no offense intended, I reckon I lack experience on php-src, so my opinions have a very small background).

I'm closing this pull request. I may submit another one with my own parsing and validation and error messages for "long or null" arguments, but I'll probably wait for changes in the API that would avoid this. Thanks to everyone here for the valuable experience.

[cataphract]

I meant zend_get_parameters_array_ex, not zend_get_parameters_ex, which is in fact deprecated (but otherwise operates in a similar fashion). You can also use first zend_parse_parameters with a Z* or z* to determine whether the argument is NULL and then call zend_parse_parameters again with the proper arguments. Only by using these methods will you be able to distinguish between 0 and NULL after the second (proper) call.

In any case, you won't need to add your own error messages or type validations. I should have stripped those from my example because the intl extension actually has its own error handling mechanism. The only exception is that you have to validate the number of arguments if you use zend_get_parameters_ex, otherwise you may overflow the array you pass. But this is not a problem if you use zpp with z* (though then you to be careful not to free the array obtained).

In the long term, adding a modifier similar to ! would be a better option. The problem with ! is that it only works with pointer types (and zend_fcall_info/cache). We need a modifier that allows the nullness to be specified on an adjacent argument passed to zpp, like this (using as an example ?):

long l;
zend_bool is_null = 1;
if (zend_parse_method_parameters(ZEND_NUM_ARGS() TSRMLS_CC,
            getThis(), "O|l?", &object, Calendar_ce_ptr, &l, &is_null) == FAILURE) {

[smalyshev]

I would rather do it differently - I'd allow to run parse on a single parameter, i.e. have initial parse with 'z' and then depending on some code have something like:

zend_parse_single_arg(my_arg, "l", &l)

So you could do your own checks and then still benefit from the clean API provided by parameter parser.