Upon random browsing in the kernel tree I noticed in accel.c:
*a++ = byte_rev[*a]
which isn't 100% correct C AFAIK. At least Stroustrup in his C++ book
warns that this kind of code has to be avoided.
Wrote a script to catch similar things all over the tree (attached).
Found some buglets. Here they are:
drivers/message/i2o/i2o_config.c:#define MODINC(x,y) (x = x++ % y)
---------------------------------------------------
Bad code style. Bad name (sounds like 'module inc').
I can't even tell from this define what the hell it is trying to do:
x++ will return unchanged x, then we obtain (x mod y),
then we store it into x... and why x++ then??!
Alan, seems like you can help here...
drivers/isdn/isdn_audio.c: *buff++ = table[*(unsigned char *)buff];
drivers/video/riva/accel.c: *a++ = byte_rev[*a];
drivers/video/riva/accel.c:/* *a++ = byte_rev[*a];
drivers/video/riva/accel.c: *a++ = byte_rev[*a];*/
drivers/usb/se401.c:
*frame++=(((*frame^255)*(*frame^255))/255)^255;
arch/mips/lib/tinycon.c: *(caddr++) = *(caddr + size_x);
arch/mips/lib/tinycon.c: *(caddr++) = (*caddr & 0xff00) | (unsigned short)
' ';
(btw, tinycon.c seriously needs Lindenting)
------------------------------------------------------------------
Undefined behavior by C std: inc/dec may happen before dereference.
Probably GCC is doing inc after right side eval, but standards say nothing
about it AFAIK. Move ++ out of the statement to be safe:
*a++ = byte_rev[*a]; => *a = byte_rev[*a]; a++;
Patch is attached.
drivers/block/paride/pf.c: if (l==0x20) j--; targ[j]=0;
drivers/block/paride/pg.c: if (l==0x20) j--; targ[j]=0;
drivers/block/paride/pt.c: if (l==0x20) j--; targ[j]=0;
(these files need Lindenting too)
----------
Missing {}
Either a bug or a very bad style (so bad that I can even imagine
that it is NOT a bug). Please double check before applying the patch!
-- vda======= bad_c.diff ======= diff -u --recursive linux-2.4.13-old/arch/mips/lib/tinycon.c linux-2.4.13-new/arch/mips/lib/tinycon.c --- linux-2.4.13-old/arch/mips/lib/tinycon.c Thu Jun 26 17:33:37 1997 +++ linux-2.4.13-new/arch/mips/lib/tinycon.c Wed Nov 21 00:54:05 2001 @@ -83,14 +83,18 @@ register int i; caddr = vram_addr; - for(i=0; i<size_x * (size_y-1); i++) - *(caddr++) = *(caddr + size_x); + for(i=0; i<size_x * (size_y-1); i++) { + *caddr = *(caddr + size_x); + caddr++; + } /* blank last line */ caddr = vram_addr + (size_x * (size_y-1)); - for(i=0; i<size_x; i++) - *(caddr++) = (*caddr & 0xff00) | (unsigned short) ' '; + for(i=0; i<size_x; i++) { + *caddr = (*caddr & 0xff00) | (unsigned short) ' '; + caddr++; + } } void print_string(const unsigned char *str) diff -u --recursive linux-2.4.13-old/drivers/isdn/isdn_audio.c linux-2.4.13-new/drivers/isdn/isdn_audio.c --- linux-2.4.13-old/drivers/isdn/isdn_audio.c Sun Sep 30 17:26:06 2001 +++ linux-2.4.13-new/drivers/isdn/isdn_audio.c Wed Nov 21 00:49:54 2001 @@ -227,8 +227,10 @@ : "0"((long) table), "1"(n), "2"((long) buff), "3"((long) buff) : "memory", "ax"); #else - while (n--) - *buff++ = table[*(unsigned char *)buff]; + while (n--) { + *buff = table[*(unsigned char *)buff]; + buff++; + } #endif } diff -u --recursive linux-2.4.13-old/drivers/usb/se401.c linux-2.4.13-new/drivers/usb/se401.c --- linux-2.4.13-old/drivers/usb/se401.c Fri Sep 14 19:27:10 2001 +++ linux-2.4.13-new/drivers/usb/se401.c Wed Nov 21 00:52:46 2001 @@ -738,7 +738,8 @@ static inline void enhance_picture(unsigned char *frame, int len) { while (len--) { - *frame++=(((*frame^255)*(*frame^255))/255)^255; + *frame = (((*frame^255)*(*frame^255))/255) ^ 255; + frame++; } } diff -u --recursive linux-2.4.13-old/drivers/video/riva/accel.c linux-2.4.13-new/drivers/video/riva/accel.c --- linux-2.4.13-old/drivers/video/riva/accel.c Mon Oct 15 18:47:13 2001 +++ linux-2.4.13-new/drivers/video/riva/accel.c Wed Nov 21 00:50:32 2001 @@ -108,9 +108,9 @@ static inline void fbcon_reverse_order(u32 *l) { u8 *a = (u8 *)l; - *a++ = byte_rev[*a]; -/* *a++ = byte_rev[*a]; - *a++ = byte_rev[*a];*/ + *a = byte_rev[*a]; a++; +/* *a = byte_rev[*a]; a++; + *a = byte_rev[*a]; a++; */ *a = byte_rev[*a]; } ======= paride.diff ======= diff -u --recursive linux-2.4.13-old/drivers/block/paride/pf.c linux-2.4.13-new/drivers/block/paride/pf.c --- linux-2.4.13-old/drivers/block/paride/pf.c Thu Nov 8 23:42:11 2001 +++ linux-2.4.13-new/drivers/block/paride/pf.c Wed Nov 21 01:00:16 2001 @@ -737,10 +737,13 @@ { int j,k,l; j=0; l=0; - for (k=0;k<len;k++) - if((buf[k+offs]!=0x20)||(buf[k+offs]!=l)) - l=targ[j++]=buf[k+offs]; - if (l==0x20) j--; targ[j]=0; + for(k=0;k<len;k++) + if((buf[k+offs]!=0x20) || (buf[k+offs]!=l)) + l = targ[j++] = buf[k+offs]; + if(l==0x20) { + j--; + targ[j]=0; + } } static int xl( char *buf, int offs ) diff -u --recursive linux-2.4.13-old/drivers/block/paride/pg.c linux-2.4.13-new/drivers/block/paride/pg.c --- linux-2.4.13-old/drivers/block/paride/pg.c Thu Nov 8 23:42:11 2001 +++ linux-2.4.13-new/drivers/block/paride/pg.c Wed Nov 21 01:00:25 2001 @@ -488,10 +488,13 @@ { int j,k,l; j=0; l=0; - for (k=0;k<len;k++) - if((buf[k+offs]!=0x20)||(buf[k+offs]!=l)) - l=targ[j++]=buf[k+offs]; - if (l==0x20) j--; targ[j]=0; + for(k=0;k<len;k++) + if((buf[k+offs]!=0x20) || (buf[k+offs]!=l)) + l = targ[j++] = buf[k+offs]; + if(l==0x20) { + j--; + targ[j]=0; + } } static int pg_identify( int unit, int log ) diff -u --recursive linux-2.4.13-old/drivers/block/paride/pt.c linux-2.4.13-new/drivers/block/paride/pt.c --- linux-2.4.13-old/drivers/block/paride/pt.c Thu Nov 8 23:42:11 2001 +++ linux-2.4.13-new/drivers/block/paride/pt.c Wed Nov 21 00:59:44 2001 @@ -574,10 +574,13 @@ { int j,k,l; j=0; l=0; - for (k=0;k<len;k++) - if((buf[k+offs]!=0x20)||(buf[k+offs]!=l)) - l=targ[j++]=buf[k+offs]; - if (l==0x20) j--; targ[j]=0; + for(k=0;k<len;k++) + if((buf[k+offs]!=0x20) || (buf[k+offs]!=l)) + l = targ[j++] = buf[k+offs]; + if(l==0x20) { + j--; + targ[j]=0; + } } static int xn( char *buf, int offs, int size ) ======= bughunter.sh with some lines wrapped by kmail :-( ======= #!/bin/sh function do_grep() { pattern="$1" shift for i in $@; do if test -d "$i"; then do_grep $pattern $i/* else #echo "$i" #echo grep -E $pattern "$i" /dev/null grep -E $pattern "$i" /dev/null fi done }
# skip lines from doc dir and with for loops (way too many false alarms)
# *var++ ... *var do_grep '[^0-9A-Za-z_]([A-Za-z_][0-9A-Za-z_]*)\+\+.*[^0-9A-Za-z_]\1[^0-9A-Za-z_]' * 2>/dev/null \ | grep -E '(for)|(Documentation)' -v >!vpp_v 2>&1 &
# *var-- ... *var do_grep '[^0-9A-Za-z_]([A-Za-z_][0-9A-Za-z_]*)--.*[^0-9A-Za-z_]\1[^0-9A-Za-z_]' * 2>/dev/null \ | grep -E '(for)|(Documentation)' -v >!vmm_v 2>&1 &
# *var ... *var++ do_grep '[^0-9A-Za-z_]([A-Za-z_][0-9A-Za-z_]*)[^0-9A-Za-z_].*[^0-9A-Za-z_]\1\+\+' * 2>/dev/null \ | grep -E '(for)|(Documentation)' -v >!v_vpp 2>&1 &
# *var ... *var-- do_grep '[^0-9A-Za-z_]([A-Za-z_][0-9A-Za-z_]*)[^0-9A-Za-z_].*[^0-9A-Za-z_]\1--' * 2>/dev/null \ | grep -E '(for)|(Documentation)' -v >!v_vmm 2>&1 & - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/