Re: misbehaviour with md5_process_bytes and maybe in optimization

Re: misbehaviour with md5_process_bytes and maybe in optimization

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 2504 bytes --]

Pierre Vittet <piervit@pvittet.com> writes:

> Thanks for your interest,
>
> I just checked revision 179127 of GCC. Last revision is 177700, it has
> not been change for 6 weeks.
>
> My file is the same as this one:
> http://gcc.gnu.org/viewcvs/trunk/libiberty/md5.c?revision=177700&view=markup
>
> in libiberty/md5.c, function md5_process_bytes start line 203.
>
> On 23/09/2011 17:13, Ian Lance Taylor wrote:
>> Pierre Vittet <piervit@pvittet.com> writes:
>> 
>>> The bug appears when:
>>> 	1) We use libiberty compiled with -O0
>>> 	2) We first call md5_process_bytes with a less than 64 bits buffer (we
>>> call his size len1).
>>> 	3) We make a new call of md5_process_bytes with a buffer which has a
>>> size len2 such as:
>>> 	len2 > 127 + 65 (so test in line 228 of md5.C will be true)
> line 228 is the following:    if (len > 64)
>>> 	128 -len1 != Mulint with Mulint %  __alignof__ (md5_uint32) != 0 (so
>>> condition on line 238 is true)
> line 238 is the following: if (UNALIGNED_P (buffer))
>>> 	len2 - (128 - len1) = Mul64 and Mul64 such as Mul %64=0 (so the loop of
>>> line 239 is broken with len = 64, this leads to the bug as, line 249,
>>> (len & ~63) = 64 and we shift the buffer without processing the data).
>
> line 239 is the following: while (len > 64)
> line 249: buffer = (const void *) ((const char *) buffer + (len & ~63));
>> 
>> The line numbers you mention do not correspond to any version of
>> libiberty/md5.c that I can see.  Can you list the exact line for each
>> line number you mention, so that your explanation is easier to follow?
>> Thanks.
>
> I give about the same explanation in the README (which is in the
> attached archive of my previous mail) but I does not use line number but
> direct quote of the code. It mights be more easy to try the plugin with
> gdb but it needs to compile libiberty.a with -O0.

Thanks, I think I have it sorted out now.

It does not happen on x86 glibc-based systems at -O2 because at -O2
<string.h> #defines STRING_ARCH_unaligned, so the problematic code is
not compiled or executed.

The error was introduced by this change:

2005-07-03  Steve Ellcey  <sje@cup.hp.com>

	PR other/13906
	* md5.c (md5_process_bytes): Check alignment.

Thanks for noticing this problem, analyzing it, and reporting it.

I committed this patch to mainline to fix the problem.  Bootstrapped on
x86_64-unknown-linux-gnu.

Ian


2011-09-23  Ian Lance Taylor  <iant@google.com>

	* md5.c (md5_process_bytes): Correct handling of unaligned
	buffer.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 951 bytes --]

Index: md5.c
===================================================================
--- md5.c	(revision 179127)
+++ md5.c	(working copy)
@@ -1,6 +1,6 @@
 /* md5.c - Functions to compute MD5 message digest of files or memory blocks
    according to the definition of MD5 in RFC 1321 from April 1992.
-   Copyright (C) 1995, 1996 Free Software Foundation, Inc.
+   Copyright (C) 1995, 1996, 2011 Free Software Foundation, Inc.
 
    NOTE: This source is derived from an old version taken from the GNU C
    Library (glibc).
@@ -245,9 +245,11 @@ md5_process_bytes (const void *buffer, s
           }
       else
 #endif
-      md5_process_block (buffer, len & ~63, ctx);
-      buffer = (const void *) ((const char *) buffer + (len & ~63));
-      len &= 63;
+	{
+	  md5_process_block (buffer, len & ~63, ctx);
+	  buffer = (const void *) ((const char *) buffer + (len & ~63));
+	  len &= 63;
+	}
     }
 
   /* Move remaining bytes in internal buffer.  */

Re: misbehaviour with md5_process_bytes and maybe in optimization

[Basile Starynkevitch]

On Fri, 23 Sep 2011 12:19:57 -0700
Ian Lance Taylor <iant@google.com> wrote:

> I committed this patch to mainline to fix the problem.  Bootstrapped on
> x86_64-unknown-linux-gnu.
> 
> 2011-09-23  Ian Lance Taylor  <iant@google.com>
> 
> 	* md5.c (md5_process_bytes): Correct handling of unaligned
> 	buffer.

This is *exactly* the same patch as
Pierre Vittet proposed in http://gcc.gnu.org/ml/gcc-patches/2011-09/msg00963.html
(but Pierre's patch has not been reviewed).

Perhaps the ChangeLog might also mention Pierre Vittet for that particular patch???

Cheers.
-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mine, sont seulement les miennes} ***

Re: misbehaviour with md5_process_bytes and maybe in optimization

[Ian Lance Taylor]

Basile Starynkevitch <basile@starynkevitch.net> writes:

> On Fri, 23 Sep 2011 12:19:57 -0700
> Ian Lance Taylor <iant@google.com> wrote:
>
>> I committed this patch to mainline to fix the problem.  Bootstrapped on
>> x86_64-unknown-linux-gnu.
>> 
>> 2011-09-23  Ian Lance Taylor  <iant@google.com>
>> 
>> 	* md5.c (md5_process_bytes): Correct handling of unaligned
>> 	buffer.
>
> This is *exactly* the same patch as
> Pierre Vittet proposed in http://gcc.gnu.org/ml/gcc-patches/2011-09/msg00963.html
> (but Pierre's patch has not been reviewed).
>
> Perhaps the ChangeLog might also mention Pierre Vittet for that particular patch???

Sorry, I didn't see his patch.

Sure, go ahead and change libiberty/ChangeLog.

Note that as I explained in my message we should not remove the
_STRING_ARCH_unaligned test.

Ian