Everything

php

C++ and FAST_ZPP macros

php.internals

Sara Golemon

8 years ago

This blog post came across my twitter today and it's certainly legit.
https://cismon.net/2017/12/18/Fast-ZPP-s-Incompatibility-with-CPP/

I tossed together this quick and dirty fix (and tested it with a
simple C++ extension), but I wanted to get a read on what branch folks
think it should be applied to.
https://github.com/sgolemon/php-src/commit/469ddd26331dbd736ad13eaac7170ccc43d09c7f

As the blog post notes, it's a simple matter to work around the bug in
extension code (indeed, an extension can simply opt to not use
FAST_ZPP).  On the other hand, the fix is pretty basic, and existing
functionality of the default expected type effectively being
Z_EXPECTED_LONG (because both have a value of zero) is just a bit....
weird.

Thoughts?  If I don't hear anything in a week, I'll just apply to 7.1
and merge up.

-Sara

Levi Morrison

8 years ago

On Mon, Dec 18, 2017 at 12:43 PM, Sara Golemon <pollita@php.net> wrote:

> This blog post came across my twitter today and it's certainly legit.
> https://cismon.net/2017/12/18/Fast-ZPP-s-Incompatibility-with-CPP/
>
> I tossed together this quick and dirty fix (and tested it with a
> simple C++ extension), but I wanted to get a read on what branch folks
> think it should be applied to.
> https://github.com/sgolemon/php-src/commit/469ddd26331dbd736ad13eaac7170ccc43d09c7f
>
> As the blog post notes, it's a simple matter to work around the bug in
> extension code (indeed, an extension can simply opt to not use
> FAST_ZPP).  On the other hand, the fix is pretty basic, and existing
> functionality of the default expected type effectively being
> Z_EXPECTED_LONG (because both have a value of zero) is just a bit....
> weird.
>
> Thoughts?  If I don't hear anything in a week, I'll just apply to 7.1
> and merge up.
>
> -Sara
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php

Is our macro `#define Z_EXPECTED_TYPE_STR(id, str) str,` ever used? If
so there might be a change in perceived behavior because the first
entry previously had "integer" and now it is "mixed".

Sara Golemon

8 years ago

On Mon, Dec 18, 2017 at 3:38 PM, Levi Morrison <levim@php.net> wrote:

>> Thoughts?  If I don't hear anything in a week, I'll just apply to 7.1
>> and merge up.
>>
> Is our macro `#define Z_EXPECTED_TYPE_STR(id, str) str,` ever used? If
> so there might be a change in perceived behavior because the first
> entry previously had "integer" and now it is "mixed".
>

It exists for the purpose of generating output message when the type
is not cast/coercible to the expected type.  The index of the string
entry corresponds 1:1 with the value of the enum, so it'll only show
"mixed" when the expect type was ANY and we failed to cast/coerce to
ANY (which will obviously never happen).

In fact, the previous state where _expected_type was initialized to
IS_UNDEF (and by extension interpreted poorly as Z_EXPECTED_LONG)
would also never happen because the cast/coersion error is only
produced by P_PARAM_*() macros who have in turn explicitly reset
_expected_type to some specific value.

The default initialization exists only to silence unhelpful compiler
warnings and not to provide any actual use or effect.

-Sara

Sara Golemon

8 years ago

On Mon, Dec 18, 2017 at 4:11 PM, Sara Golemon <pollita@php.net> wrote:

> On Mon, Dec 18, 2017 at 3:38 PM, Levi Morrison <levim@php.net> wrote:
>>> Thoughts?  If I don't hear anything in a week, I'll just apply to 7.1
>>> and merge up.
>>>
>> Is our macro `#define Z_EXPECTED_TYPE_STR(id, str) str,` ever used? If
>> so there might be a change in perceived behavior because the first
>> entry previously had "integer" and now it is "mixed".
>>
> It exists for the purpose of generating output message when the type
> is not cast/coercible to the expected type.  The index of the string
> entry corresponds 1:1 with the value of the enum, so it'll only show
> "mixed" when the expect type was ANY and we failed to cast/coerce to
> ANY (which will obviously never happen).
>
> In fact, the previous state where _expected_type was initialized to
> IS_UNDEF (and by extension interpreted poorly as Z_EXPECTED_LONG)
> would also never happen because the cast/coersion error is only
> produced by P_PARAM_*() macros who have in turn explicitly reset
> _expected_type to some specific value.
>
> The default initialization exists only to silence unhelpful compiler
> warnings and not to provide any actual use or effect.
>

