[Debburn-devel] [PATCH] fixes for aliasing bugs
Lorenz Minder
lminder at gmx.net
Wed Sep 27 21:34:53 UTC 2006
Hi,
This patch fixes a number of aliasing bugs in the code; for none of
these I could actually observe wrong behaviour with the compilers I
currently use, but it's nonetheless wrong and it may fail on other
compilers, or ones that do more optimization.
Details:
* In cdrecord, use char * for strings, rather than unsigned char *; this
is necessary because, while char and unsigned char can alias, char *
and unsigned char * cannot.
* In mkisofs.h, include <stddef.h> for a correct definition of
offsetof(). (In fact, the other one should possibly be removed.)
Before anyone asks: Yes I've read the comment in write.c I've removed,
and yes, I still think the old way to write it was buggy. And, no, cast
chains do not solve aliasing issues. (However, the fact that the old
writing was very obfuscated suggests that maybe the obvious solution
does not work on some old / weird compiler; some checking might be in
order.) Here's an example for the non-believers, to try with gcc4 -O3:
~$ cat test.c
#include <stdio.h>
int main() {
int a = 1;
*((short *)(void *) &a) += 1;
printf("%d\n", a);
return a;
}
~$ /usr/local/gcc4/bin/gcc -O3 test.c
~$ ./a.out
1
The remaining aliasing-bugs I spotted are in libschily/strtod.c; and
they can be fixed by fetching a newer version of that file in the NetBSD
source tree. I've done that on my copy, but I'm not yet submitting that
because it's a rather messy change and I hope we will soon be able to
instead ditch strtod.c altogether.
Best,
--Lorenz
Index: cdda2wav/cdda2wav.c
===================================================================
--- cdda2wav/cdda2wav.c (revision 351)
+++ cdda2wav/cdda2wav.c (working copy)
@@ -353,14 +355,14 @@
, (unsigned long) global.cddb_id
, Get_MCN()
, Get_ISRC(track)
- , global.creator != NULL ? global.creator : (const unsigned char *)""
+ , global.creator != NULL ? global.creator : ""
, global.trackcreator[track] != NULL ? global.trackcreator[track] :
- (global.creator != NULL ? global.creator : (const unsigned char *)"")
- , global.disctitle != NULL ? global.disctitle : (const unsigned char *)""
+ (global.creator != NULL ? global.creator : "")
+ , global.disctitle != NULL ? global.disctitle : ""
);
fprintf(info_fp,
"Tracktitle=\t'%s'\n"
- , global.tracktitle[track] ? global.tracktitle[track] : (const unsigned char *)""
+ , global.tracktitle[track] ? global.tracktitle[track] : ""
);
fprintf(info_fp, "Tracknumber=\t%u\n"
, track
Index: cdda2wav/global.h
===================================================================
--- cdda2wav/global.h (revision 351)
+++ cdda2wav/global.h (working copy)
@@ -88,11 +88,11 @@
int illleadout_cd;
int reads_illleadout;
unsigned char *cdindex_id;
- unsigned char *creator;
- unsigned char *copyright_message;
- unsigned char *disctitle;
- unsigned char *tracktitle[100];
- unsigned char *trackcreator[100];
+ char *creator;
+ char *copyright_message;
+ char *disctitle;
+ char *tracktitle[100];
+ char *trackcreator[100];
index_list *trackindexlist[100];
int paranoia_selected;
Index: cdda2wav/toc.c
===================================================================
--- cdda2wav/toc.c (revision 351)
+++ cdda2wav/toc.c (working copy)
@@ -900,7 +899,7 @@
int finished = 0;
char *p = inbuff;
int ind = 0;
- char ** target = (char **)&global.creator;
+ char ** target = &global.creator;
do {
while (readbytes > 0) {
@@ -974,12 +973,12 @@
*target = realloc(*target, strlen(*target) + clen - 1);
}
if (*target != NULL) {
- strcat((char *)*target, inbuff+ind+7);
+ strcat(*target, inbuff+ind+7);
}
/* handle part after the delimiter, if present */
if (res != NULL) {
- target = (char **)&global.disctitle;
+ target = &global.disctitle;
/* skip the delimiter */
q += 3;
clen = p - q;
@@ -1556,17 +1555,15 @@
};
static char *
-ascii2html __PR((unsigned char *inp));
+ascii2html __PR((char *inp));
-static char *
-ascii2html(inp)
- unsigned char *inp;
+static char *ascii2html(char *inp)
{
- static unsigned char outline[300];
- unsigned char *outp = outline;
+ static char outline[300];
+ char *outp = outline;
-#define copy_translation(a,b) else if (*inp == (a)) \
-{ strcpy((char *)outp, (b)); outp += sizeof((b))-1; }
+#define copy_translation(a,b) else if (*inp == (char)(a)) \
+{ strcpy(outp, (b)); outp += sizeof((b))-1; }
while (*inp != '\0') {
if (0) ;
@@ -1575,16 +1572,16 @@
copy_translation('<', "<")
copy_translation('>', ">")
copy_translation(160, " ")
- else if (*inp < 192) {
+ else if (((unsigned char)*inp) < 192) {
*outp++ = *inp;
} else {
- strcpy((char *)outp, a2h[*inp-192]);
- outp += strlen(a2h[*inp-192]);
+ strcpy(outp, a2h[((unsigned char)*inp) - 192]);
+ outp += strlen(a2h[((unsigned char)*inp) -192]);
}
inp++;
}
*outp = '\0';
- return (char *) outline;
+ return outline;
}
#undef copy_translation
@@ -1814,13 +1811,12 @@
#endif
}
-static char *quote __PR((unsigned char *string));
+static char *quote __PR((char *string));
-static char *quote(string)
- unsigned char * string;
+static char *quote(char *string)
{
static char result[200];
- unsigned char *p = (unsigned char *)result;
+ char *p = result;
while (*string) {
if (*string == '\'' || *string == '\\') {
@@ -1886,7 +1882,7 @@
}
fprintf( stderr,
- "Album title: '%s'", (void *)global.disctitle != NULL
+ "Album title: '%s'", global.disctitle != NULL
? quote(global.disctitle) : "");
fprintf( stderr, " from '%s'\n", (void *)global.creator != NULL
Index: mkisofs/mkisofs.h
===================================================================
--- mkisofs/mkisofs.h (revision 351)
+++ mkisofs/mkisofs.h (working copy)
@@ -40,6 +40,7 @@
#include <mconfig.h> /* Must be before stdio.h for LARGEFILE support */
+#include <stddef.h>
#include <stdio.h>
#include <statdefs.h>
#include <stdxlib.h>
Index: mkisofs/write.c
===================================================================
--- mkisofs/write.c (revision 351)
+++ mkisofs/write.c (working copy)
@@ -317,19 +317,9 @@
exit(1);
#endif
}
- /*
- * This comment is in hope to prevent silly people from
- * e.g. SuSE (who did not yet learn C but believe that
- * they need to patch other peoples code) from changing the
- * next cast into an illegal lhs cast expression.
- * The cast below is the correct way to handle the problem.
- * The (void *) cast is to avoid a GCC warning like:
- * "warning: dereferencing type-punned pointer will break \
- * strict-aliasing rules"
- * which is wrong this code. (void *) introduces a compatible
- * intermediate type in the cast list.
- */
- count -= got, *(char **)(void *)&buffer += size * got;
+
+ buffer += size * got;
+ count -= got;
}
}
More information about the Debburn-devel
mailing list