I remeber we discussed using asserts and decided they're a bad idea for WMF-deployed code - yet I see
Warning: assert() [<a href='function.assert'>function.assert</a>]: Assertion failed in /usr/local/apache/common-local/php-1.22wmf12/extensions/WikibaseDataModel/DataModel/Claim/Claims.php on line 291
Thoughts?
On Tue, Jul 30, 2013 at 5:28 PM, Max Semenik maxsem.wiki@gmail.com wrote:
Warning: assert() [<a href='function.assert'>function.assert</a>]: Assertion failed in /usr/local/apache/common-local/php-1.22wmf12/extensions/WikibaseDataModel/DataModel/Claim/Claims.php on line 291
Is this on a production server? If so then the even more confusing question would be why assertions are enabled on an enterprise system...
As for whether MW should use assertions, I don't remember/wasn't there for the original discussion, so I can't comment on that, although personally I don't see how they're that bad an idea.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2016 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
As for whether MW should use assertions, I don't remember/wasn't there for the original discussion, so I can't comment on that, although personally I don't see how they're that bad an idea.
I wasn't there either; but from my experience assertions are bad because you are using them to guard for unexpected behavior and to terminate upon detection. This is, usually, unexpected. IMO throw an exception if you're going to do this checking; then you at least get the stack trace (and function arguments!) for the last chance error handler; not to mention you're giving upstream code the chance to catch it in case it can be handled.
~Matt Walker Wikimedia Foundation Fundraising Technology Team
On Tue, Jul 30, 2013 at 2:30 PM, Tyler Romeo tylerromeo@gmail.com wrote:
On Tue, Jul 30, 2013 at 5:28 PM, Max Semenik maxsem.wiki@gmail.com wrote:
Warning: assert() [<a href='function.assert'>function.assert</a>]: Assertion failed in
/usr/local/apache/common-local/php-1.22wmf12/extensions/WikibaseDataModel/DataModel/Claim/Claims.php
on line 291
Is this on a production server? If so then the even more confusing question would be why assertions are enabled on an enterprise system...
As for whether MW should use assertions, I don't remember/wasn't there for the original discussion, so I can't comment on that, although personally I don't see how they're that bad an idea.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2016 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 31/07/13 07:28, Max Semenik wrote:
I remeber we discussed using asserts and decided they're a bad idea for WMF-deployed code - yet I see
Warning: assert() [<a href='function.assert'>function.assert</a>]: Assertion failed in /usr/local/apache/common-local/php-1.22wmf12/extensions/WikibaseDataModel/DataModel/Claim/Claims.php on line 291
The original discussion is here:
http://thread.gmane.org/gmane.science.linguistics.wikipedia.technical/59620
Judge for yourself.
-- Tim Starling
I think the real issue here is just that assertions sometimes aren't used correctly.
Assertions and exceptions are fundamentally different concepts. Assertions should be used for statements that literally should always be true. And I mean that almost mathematically, as in most assertions should be able to be logically proven. This is why they can be turned off on production servers, because they simply won't happen.
Exceptions are just what the name says: exceptions. While they shouldn't happen often, exceptions do happen, and thus need to be caught and handled.
Also, assertions in PHP do not have any performance overhead once they're turned off for production servers, so that won't be an issue either.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2016 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Tue, Jul 30, 2013 at 6:28 PM, Tim Starling tstarling@wikimedia.orgwrote:
On 31/07/13 07:28, Max Semenik wrote:
I remeber we discussed using asserts and decided they're a bad idea for WMF-deployed code - yet I see
Warning: assert() [<a href='function.assert'>function.assert</a>]: Assertion failed in
/usr/local/apache/common-local/php-1.22wmf12/extensions/WikibaseDataModel/DataModel/Claim/Claims.php
on line 291
The original discussion is here:
< http://thread.gmane.org/gmane.science.linguistics.wikipedia.technical/59620
Judge for yourself.
-- Tim Starling
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 31/07/13 08:45, Tyler Romeo wrote:
Assertions and exceptions are fundamentally different concepts. Assertions should be used for statements that literally should always be true. And I mean that almost mathematically, as in most assertions should be able to be logically proven. This is why they can be turned off on production servers, because they simply won't happen.
Interesting concept. I think in C, they are most often used for validating function input, so obviously they can be hit. The Wikipedia articles [[Assertion (software development)]] and [[Precondition]] both mention this usage.
In the Wikidata code in question, assertions are used for both preconditions and postconditions of non-private functions.
-- Tim Starling
Hey,
Assertions should be used for statements that literally should always be
true.
Indeed. This is the case for the assertion that is failing. Apparently there is a bug somewhere, and a lack of appropriate tests. But never mind that bug, we got a much more exciting flame war to keep going here :)
IMO throw an exception if you're
going to do this checking; then you at least get the stack trace (and function arguments!) for the last chance error handler; not to mention you're giving upstream code the chance to catch it in case it can be handled.
Agreed. Exceptions are generally better. Assertions are the lazy way out, just like type hints. Type hint violations do pretty much the same thing as assertion violations. Arguing against asserts while not minding type hints is odd, as the later is arguably more of an issue, as these ones causes issues on invalid input. Putting in a pile of ifs that do instanceof checks and whatnot does cause clutter.
So as with pretty much everything in the field of engineering, think about the tradeoffs of the various approaches whenever you need to pick one. Deciding on one and then religiously following it everywhere is going to end poorly, no matter which approach you pick.
ps. Exception handling is generally done quite badly in MW core and extensions. See http://sourceforge.net/mailarchive/message.php?msg_id=31231379
Cheers
-- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. ~=[,,_,,]:3 --
Hi Tyler,
good to see that since the last discussion of this topic, more people are in favor of allowing asserts :-)
On Tue, Jul 30, 2013 at 06:45:37PM -0400, Tyler Romeo wrote:
I think the real issue here is just that assertions sometimes aren't used correctly.
I wholeheartedly agree.
Best regards, Christian
On 07/30/2013 06:28 PM, Tim Starling wrote:
On 31/07/13 07:28, Max Semenik wrote:
I remeber we discussed using asserts and decided they're a bad idea for WMF-deployed code - yet I see
Warning: assert() [<a href='function.assert'>function.assert</a>]: Assertion failed in /usr/local/apache/common-local/php-1.22wmf12/extensions/WikibaseDataModel/DataModel/Claim/Claims.php on line 291
The original discussion is here:
http://thread.gmane.org/gmane.science.linguistics.wikipedia.technical/59620
Judge for yourself.
I'll further elaborate on the "[...] you have to put the source code inside a string [...]" part. From the [documentation][1]:
If the assertion is given as a string it will be evaluated as PHP code by assert().
As in: that function is just as evil as eval(), and the innocent looking
assert( "$_GET[id] > 0" );
can actually be a security vulnerability, depending on server configuration (yes, servers can be and are misconfigured). And when assert() is used like this (yes, there actually is one of these in WikibaseDataModel):
assert( $this->functionFromSuperclass() );
it might be necessary to check multiple files to verify that a string is not passed to assert().
Perhaps it might make sense to do
assert( (bool)( ... ) );
though, as pointed out previously, this really is no better than, say:
if ( !( ... ) ) { throw new MWException( '...' ); }
[1]: http://php.net/manual/en/function.assert.php
On Tue, Jul 30, 2013 at 7:37 PM, Kevin Israel pleasestand@live.com wrote:
As in: that function is just as evil as eval(), and the innocent looking
assert( "$_GET[id] > 0" ); assert( $this->functionFromSuperclass() );
This is what I mean by misusing the assert function. Assert should always be called by passing a single-quoted string as an argument. If used correctly, it is no more a security vulnerability than if you were to put the same code into an if statement.
Also, like I said, assertions are for statements that are always true, so checking user input with assertions is incorrect.
Interesting concept. I think in C, they are most often used for
validating function input, so obviously they can be hit. The Wikipedia articles [[Assertion (software development)]] and [[Precondition]] both mention this usage.
Using assertions to validate function input is indeed a valid usage, but it should be done in ways where they won't be hit. In other words, they should not be used for data validation; they should be used in cases where *the program expects the data to already be valid*.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2016 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On 31/07/13 09:46, Tyler Romeo wrote:
Interesting concept. I think in C, they are most often used for validating function input, so obviously they can be hit. The Wikipedia articles [[Assertion (software development)]] and [[Precondition]] both mention this usage.
Using assertions to validate function input is indeed a valid usage, but it should be done in ways where they won't be hit. In other words, they should not be used for data validation; they should be used in cases where *the program expects the data to already be valid*.
I think exceptions should be used for that. Like I said in 2012, the implementation of assertions in PHP has lots of problems.
You said previously:
This is why they can be turned off on production servers, because they simply won't happen.
You can't mathematically prove that the behaviour of future developers will follow your expectations. Assertions intended to enforce developer behaviour are routinely hit in production. It's not correct to assume that unit testing will discover all bugs, and that production code will be perfect.
Also, assertions in PHP do not have any performance overhead once they're turned off for production servers, so that won't be an issue either.
That's only the case when the code is encoded as a string, which it's not in Wikibase. And as PleaseStand points out, the fact that assert() can act like eval() can make it hard to verify that code is not an arbitrary script execution vulnerability.
Note that the current thread is about a bug discovered by an assertion logged in production. If we disabled assertions in production, we would not know about it.
-- Tim Starling
My take on assertions, which I also tried to stick to in Wikibase, is as follows:
* A failing assertion indicates a "local" error in the code or a bug in PHP; They should not be used to check preconditions or validate input. That's what InvalidArgumentException is for (and I wish type hints would trigger that, and not a "fatal error"). Precondition checks can always fail, never trust the caller. Assertions are things that should *always* be true.
* Use assertions to check postconditions (and perhaps invariants). That is, use them to assert that the code in the method (and maybe class) that contains the assert is correct. Do not use them to enforce caller behavior.
* Use boolean expressions in assertions, not strings. The speed advantage of strings is not big, since the expression should be a very basic one anyway, and strings are awkward to read, write, and, as mentioned before, potentially dangerous, because they are eval()ed.
* The notion of "bailing out" on "fatal errors" is a misguided remnant from the days when PHP didn't have exceptions. In my mind, assertions should just throw an (usually unhandled) exception, like Java's AssertionError.
I think if we stick with this, assertions are potentially useful, and harmless at worst. But if there is consensus that they should not be used anywhere, ever, we'll remove them. I don't really see how the resulting boiler plate would be cleaner or safer:
if ( $foo > $bar ) { throw new OMGWTFError(); }
-- daniel
Am 31.07.2013 00:28, schrieb Tim Starling:
On 31/07/13 07:28, Max Semenik wrote:
I remeber we discussed using asserts and decided they're a bad idea for WMF-deployed code - yet I see
Warning: assert() [<a href='function.assert'>function.assert</a>]: Assertion failed in /usr/local/apache/common-local/php-1.22wmf12/extensions/WikibaseDataModel/DataModel/Claim/Claims.php on line 291
The original discussion is here:
http://thread.gmane.org/gmane.science.linguistics.wikipedia.technical/59620
Judge for yourself.
-- Tim Starling
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Hi,
On Wed, Jul 31, 2013 at 10:36:56AM +0200, Daniel Kinzler wrote:
- Use boolean expressions in assertions, not strings.
I do not agree that this is best practice in PHP.
Execution time being only part of argument here. Among other arguments are readability of the error message. When using strings, the error message contains the condition of the failed assertion.
But as has been pointed out by other posts in this thread, correct quotation of the string is obviously crucial.
The speed advantage of strings is not big, since the expression should be a very basic one anyway, [...]
They should? In practice they typically are not. For example
assert( $this->indicesAreUpToDate() );
of WikibaseDataModel/DataModel/Claim/Claims.php boils down to nested looping, if I read the code correctly. But (leaving the question aside whether that part is misusing assertions) using complex predicates is totally ok (and even useful) when putting them in strings, as the expression would not get evaluated in production anyways.
- The notion of "bailing out" on "fatal errors" is a misguided remnant
from the days when PHP didn't have exceptions. In my mind, assertions should just throw an (usually unhandled) exception
We could get that by adapting assert_options ...
, like Java's AssertionError.
That comparison is ill suited. Java's AssertionError is a java.lang.Error and not java.lang.Exception. And thereby it's clear what the Java world thinks about catching assertion failures [1]:
An Error [...] indicates serious problems that a reasonable application should not try to catch.
But then again… maybe that was what you meant by “usually unhandled” anyways.
I don't really see how the resulting boiler plate would be cleaner or safer:
if ( $foo > $bar ) { throw new OMGWTFError(); }
Totally! It looks even less clean to me, as after such guards only the negated condition holds. So (when not misusing them) asserts are a way to reduce complexity of reading code.
Best regards, Christian
[1] http://docs.oracle.com/javase/7/docs/api/java/lang/Error.html but this intepretation stands since around Java 1.4 and has proven useful.
On 31/07/13 18:36, Daniel Kinzler wrote:
Assertions are things that should *always* be true.
In my mind, assertions should just throw an (usually unhandled) exception, like Java's AssertionError.
Indeed. In C, assert() will abort the program if it is enabled, which is hard to miss. It is not comparable to the PHP assert() function.
I don't really see how the resulting boiler plate would be cleaner or safer:
if ( $foo > $bar ) { throw new OMGWTFError(); }
The reasons I don't like assert() are:
1. It doesn't throw an exception 2. It acts like eval()
We could have a library of PHPUnit-style assertion functions which throw exceptions and don't act like eval(), I would be fine with that. Maybe MWAssert::greaterThan( $foo, $bar ) or something.
-- Tim Starling
On Wed, Jul 31, 2013 at 7:42 AM, Tim Starling tstarling@wikimedia.orgwrote:
Indeed. In C, assert() will abort the program if it is enabled, which is hard to miss. It is not comparable to the PHP assert() function.
...except PHP's assert() *also* aborts the program if enabled. What am I missing here?
The reasons I don't like assert() are:
- It doesn't throw an exception
- It acts like eval()
We could have a library of PHPUnit-style assertion functions which throw exceptions and don't act like eval(), I would be fine with that. Maybe MWAssert::greaterThan( $foo, $bar ) or something.
1. It's fairly trivial to use assert_options() to make assertions throw exceptions if you really wanted to while developing. 2. Except it's not. Again, you're welcome to give an example where code provided as a string in an assertion is not exactly the same as having the code hardcoded.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2016 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
$_GET["foo"] = 'include( "evil_file.php" )'; assert( '$_GET["foo"] == "fluffy bunny rabbit"' ); // This is fine assert( "$_GET['foo'] == 'fluffy bunny rabbit'" ); // But this is not
Deliberately using a function which reduces the security of your application to relying on everyone choosing the correct type of quotes is definitely asking for trouble.
--HM
On 31 July 2013 13:19, Tyler Romeo tylerromeo@gmail.com wrote:
On Wed, Jul 31, 2013 at 7:42 AM, Tim Starling <tstarling@wikimedia.org
wrote:
Indeed. In C, assert() will abort the program if it is enabled, which is hard to miss. It is not comparable to the PHP assert() function.
...except PHP's assert() *also* aborts the program if enabled. What am I missing here?
The reasons I don't like assert() are:
- It doesn't throw an exception
- It acts like eval()
We could have a library of PHPUnit-style assertion functions which throw exceptions and don't act like eval(), I would be fine with that. Maybe MWAssert::greaterThan( $foo, $bar ) or something.
- It's fairly trivial to use assert_options() to make assertions throw
exceptions if you really wanted to while developing. 2. Except it's not. Again, you're welcome to give an example where code provided as a string in an assertion is not exactly the same as having the code hardcoded.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2016 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Wed, Jul 31, 2013 at 8:38 AM, Happy Melon happy.melon.wiki@gmail.comwrote:
Deliberately using a function which reduces the security of your application to relying on everyone choosing the correct type of quotes is definitely asking for trouble.
I don't see how this is an issue. htmlspecialchars() can cause an XSS vulnerability if you pass it the wrong ENT_ constant. Should we just stop using htmlspecialchars() in case developers pass the wrong constant?
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2016 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On 31 July 2013 15:01, Tyler Romeo tylerromeo@gmail.com wrote:
On Wed, Jul 31, 2013 at 8:38 AM, Happy Melon <happy.melon.wiki@gmail.com
wrote:
Deliberately using a function which reduces the security of your application to relying on everyone choosing the correct type of quotes is definitely asking for trouble.
I don't see how this is an issue. htmlspecialchars() can cause an XSS vulnerability if you pass it the wrong ENT_ constant. Should we just stop using htmlspecialchars() in case developers pass the wrong constant?
Yes, IMO, it should be abstracted away with a carefully-written wrapper function that bridges the semantic gap between "I want to do some character conversions" and "I want to make this text safe to echo to the browser", but that's just the point. Of course there are plenty of language features you can point to that open up pitfalls; each one having its own severity and ease-of-discovery. htmlspecialchars() has a medium severity and very easy discovery, and it's a problem that's easy to eliminate by abstracting the call to ensure it's always given the proper arguments. My example was to disprove your point that assert() with string arguments is not as bad as eval(); it is, for exactly the same reasons. Of course it's possible to use eval() safely, just like any other construct, but general consensus is that eval()'s security holes are severe enough and difficult-to-spot enough to warrant strongly discouraging its use, and there is no reason not to treat assert()-with-string-args the same way.
--HM
On Wed, Jul 31, 2013 at 10:24 AM, Happy Melon happy.melon.wiki@gmail.comwrote:
Yes, IMO, it should be abstracted away with a carefully-written wrapper function that bridges the semantic gap between "I want to do some character conversions" and "I want to make this text safe to echo to the browser", but that's just the point. Of course there are plenty of language features you can point to that open up pitfalls; each one having its own severity and ease-of-discovery. htmlspecialchars() has a medium severity and very easy discovery, and it's a problem that's easy to eliminate by abstracting the call to ensure it's always given the proper arguments. My example was to disprove your point that assert() with string arguments is not as bad as eval(); it is, for exactly the same reasons. Of course it's possible to use eval() safely, just like any other construct, but general consensus is that eval()'s security holes are severe enough and difficult-to-spot enough to warrant strongly discouraging its use, and there is no reason not to treat assert()-with-string-args the same way.
Then I guess I just have more faith in our code review. Nonetheless, assert() provides an important functionality in being able to allow code checks that do not incur a performance penalty in a production environment.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2016 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On 31/07/13 22:19, Tyler Romeo wrote:
On Wed, Jul 31, 2013 at 7:42 AM, Tim Starling tstarling@wikimedia.orgwrote:
Indeed. In C, assert() will abort the program if it is enabled, which is hard to miss. It is not comparable to the PHP assert() function.
...except PHP's assert() *also* aborts the program if enabled. What am I missing here?
The php.ini option assert.bail is 0 by default.
-- Tim Starling
On Wed, Jul 31, 2013 at 7:28 PM, Tim Starling tstarling@wikimedia.orgwrote:
The php.ini option assert.bail is 0 by default.
So? It's the same way in Java. You have to turn on assertions. It's kind of natural to assume that if assertions are off the won't cause fatal errors.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2016 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On 01/08/13 10:05, Tyler Romeo wrote:
On Wed, Jul 31, 2013 at 7:28 PM, Tim Starling tstarling@wikimedia.orgwrote:
The php.ini option assert.bail is 0 by default.
So? It's the same way in Java. You have to turn on assertions. It's kind of natural to assume that if assertions are off the won't cause fatal errors.
It shouldn't be possible to switch them off, and they shouldn't be off by default. I covered this in the 2012 thread. If the error is serious and unexpected, and likely to cause undesirable behaviour, then it should throw an exception.
Maybe we should just unconditionally call assert_options(ASSERT_CALLBACK, ...) from Setup.php and have the callback function throw an exception. But that would still leave the problem of acting like eval(), that's not configurable.
-- Tim Starling
On Wed, Jul 31, 2013 at 10:47 PM, Tim Starling tstarling@wikimedia.orgwrote:
If the error is serious and unexpected, and likely to cause undesirable behaviour
If this is the case, then you don't use assertions. You would use assertions for things that don't have major side effects on the program, but generally are logically unexpected. You use assertions for things that will only break during development. It's not like the designers of C and Java just blindly put in a way to disable assertions but not exceptions.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2016 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On 01/08/13 15:49, Tyler Romeo wrote:
On Wed, Jul 31, 2013 at 10:47 PM, Tim Starling tstarling@wikimedia.orgwrote:
If the error is serious and unexpected, and likely to cause undesirable behaviour
If this is the case, then you don't use assertions. You would use assertions for things that don't have major side effects on the program, but generally are logically unexpected. You use assertions for things that will only break during development.
So it should only be used for things that are mathematically provable to not occur, but don't have any serious effect on the program if they do occur? I think that leaves a pretty small and uninteresting category. Maybe we have a different definition of serious.
It's not like the designers of C and Java just blindly put in a way to disable assertions but not exceptions.
The designers of C and Java believed that:
* The overhead of assertion checking is significant * Development ends with product release
Neither of these assumptions are true for MediaWiki. Development continues indefinitely, deployment occurs at least once a week, and debugging is often done on the production codebase. So all sorts of errors which "should" only happen during development do in fact happen in production.
Even the tarball releases are often debugged after they are deployed to external websites.
-- Tim Starling
wikitech-l@lists.wikimedia.org