To clarify one last thing: Simply changing the IS_UNDEF on that
initialization line to Z_EXPECTED_LONG would have also worked here
because as stated above, it's never *actually* used without being
reset to a meaningful case.  I went with a new enum value to make the
intent more clear to someone reading this in the future.

If it makes anyone more comfortable, I can make the 7.1/7.2 fix be
that simple, and add the new enum value in master, or even just forgo
the new enum value in favor of an inline comment explaining why the
default value in Z_EXPECTED_LONG.

-Sara

Levi Morrison

8 years ago

On Mon, Dec 18, 2017 at 2:31 PM, Sara Golemon <pollita@php.net> wrote:

> On Mon, Dec 18, 2017 at 4:11 PM, Sara Golemon <pollita@php.net> wrote:
>> On Mon, Dec 18, 2017 at 3:38 PM, Levi Morrison <levim@php.net> wrote:
>>>> Thoughts?  If I don't hear anything in a week, I'll just apply to 7.1
>>>> and merge up.
>>>>
>>> Is our macro `#define Z_EXPECTED_TYPE_STR(id, str) str,` ever used? If
>>> so there might be a change in perceived behavior because the first
>>> entry previously had "integer" and now it is "mixed".
>>>
>> It exists for the purpose of generating output message when the type
>> is not cast/coercible to the expected type.  The index of the string
>> entry corresponds 1:1 with the value of the enum, so it'll only show
>> "mixed" when the expect type was ANY and we failed to cast/coerce to
>> ANY (which will obviously never happen).
>>
>> In fact, the previous state where _expected_type was initialized to
>> IS_UNDEF (and by extension interpreted poorly as Z_EXPECTED_LONG)
>> would also never happen because the cast/coersion error is only
>> produced by P_PARAM_*() macros who have in turn explicitly reset
>> _expected_type to some specific value.
>>
>> The default initialization exists only to silence unhelpful compiler
>> warnings and not to provide any actual use or effect.
>>
> To clarify one last thing: Simply changing the IS_UNDEF on that
> initialization line to Z_EXPECTED_LONG would have also worked here
> because as stated above, it's never *actually* used without being
> reset to a meaningful case.  I went with a new enum value to make the
> intent more clear to someone reading this in the future.
>
> If it makes anyone more comfortable, I can make the 7.1/7.2 fix be
> that simple, and add the new enum value in master, or even just forgo
> the new enum value in favor of an inline comment explaining why the
> default value in Z_EXPECTED_LONG.
>
> -Sara

I am fine with the change; I just wanted to double-check that aspect.

Anatoliy Belsky

8 years ago

Hi Sara,

> -----Original Message-----
> From: php@golemon.com [mailto:php@golemon.com] On Behalf Of Sara
> Golemon
> Sent: Monday, December 18, 2017 8:44 PM
> To: PHP internals <internals@lists.php.net>
> Subject: [PHP-DEV] C++ and FAST_ZPP macros
>
> This blog post came across my twitter today and it's certainly legit.
> https://cismon.net/2017/12/18/Fast-ZPP-s-Incompatibility-with-CPP/
>
> I tossed together this quick and dirty fix (and tested it with a simple C++
> extension), but I wanted to get a read on what branch folks think it should be
> applied to.
> https://github.com/sgolemon/php-
> src/commit/469ddd26331dbd736ad13eaac7170ccc43d09c7f
>
> As the blog post notes, it's a simple matter to work around the bug in extension
> code (indeed, an extension can simply opt to not use FAST_ZPP).  On the other
> hand, the fix is pretty basic, and existing functionality of the default expected
> type effectively being Z_EXPECTED_LONG (because both have a value of zero) is
> just a bit....
> weird.
>
> Thoughts?  If I don't hear anything in a week, I'll just apply to 7.1 and merge up.
>

