libcrypt/md5.c: a few statics can be auto variables; strlen("const") is silly

Will Newton will.newton at gmail.com
Thu Jun 12 10:14:07 UTC 2008


On Thu, Jun 12, 2008 at 8:08 AM, Denys Vlasenko
<vda.linux at googlemail.com> wrote:
> On Wednesday 11 June 2008 17:32, Will Newton wrote:
>> >> This looks like it should be safe. Although it might be worth finding
>> >> the original author and asking why it is done this way, following the
>> >> Debian openssl incident. ;-)
>> >
>> > openssl incident? Did I miss some fun? :)
>>
>> This is a fairly good summary:
>>
>> http://research.swtch.com/2008/05/lessons-from-debianopenssl-fiasco.html
>>
>> The general lesson being: be very careful when modifying crypto code.
>
> I'd place the blame on the author.  He wrote unreadable code.
> Unfortunately, it's far too common.

Indeed.

>> btw I tested the strlen change and this code:
>>
>> #include <string.h>
>>
>> static const unsigned char __md5__magic[] = "$1$";
>>
>> int foo(void)
>> {
>>   return strlen(__md5__magic);
>> }
>>
>> compiles to this:
>>
>> Disassembly of section .text:
>>
>> 00000000 <foo>:
>>    0:   55                      push   %ebp
>>    1:   89 e5                   mov    %esp,%ebp
>>    3:   b8 03 00 00 00          mov    $0x3,%eax
>>    8:   5d                      pop    %ebp
>>    9:   c3                      ret
>
>
> Don't place too much faith in gcc, that would be a mistake.
> Piling up a bit more code around, and it will fail to optimize
> it completely.
>
> This is the patch wich ONLY replaces strlen("const") with
> sizeof("const")-1. Let's check assembly, do we see strlen
> in original version?

What compiler flags were used? It's possible to disable this
optimization in gcc.

Maybe replace strlen with __builtin_strlen?



More information about the uClibc mailing list