[BUG] in drivers/char/joystick/magellan.c

[BUG] in drivers/char/joystick/magellan.c

[Jörn Engel]

Hi!

Without the patch below, the \0 terminating the string is written
anywhere. nibbles[] would be even better, I guess.
Can you check for stupidity on my side?

Jörn

-- 
But this is not to say that the main benefit of Linux and other GPL
software is lower-cost. Control is the main benefit--cost is secondary.
-- Bruce Perens

diff -Naur linux-2.4.21-pre3-ac4/drivers/char/joystick/magellan.c scratch/drivers/char/joystick/magellan.c
--- linux-2.4.21-pre3-ac4/drivers/char/joystick/magellan.c	Thu Sep 13 00:34:06 2001
+++ scratch/drivers/char/joystick/magellan.c	Mon Jan 27 13:49:54 2003
@@ -66,7 +66,7 @@
 
 static int magellan_crunch_nibbles(unsigned char *data, int count)
 {
-	static unsigned char nibbles[16] = "0AB3D56GH9:K<MN?";
+	static unsigned char nibbles[17] = "0AB3D56GH9:K<MN?";
 
 	do {
 		if (data[count] == nibbles[data[count] & 0xf])

Re: [BUG] in drivers/char/joystick/magellan.c

[Vojtech Pavlik]

On Tue, Jan 28, 2003 at 04:53:12PM +0100, Jörn Engel wrote:
> Hi!
> 
> Without the patch below, the \0 terminating the string is written
> anywhere. nibbles[] would be even better, I guess.

Well, the zero isn't used, so it might make sense to use '0', 'A', 'B' ...
... though that's not very nice either.

> Can you check for stupidity on my side?

Can't find any. ;) Patch applied with [].

> 
> Jörn
> 
> -- 
> But this is not to say that the main benefit of Linux and other GPL
> software is lower-cost. Control is the main benefit--cost is secondary.
> -- Bruce Perens
> 
> diff -Naur linux-2.4.21-pre3-ac4/drivers/char/joystick/magellan.c scratch/drivers/char/joystick/magellan.c
> --- linux-2.4.21-pre3-ac4/drivers/char/joystick/magellan.c	Thu Sep 13 00:34:06 2001
> +++ scratch/drivers/char/joystick/magellan.c	Mon Jan 27 13:49:54 2003
> @@ -66,7 +66,7 @@
>  
>  static int magellan_crunch_nibbles(unsigned char *data, int count)
>  {
> -	static unsigned char nibbles[16] = "0AB3D56GH9:K<MN?";
> +	static unsigned char nibbles[17] = "0AB3D56GH9:K<MN?";
>  
>  	do {
>  		if (data[count] == nibbles[data[count] & 0xf])

-- 
Vojtech Pavlik
SuSE Labs

Re: [BUG] in drivers/char/joystick/magellan.c

[Jörn Engel]

On Tue, 28 January 2003 16:57:19 +0100, Vojtech Pavlik wrote:
> On Tue, Jan 28, 2003 at 04:53:12PM +0100, Jörn Engel wrote:
> > 
> > Without the patch below, the \0 terminating the string is written
> > anywhere. nibbles[] would be even better, I guess.
> 
> Well, the zero isn't used, so it might make sense to use '0', 'A', 'B' ...
> ... though that's not very nice either.

And maintenance wins over any byte of memory. :)

> > Can you check for stupidity on my side?
> 
> Can't find any. ;) Patch applied with [].

Cool!

Jörn

-- 
A victorious army first wins and then seeks battle.
-- Sun Tzu

Re: [BUG] in drivers/char/joystick/magellan.c

[Horst von Brand]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]

Vojtech Pavlik <vojtech@suse.cz> said:
> On Tue, Jan 28, 2003 at 04:53:12PM +0100, Jörn Engel wrote:
> > Hi!
> > 
> > Without the patch below, the \0 terminating the string is written
> > anywhere.

No. As the length is explicitly given, C will just fill out that many bytes
from the given string.

> >           nibbles[] would be even better, I guess.
> 
> Well, the zero isn't used, so it might make sense to use '0', 'A', 'B' ...
> ... though that's not very nice either.
> 
> > Can you check for stupidity on my side?
> 
> Can't find any. ;) Patch applied with [].

> > diff -Naur linux-2.4.21-pre3-ac4/drivers/char/joystick/magellan.c scratch/dri
> vers/char/joystick/magellan.c
> > --- linux-2.4.21-pre3-ac4/drivers/char/joystick/magellan.c	Thu Sep 13 00:3
> 4:06 2001
> > +++ scratch/drivers/char/joystick/magellan.c	Mon Jan 27 13:49:54 2003
> > @@ -66,7 +66,7 @@
> >  
> >  static int magellan_crunch_nibbles(unsigned char *data, int count)
> >  {
> > -	static unsigned char nibbles[16] = "0AB3D56GH9:K<MN?";
> > +	static unsigned char nibbles[17] = "0AB3D56GH9:K<MN?";
> >  
> >  	do {
> >  		if (data[count] == nibbles[data[count] & 0xf])

C says only the first 16 bytes get used as initializer, i.e., the '\0' is
(silently) discarded. The patch makes the array grow, for no reason; thus
making its read-only data usage probably 4 or even 16 bytes (padding)
larger.

Sure, it can be argued that this is bad style as nibbles[] really isn't a
string, and should not be initialized like such... but

	static unsigned char nibbles[] = {'0', 'A', ..., '?'};

is just awful.

Please leave it alone, or add a comment like:

 /* nibbles is no string, it is just initialized as such for convenience */
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: [BUG] in drivers/char/joystick/magellan.c

[Martin Mares]

Hi!

> Without the patch below, the \0 terminating the string is written
> anywhere. nibbles[] would be even better, I guess.
> Can you check for stupidity on my side?

As far as I remember, the ANSI C permits initialization of a char array
with a string of the same length and defines that the trailing \0 is
dropped in such cases. However, I cannot quote the right chapter and
verse by heart nor am I sure it's still permitted by C99, so better
check yourself.

				Have a nice fortnight
-- 
Martin `MJ' Mares   <mj@ucw.cz>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
return(EIEIO); /* Here-a-bug, There-a-bug... */

Re: [BUG] in drivers/char/joystick/magellan.c

[Jörn Engel]

On Tue, 28 January 2003 18:57:57 +0100, Martin Mares wrote:
> 
> As far as I remember, the ANSI C permits initialization of a char array
> with a string of the same length and defines that the trailing \0 is
> dropped in such cases. However, I cannot quote the right chapter and
> verse by heart nor am I sure it's still permitted by C99, so better
> check yourself.

This is interesting, but still dangerous, if the code ever gets
changed. You remember what a Mr. Murphy said about having several
possibilities and one of them leading to a disaster? :)

Jörn

-- 
Beware of bugs in the above code; I have only proved it correct, but
not tried it.
-- Donald Knuth

Re: [BUG] in drivers/char/joystick/magellan.c

[Randy.Dunlap]

On Tue, 28 Jan 2003, Martin Mares wrote:

| Hi!
|
| > Without the patch below, the \0 terminating the string is written
| > anywhere. nibbles[] would be even better, I guess.
| > Can you check for stupidity on my side?
|
| As far as I remember, the ANSI C permits initialization of a char array
| with a string of the same length and defines that the trailing \0 is
| dropped in such cases. However, I cannot quote the right chapter and
| verse by heart nor am I sure it's still permitted by C99, so better
| check yourself.

The closest that I find in a quick scan is:

ANSI/ISO/IEC 9899-1999, page 126, section 6.7.8, constraint 14:

An array of character type may be initialized by a character string
literal, optionally enclosed in braces. Successive characters of the
character string literal (including the terminating null character if
there is room or if the array is of unknown size) initialize the
elements of the array.

-- 
~Randy

Re: [BUG] in drivers/char/joystick/magellan.c

[Vojtech Pavlik]

On Tue, Jan 28, 2003 at 10:47:35AM -0800, Randy.Dunlap wrote:
> On Tue, 28 Jan 2003, Martin Mares wrote:
> 
> | Hi!
> |
> | > Without the patch below, the \0 terminating the string is written
> | > anywhere. nibbles[] would be even better, I guess.
> | > Can you check for stupidity on my side?
> |
> | As far as I remember, the ANSI C permits initialization of a char array
> | with a string of the same length and defines that the trailing \0 is
> | dropped in such cases. However, I cannot quote the right chapter and
> | verse by heart nor am I sure it's still permitted by C99, so better
> | check yourself.
> 
> The closest that I find in a quick scan is:
> 
> ANSI/ISO/IEC 9899-1999, page 126, section 6.7.8, constraint 14:
> 
> An array of character type may be initialized by a character string
> literal, optionally enclosed in braces. Successive characters of the
> character string literal (including the terminating null character if
> there is room or if the array is of unknown size) initialize the
> elements of the array.

Which means it was OK.

-- 
Vojtech Pavlik
SuSE Labs

Re: [BUG] in drivers/char/joystick/magellan.c

[Jörn Engel]

On Tue, 28 January 2003 22:03:53 +0100, Vojtech Pavlik wrote:
> On Tue, Jan 28, 2003 at 10:47:35AM -0800, Randy.Dunlap wrote:
> > 
> > An array of character type may be initialized by a character string
> > literal, optionally enclosed in braces. Successive characters of the
> > character string literal (including the terminating null character if
> > there is room or if the array is of unknown size) initialize the
> > elements of the array.
> 
> Which means it was OK.

Agreed. objdump -s gives the following:

 0040 30414233 44353647 48393a4b 3c4d4e3f  0AB3D56GH9:K<MN?
 0050 ffffffff 00000000 60010000 e0010000  ........`.......

So gcc does follow the spec here. (redhat gcc 2.96)

Thank you for the clarification, Martin and Randy.

Jörn

-- 
"Security vulnerabilities are here to stay."
-- Scott Culp, Manager of the Microsoft Security Response Center, 2001