C++11 requires a validity scope for enums, thus this change is unavoidable. IMO your second suggestion were safer in the spirit of BC - Z_EXPECTED_LONG instead of IS_UNDEF by default, and a new in master. Just because a workaround is possible and otherwise the issue is not critical.

Regards

Anatol

Sara Golemon

8 years ago

On Tue, Dec 19, 2017 at 3:29 PM, Anatol Belski <ab@php.net> wrote:

>> As the blog post notes, it's a simple matter to work around the bug in extension
>> code (indeed, an extension can simply opt to not use FAST_ZPP).  On the other
>> hand, the fix is pretty basic, and existing functionality of the default expected
>> type effectively being Z_EXPECTED_LONG (because both have a value of zero) is
>> just a bit....
>> weird.
>>
>> Thoughts?  If I don't hear anything in a week, I'll just apply to 7.1 and merge up.
>>
> C++11 requires a validity scope for enums, thus this change is unavoidable.
> IMO your second suggestion were safer in the spirit of BC - Z_EXPECTED_LONG
> instead of IS_UNDEF by default, and a new in master. Just because a workaround
> is possible and otherwise the issue is not critical.
>

Yeah, the more I think about it, the more I realize that's the more
prudent approach given the non-critical nature of this.  Could you
clarify as 7.0 RM if you want this on your branch? AIUI we're in
security-only for 7.0 now, yes?

-Sara

Anatoliy Belsky

8 years ago

> -----Original Message-----
> From: php@golemon.com [mailto:php@golemon.com] On Behalf Of Sara
> Golemon
> Sent: Tuesday, December 19, 2017 10:05 PM
> To: Anatol Belski <ab@php.net>
> Cc: PHP internals <internals@lists.php.net>
> Subject: Re: [PHP-DEV] C++ and FAST_ZPP macros
>
> On Tue, Dec 19, 2017 at 3:29 PM, Anatol Belski <ab@php.net> wrote:
> >> As the blog post notes, it's a simple matter to work around the bug
> >> in extension code (indeed, an extension can simply opt to not use
> >> FAST_ZPP).  On the other hand, the fix is pretty basic, and existing
> >> functionality of the default expected type effectively being
> >> Z_EXPECTED_LONG (because both have a value of zero) is just a bit....
> >> weird.
> >>
> >> Thoughts?  If I don't hear anything in a week, I'll just apply to 7.1 and merge
> up.
> >>
> > C++11 requires a validity scope for enums, thus this change is unavoidable.
> > IMO your second suggestion were safer in the spirit of BC -
> > Z_EXPECTED_LONG instead of IS_UNDEF by default, and a new in master.
> > Just because a workaround is possible and otherwise the issue is not critical.
> >
> Yeah, the more I think about it, the more I realize that's the more prudent
> approach given the non-critical nature of this.  Could you clarify as 7.0 RM if you
> want this on your branch? AIUI we're in security-only for 7.0 now, yes?
>

In case of 7.0, it's a border case now. 7.0.27RC1 is out, the final is not yet. Strictly speaking, this issue doesn't qualify as a last second fit. Nevertheless I'd say, replacing IS_UNDEF with Z_EXPECTED_LONG is totally fine in C89 as it's only aware of the actual value, the issue in latest C++ is fixed thereby. From Jordi's stats 2017 is to see, that 7.0 was 1/3 in January at least, perhaps less today, so in general it'd be still some goal projects would target. As a last second patch, I think the nonintrusive variant would be acceptable, as C++ standard moves forward an we'd see ever more usage even of C++14 much later this year. If no one sees an obvious issue, please merge into 7.0 next days. I'll be testing subsequently as well with any possible exts. 7.0.27 GA is the cut.

Thanks

Anatol