gen_stub.php add sensitive param in legacy file

[remicollet]

Description

See php-memcached-dev/php-memcached#516

in legacy file

zend_mark_function_parameter_as_sensitive(&class_entry_Memcached->function_table, "setsaslauthdata", 1);

This is obviously unwanted as this is 8.2+ only.
(legacy file is used by old versions, usually 7.x)

PHP Version

PHP 8.2.0alpha2

Operating System

N/A

[remicollet]

@kocsismate can you please take a look, related to debd38f

[TimWolla]

Not sure if it would help, but I'd be fine with dropping zend_mark_function_parameter_as_sensitive() entirely and just inlining its implementation in the *_arginfo.h files. I added the function primarily when it wasn't yet clear whether the attribute can be added by stubs.

[kocsismate]

@remicollet I'll take a look tomorrow!

[kocsismate]

Not sure if it would help, but I'd be fine with dropping zend_mark_function_parameter_as_sensitive() entirely and just inlining its implementation in the *_arginfo.h files. I added the function primarily when it wasn't yet clear whether the attribute can be added by stubs.

I think it's fine as-is. Inlining the function in arginfo files wouldn't work anyway, since the attribute class is not defined on older PHP versions.

When I thought about the possible solutions for the problem, I realized that the @generate-legacy-arginfo annotation doesn't matter much, since it doesn't affect compatibility with PHP 8.0 and PHP 8.1. What you really need is three different arginfo files:

• one for PHP 7 without types and sensitive parameters
• one for PHP 8.0 and PHP 8.1 with types, but without sensitive parameters
• one for full PHP 8.2 syntax

We could achieve this by adding support for a --target-version CLI option (possible values: 7, 8.0, 8.1, 8.2) and you should run gen_stub.php for all PHP versions. Alternatively, I could imagine adding support for parameters to the @generate-legacy-arginfo annotation, having the same possible values I mentioned above. Then you could include the correct arginfo file depending on PHP_VERSION_ID.

My other idea is to add the #ifdef PHP_VERSION_ID > 80200 directly around zend_mark_function_parameter_as_sensitive() calls. I think it would be a very lightweight solution, but it would be a bit weird to have these inside php-src, so I'm not really in favor of this idea... Alternatively, we could decide based on @generate-legacy-arginfo whether to include the ifdefs or not. In this case, @generate-legacy-arginfo should still have a minimally supported version parameter: @generate-legacy-arginfo 7 would be the default, and by declaring (@generate-legacy-arginfo 8.1, sensitive parameters were declared conditionally.

@remicollet Would any of these ideas sound reasonable to you? I think I lean towards the last idea.

[TimWolla]

since the attribute class is not defined on older PHP versions.

Unless attributes in native code differ from userland code, it should work if instead of referencing zend_ce_sensitive_parameter->name the SensitiveParameter name is also inlined (see #8839). Even if the attribute is not backed by an existing class, it can be applied.

This might even be desirable, because then the attribute is also visible to code running on PHP 8.1 and lower, if some exception handlers want to handle the sanitization in userland (the exception handler in the application I maintain does).

[kocsismate]

This might even be desirable, because then the attribute is also visible to code running on PHP 8.1 and lower, if some exception handlers want to handle the sanitization in userland

IMO we cannot do such major changes on already released versions.

[TimWolla]

IMO we cannot do such major changes on already released versions.

To clarify: I am not proposing doing this for released php-src versions / branches, that would be non-sense.

This is about non-bundled extensions that receive a new release (with a single new version number) where the release supports, say PHP 8.0 to 8.2. In those non-bundled extensions it might be desirable if the attribute was visible even if the extension runs on PHP 8.0. It's up to the maintainer, of course.