GNOME Bugzilla – Bug 673191
glib/gchecksum.c warning: dereferencing type-punned pointer...
Last modified: 2012-04-05 17:29:27 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);
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.
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.
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.
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
(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.
Created attachment 211302 [details] [review] gchecksum: fix strict-aliasing warnings in the MD5 code
Review of attachment 211302 [details] [review]: Looks ok to me
Attachment 211302 [details] pushed as a81a062 - gchecksum: fix strict-aliasing warnings in the MD5 code