lkml.org 
[lkml]   [2001]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Subject[BUG] Bad #define, nonportable C, missing {}
Date
Hi,

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/

\
 
 \ /
  Last update: 2005-03-22 13:13    [W:0.184 / U:0.332 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site