Can someone apply this to HEAD and PHP_5_4, or let me have karma.
Thanks
Alan
Can someone apply this to HEAD and PHP_5_4, or let me have karma.
Thanks
Alan
+/* {{{ proto bool is_subclass_of(mixed object_or_string, string
class_name [, bool allow_string=true])
+/* {{{ proto bool is_a(mixed object_or_string, string class_name [,
bool allow_string=false])
This is quite strange - two almost identical functions with completely
different default semantics.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Can someone apply this to HEAD and PHP_5_4, or let me have karma.
Thanks
Alan+/* {{{ proto bool is_subclass_of(mixed object_or_string, string
class_name [, bool allow_string=true])
+/* {{{ proto bool is_a(mixed object_or_string, string class_name [,
bool allow_string=false])This is quite strange - two almost identical functions with completely
different default semantics.
Yes, and if you can think of a way to depreciate them now so that it
does not break BC and could be changed later, please feel free, but I've
looked at it quite a bit and could not find any other way.
Regards
Alan
Can someone apply this to HEAD and PHP_5_4, or let me have karma.
Thanks
Alan+/* {{{ proto bool is_subclass_of(mixed object_or_string, string
class_name [, bool allow_string=true])
+/* {{{ proto bool is_a(mixed object_or_string, string class_name [,
bool allow_string=false])This is quite strange - two almost identical functions with completely
different default semantics.
Yes, and if you can think of a way to depreciate them now so that it
does not break BC and could be changed later, please feel free, but I've
looked at it quite a bit and could not find any other way.
Argh! Pet-peeve, it's deprecate. Depreciate means something completely
different.
I agree that it is slightly messy, but we have painted ourselves into a
bit of a corner with the 5.3 mess. Stas, the whole point here is that
changing the is_a() default in 5.3 caused huge problems, including
security ones, so setting allow_string to false by default fixes that BC
break in 5.3. Considering the huge amount of code hit by this I think we
should keep that default in 5.4.
Alan, does is_subclass_of() actually need the allow_string flag at all?
It has always been documented take a mixed and hit the autoloader, so
why would we need an option to disable the string?
-Rasmus
Can someone apply this to HEAD and PHP_5_4, or let me have karma.
Thanks
Alan+/* {{{ proto bool is_subclass_of(mixed object_or_string, string
class_name [, bool allow_string=true])
+/* {{{ proto bool is_a(mixed object_or_string, string class_name [,
bool allow_string=false])This is quite strange - two almost identical functions with completely
different default semantics.
Yes, and if you can think of a way to depreciate them now so that it
does not break BC and could be changed later, please feel free, but I've
looked at it quite a bit and could not find any other way.
Argh! Pet-peeve, it's deprecate. Depreciate means something completely
different.
Agh to much accounting recently ;)
I agree that it is slightly messy, but we have painted ourselves into a
bit of a corner with the 5.3 mess. Stas, the whole point here is that
changing theis_a()default in 5.3 caused huge problems, including
security ones, so setting allow_string to false by default fixes that BC
break in 5.3. Considering the huge amount of code hit by this I think we
should keep that default in 5.4.Alan, does
is_subclass_of()actually need the allow_string flag at all?
It has always been documented take a mixed and hit the autoloader, so
why would we need an option to disable the string?
I don't think I've used is_subclass_of for years, the flag is a knock on
effect from sharing the same code, and may be useful in some edge cases
(if somebody wanted to use subclass_of with user input.. ), since that
bit does not affect me, I do not mind either way.
To disable for subclass of it is quite trivial, I can get a full patch
ready (including updated tests) if we go that way.
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, only_subclass ?
"zs" : "zs|b", &obj, &class_name, &class_name_len, &allow_string) ==
FAILURE) {
Regards
Alan
-Rasmus
Hi!
I agree that it is slightly messy, but we have painted ourselves into a
bit of a corner with the 5.3 mess. Stas, the whole point here is that
changing theis_a()default in 5.3 caused huge problems, including
security ones, so setting allow_string to false by default fixes that BC
I've read complaints about is potentially causing security problems, but
is there code out there that was OK before and has security problem with
this change? I mean, a real-life app?
I'm thinking maybe we should have this options - but maybe have both
defaults set to true? This way if you have buggy code and you absolutely
refuse to move to proper code you can easily fix it by putting false
where needed, but at least our API is not broken anymore.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi!
I agree that it is slightly messy, but we have painted ourselves into a
bit of a corner with the 5.3 mess. Stas, the whole point here is that
changing theis_a()default in 5.3 caused huge problems, including
security ones, so setting allow_string to false by default fixes that BCI've read complaints about is potentially causing security problems, but is
there code out there that was OK before and has security problem with this
change? I mean, a real-life app?
There was example codes in previous discussions, here and on security.
The document used for the CVE assignment has some as well.
http://www.byte.nl/blog/2011/09/23/security-bug-in-is_a-function-in-php-5-3-7-5-3-8/
https://bugs.php.net/bug.php?id=55475
https://bugzilla.redhat.com/show_bug.cgi?id=741020
Now whether this exact use case is used in a real life app, nobody can
say it, but it really does not matter (or one can dig codesearch&co to
find some).
I'm thinking maybe we should have this options - but maybe have both
defaults set to true? This way if you have buggy code and you absolutely
refuse to move to proper code you can easily fix it by putting false where
needed, but at least our API is not broken anymore.
It is not correct. If a code, for whatever reason, was working fine
until now then there is no reason one has to change it to make it work
again, or even worst in this case, to make it safe again.
And yes, I agree that this kind of code is ugly and should not have
written that way, but we cannot do anything but be sure that we don't
make this code even worst.
Cheers,
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
Hi!
There was example codes in previous discussions, here and on security.
The document used for the CVE assignment has some as well.http://www.byte.nl/blog/2011/09/23/security-bug-in-is_a-function-in-php-5-3-7-5-3-8/
https://bugs.php.net/bug.php?id=55475
https://bugzilla.redhat.com/show_bug.cgi?id=741020
I know about these, this is why I specifically asked for real-life
examples. And yes, it does matter - if it's completely invented example,
it's one thing, if it's part of live widely deployed code, it's another.
It is not correct. If a code, for whatever reason, was working fine
until now then there is no reason one has to change it to make it work
again, or even worst in this case, to make it safe again.And yes, I agree that this kind of code is ugly and should not have
written that way, but we cannot do anything but be sure that we don't
make this code even worst.
The problem is not the beauty of such code, but the fact that we are
breaking our APIs to match somebody's code that is insecure anyway,
should never be written in such a way and could very well be completely
imaginary. The reason to change such code is exactly that this code is
insecure and broken, and the fact that nobody could break it by one
specific way doesn't mean there aren't other ways.
Changing APIs after they're public is very hard, I know it. However
right now the API we have is bad, and if we say we would never change
anything in order so broken code could stay broken, our API would be bad
forever.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi!
There was example codes in previous discussions, here and on security.
The document used for the CVE assignment has some as well.http://www.byte.nl/blog/2011/09/23/security-bug-in-is_a-function-in-php-5-3-7-5-3-8/
https://bugs.php.net/bug.php?id=55475
https://bugzilla.redhat.com/show_bug.cgi?id=741020I know about these, this is why I specifically asked for real-life examples.
And yes, it does matter - if it's completely invented example, it's one
thing, if it's part of live widely deployed code, it's another.
They are not completely invented examples. Or do you ask for any
single issue we have to show the real life examples? In short, we do
not.
It is not correct. If a code, for whatever reason, was working fine
until now then there is no reason one has to change it to make it work
again, or even worst in this case, to make it safe again.And yes, I agree that this kind of code is ugly and should not have
written that way, but we cannot do anything but be sure that we don't
make this code even worst.The problem is not the beauty of such code, but the fact that we are
breaking our APIs to match somebody's code that is insecure anyway,
The fact is that we have many broken APIs we can't fix for BC reasons.
We have to live with that in the 5.x serie.
should never be written in such a way and could very well be completely imaginary.
could, should, would, it does not change the problem for a single
yota. We can't have a variable ways to work with BC and security
issues. We simply can't as we have no way, absolutely no way, to be
sure that a given case is not used (widely or not).
The reason to change such code is exactly that this code is insecure and
broken, and the fact that nobody could break it by one specific way doesn't
mean there aren't other ways.
We have discussed that already on security, I barely see a reason to
begin this discussion again. There is a clear possible security
problem, clearly identified and not present before this "fix" was
applied. It is easy to fix and does not make PHP worst or better than
what it is now but only ensure that there is no BC, or new security
issues.
Cheers,
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
Hi!
We have discussed that already on security, I barely see a reason to
begin this discussion again. There is a clear possible security
problem, clearly identified and not present before this "fix" was
applied. It is easy to fix and does not make PHP worst or better than
what it is now but only ensure that there is no BC, or new security
issues.
Yes, the security problem was present before the fix was applied, and we
discussed it on security where I repeatedly pointed out that this code
has security hole regardless of any changes in PHP, the change only adds
one scenario where it can be exploited, but there are many others.
It definitely makes PHP worse by propagating inconsistent APIs.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Yes, the security problem was present before the fix was applied
No, it was not. See the examples in the links I pasted earlier.
The code was safe, under controlled context, before this change has
applied. With the change the code becomes unsafe under uncontrolled
context. That's not acceptable and besides the BC break, it introduce
a security flaw. As stated many times, by many persons, in the
previous discussion(s).
Cheers,
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
It definitely makes PHP worse by propagating inconsistent APIs.
I created a patch against 5.4:
https://bugs.php.net/patch-display.php?bug_id=55475&patch=is_a_5.4_alternati
ve&revision=latest
The patch changes the behavior to:
is_a("ab", "b") // false
is_a("ab", "b", true) // autoload "ab", autoload "b" -- false
is_subclass_of("ab", "b") // false
is_subclass_of("ab", "b", true) // autoload "ab", autoload "b" -- false
Both class names can be "autoloaded" but not by default
Thoughts?
Supporting strings 'by default' in is_a() has the downside that it
produces slightly unpredictable results. If you are accepting 'mixed'
arguments, some of which may be strings, there is a slim chance that the
string you accept will match the class or it's parent by accident.
Personally if I did not believe that the barrier for a BC change has to
be alot higher than 'prettier or consistent API', i would have suggested
that is_subclass_of should be changed to be 'consistent', and safer.
While the old justification for using is_a() over instanceof, has almost
gone (it was the only way to write code that was portable to PHP4 &
PHP5.*, except for the annoying depreciation message). I still consider
is_a(), especially with negative testing slightly clearer to read than
instanceof.
Regards
Alan
It definitely makes PHP worse by propagating inconsistent APIs.
I created a patch against 5.4:
https://bugs.php.net/patch-display.php?bug_id=55475&patch=is_a_5.4_alternati
ve&revision=latestThe patch changes the behavior to:
is_a("ab", "b") // false
is_a("ab", "b", true) // autoload "ab", autoload "b" -- falseis_subclass_of("ab", "b") // false
is_subclass_of("ab", "b", true) // autoload "ab", autoload "b" -- falseBoth class names can be "autoloaded" but not by default
Thoughts?