After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 673191 - glib/gchecksum.c warning: dereferencing type-punned pointer...
glib/gchecksum.c warning: dereferencing type-punned pointer...
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
2.30.x
Other Linux
: Normal minor
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2012-03-30 15:27 UTC by ncahill_alt
Modified: 2012-04-05 17:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gchecksum: fix strict-aliasing warnings in the MD5 code (3.64 KB, patch)
2012-04-04 14:41 UTC, Dan Winship
committed Details | Review

Description ncahill_alt 2012-03-30 15:27:28 UTC
I get this message on glib 2.30.3, for glib/gchecksum.c lines 411 and 412.  These are the lines:

((guint32 *) md5->data)[14] = md5->bits[0];
((guint32 *) md5->data)[15] = md5->bits[1];

What happens is, md5->data is written to through a (guint32 *), then read as a (guchar *).

Below is the fix.

Thank you.
Neil.


--- gchecksum.c 2012-03-11 21:43:28.000000000 +0000
+++ /var/altsrc/glib-2.30.2/glib/gchecksum.c    2012-03-30 15:54:26.000000000 +0100
@@ -408,8 +408,11 @@
   md5_byte_reverse (md5->data, 14);
 
   /* Append length in bits and transform */
-  ((guint32 *) md5->data)[14] = md5->bits[0];
-  ((guint32 *) md5->data)[15] = md5->bits[1];
+  /* BUG 1234: the following violates strict aliasing
+   * ((guint32 *) md5->data)[14] = md5->bits[0];
+   * ((guint32 *) md5->data)[15] = md5->bits[1]; */
+  memcpy(md5->data+(14*sizeof(guint32)), &(md5->bits[0]), sizeof(guint32));
+  memcpy(md5->data+(15*sizeof(guint32)), &(md5->bits[1]), sizeof(guint32));
 
   md5_transform (md5->buf, (guint32 *) md5->data);
   md5_byte_reverse ((guchar *) md5->buf, 4);
Comment 1 André Klapper 2012-03-30 15:37:55 UTC
Thanks for the report and the patch!
Attaching patches as attachments (with the checkbox "patch" enabled) is welcome, so they can be explicitly queried for in Bugzilla's search.
Comment 2 Colin Walters 2012-03-30 15:56:10 UTC
Kind of makes my stomach twist that we have strict-aliasing bugs in our crypto code =/

Quick background on strict-aliasing: The compiler is totally free here to load in stale data, which would result in an incorrect checksum.  In practice it's unlikely to do so, but it's possible.  Major projects like the linux kernel simply compile their entire source with -fno-strict-aliasing.

Someone should see if there's newer/patched versions of md5 around.  There probably are.
Comment 3 Colin Walters 2012-03-30 15:59:37 UTC
As far as the patch, beyond what André said, please refer to: https://live.gnome.org/GnomeLove/SubmittingPatches

* Git-formatted patches with a rationale are best.
* There's no reason to leave the old commented out code, just replace it - git will retain history.
Comment 4 johndoe32102002 2012-04-04 01:02:52 UTC
I just tried git and still have this error:

CC     gchecksum.lo
gchecksum.c: In function 'md5_sum_close':
gchecksum.c:412:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
gchecksum.c:413:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  CC     gconvert.lo
gconvert.c:66:2: error: #error GNU libiconv not in use but included iconv.h is from libiconv
Comment 5 Dan Winship 2012-04-04 14:41:01 UTC
(In reply to comment #2)
> Kind of makes my stomach twist that we have strict-aliasing bugs in our crypto
> code =/

It's not really that kind of crypto bug; if GChecksum fails, you just stop being able to interoperate. (eg, you think other people's data is corrupted and they think your data is corrupted, because your checksums don't match).

In the worst case, if GChecksum did something that made it semi-predictably ignore some of the input data or semi-predictably mis-generate some of the output data, then there would be a security hole when both the producer and the consumer were using GChecksum, since you could more easily find collisions. But it's unlikely that that could happen without glib/tests/checksum failing.

> Someone should see if there's newer/patched versions of md5 around.  There
> probably are.

There's no newer/patched version of the original 20-year-old public domain implementation, and everyone who has used that code has tweaked it slightly, so we can't trivially just take someone else's patch.
Comment 6 Dan Winship 2012-04-04 14:41:18 UTC
Created attachment 211302 [details] [review]
gchecksum: fix strict-aliasing warnings in the MD5 code
Comment 7 Matthias Clasen 2012-04-04 23:22:48 UTC
Review of attachment 211302 [details] [review]:

Looks ok to me
Comment 8 Dan Winship 2012-04-05 17:29:23 UTC
Attachment 211302 [details] pushed as a81a062 - gchecksum: fix strict-aliasing warnings in the MD5 code