Re: [libiberty] problem with unaligned pointers when using md5_process_bytes

Re: [libiberty] problem with unaligned pointers when using md5_process_bytes

[Steve Ellcey]

On Thu, 2011-09-15 at 14:00 +0200, piervit@pvittet.com wrote:
> Hello,
> 
> I request your help on the following problem:
> Recently, I used the libiberty md5_process_bytes function (for some 
> MELT code) and got a problem with it:
> 
> I don't know why but on some systems (debian i686) I got some 
> unaligned pointers (only when using MELT, I could not reproduce 
> elsewhere, so that might come from me or from MELT). Normally, even 
> with unaligned pointers, this should works as the function 
> md5_process_bytes handles this case:
> 
> #if !_STRING_ARCH_unaligned
> /* To check alignment gcc has an appropriate operator.  Other
>    compilers don't.  */
> # if __GNUC__ >= 2
> #  define UNALIGNED_P(p) (((md5_uintptr) p) % __alignof__ (md5_uint32) 
> != 0)
> # else
> #  define UNALIGNED_P(p) (((md5_uintptr) p) % sizeof (md5_uint32) != 
> 0)
> # endif
> 	len --;


Where did this ' len --;' come from, it is not in my code.  Maybe that
is where you are getting unaligned.


>       if (UNALIGNED_P (buffer))
>         while (len > 64)
>           {
> 			memcpy (ctx->buffer, buffer, 64);
>             md5_process_block (ctx->buffer, 64, ctx);
>             buffer = (const char *) buffer + 64;
>             len -= 64;
>           }
>       else
> #endif
>       md5_process_block (buffer, len & ~63, ctx);
>       buffer = (const void *) ((const char *) buffer + (len & ~63));
>       len &= 63;
>     }
> 
> However the handling looks quite strange to me, if UNALIGNED_P 
> (buffer) (as this is the case for me) is true, we normally travel the 
> while, but then, we execute a new time the line "buffer = (const void 
> *) ((const char *) buffer + (len & ~63));", which appear to shift the 
> buffer a new time (for more detail, you can have a look at my given gdb 
> trace).
> 
> I was able to fully compile GCC MELT branch by only adding a pair of 
> braces as you can see on the given diff file. I guess this is a bug in 
> libiberty (even if I can't understand why my pointer is not aligned).
> 
> So can you confirm (or no) that there is a bug in this function (I can 
> post to gcc-patches if you think it is a bug), and give me informations 
> about how to explain the fact that I get unaligned pointers.
> 
> Thanks a lot for your help!
> 
> Pierre Vittet

It certainly seems to me that those last three lines should be in a
single block so they are all controlled by the else clause.  Though I
also wonder if anyone ever defines _STRING_ARCH_unaligned.  It doesn't
happen anywhere in configure so I guess the only way it would get
defined is by a user via CFLAGS.  I wonder if this #if is even needed,
then we could make the code cleaner by not having an partially ifdef'ed
if/else statement.

Steve Ellcey
sje@cup.hp.com

Re: [libiberty] problem with unaligned pointers when using md5_process_bytes

[Pierre Vittet]

>>
>> #if !_STRING_ARCH_unaligned
>> /* To check alignment gcc has an appropriate operator.  Other
>>    compilers don't.  */
>> # if __GNUC__ >= 2
>> #  define UNALIGNED_P(p) (((md5_uintptr) p) % __alignof__ (md5_uint32) 
>> != 0)
>> # else
>> #  define UNALIGNED_P(p) (((md5_uintptr) p) % sizeof (md5_uint32) != 
>> 0)
>> # endif
>> 	len --;
> 
> 
> Where did this ' len --;' come from, it is not in my code.  Maybe that
> is where you are getting unaligned.
Sorry, it is an error from me when I copied, as I could not debug the
macro, I added a line a len ++ in the conditionnal preprocessor branch
to see what was defined, and then a len -- to get normal size back, I
forgot to clean the len --. That does not change the problem.
> 
> 
>>       if (UNALIGNED_P (buffer))
>>         while (len > 64)
>>           {
>> 			memcpy (ctx->buffer, buffer, 64);
>>             md5_process_block (ctx->buffer, 64, ctx);
>>             buffer = (const char *) buffer + 64;
>>             len -= 64;
>>           }
>>       else
>> #endif
>>       md5_process_block (buffer, len & ~63, ctx);
>>       buffer = (const void *) ((const char *) buffer + (len & ~63));
>>       len &= 63;
>>     }

>> I was able to fully compile GCC MELT branch by only adding a pair of 
>> braces as you can see on the given diff file. I guess this is a bug in 
>> libiberty (even if I can't understand why my pointer is not aligned).

> 
> It certainly seems to me that those last three lines should be in a
> single block so they are all controlled by the else clause.  Though I
> also wonder if anyone ever defines _STRING_ARCH_unaligned.  It doesn't
> happen anywhere in configure so I guess the only way it would get
> defined is by a user via CFLAGS.  I wonder if this #if is even needed,
> then we could make the code cleaner by not having an partially ifdef'ed
> if/else statement.
Thanks, I will propose a patch on gcc-patches.

Pierre Vittet
> 
> Steve Ellcey
> sje@cup.hp.com
> 
> 
>