Re: Bug in digest_md5_parse_challenge

On Sat, Aug 16, 2008 at 8:42 PM, Paul Lesniewski <[email protected]> wrote:
> On Sat, Aug 16, 2008 at 8:38 PM, Paul Lesniewski <[email protected]> wrote:
>> On Sat, Aug 16, 2008 at 8:00 PM, Pablo Álvarez de Sotomayor Posadillo
>> <[email protected]> wrote:
>>>
>>>    Paul> If base64_decode() returns FALSE, then !== should catch it.
>>>    Paul> Can you explain why !== does not work?
>>>
>>> Well, you are right, that solve the bug, but it still remain another
>>> bug, and I discovered it now, when I've check why with the !=
>>> condition turn off the warnings and with !== still remain. The new bug
>>> is produced because the while loops once more than the neccesary, that
>>> is, in each while loop the function substract one part of the
>>> $challenge string, so when the string is empty it shows a message
>>> warning about that (the exact message is " Uninitialized string
>>> offset: 0"). The program pass the !== condition if the variables are
>>> not equals or not of the same type, so, in the case described, a
>>> boolean type is diferent from a string type, passing the condition. In
>>> the other hand, with the != condition your are doing an underlying
>>> cast and the while stops at the correct loop. Remember that an empty
>>> string ("") is considered as FALSE in a cast conversion. So, finally,
>>> or you put one != condition or you put two !== condition (one with the
>>> boolean comparision and another one with the string comparision), but
>>> in the first case it's recommended a comment explaining that
>>> comparision.
>>
>> The reason I chose !== was because with stable code, we need to treat
>> the rest of the code as a black box and touch as little as we can with
>> any changes, and the !== means it will only test for a single
>> condition (the one initially identified, where base64_decode() returns
>> specifically FALSE), whereas != can catch any other value that can be
>> evaluated as FALSE such as 0, "", array(), and "0".  It is not good to
>> make assumptions about the meaning of those other possible values.
>>
>> So, if we continue on that track, we'd have to add another
>> type-specific check, but I'm now going to step away from the black box
>> approach and look inside the loop, where we can see that it is clear
>> that any of the other types that can be cast as boolean FALSE are not
>> expected therein.  Therefore, the better solution is (and please, if
>> you don't mind, test this and let us know if it works OK):
>>
>>    while (!empty($challenge)) {
>
> Additionally, if base64_decode() returns FALSE, we need to add one
> more line ABOVE the while statement so that the return value is
> initialized (otherwise, it should generate a PHP notice):
>
> $parsed = array();

Although that would also be expected to generate more notices in
digest_md5_response(), and although technically there should be some
more robust error checking here, I think the overall assumption is
that if you get that far, you have bigger problems (this functionality
not configured correctly on the server).

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
-----
squirrelmail-devel mailing list
Posting guidelines: http://squirrelmail.org/postingguidelines
List address: [email protected]
List archives: http://news.gmane.org/gmane.mail.squirrelmail.devel
List info (subscribe/unsubscribe/change options): https://lists.sourceforge.net/lists/listinfo/squirrelmail-devel