[9fans] possible buffer overflow in devcons.c?

[9fans] possible buffer overflow in devcons.c?

[Yoann Padioleau]

Hi,

In devcons.c there is

/*
 *  Put character, possibly a rune, into read queue at interrupt time.
 *  Called at interrupt time to process a character.
 */
int
kbdputc(Queue*, int ch)
{
	int i, n;
	char buf[3]; <----- enough?
	Rune r;
	char *next;

	if(kbd.ir == nil)
		return 0;		/* in case we're not inited yet */
	
	ilock(&kbd.lockputc);		/* just a mutex */
	r = ch;
	n = runetochar(buf, &r);
	for(i = 0; i < n; i++){
		next = kbd.iw+1;
		if(next >= kbd.ie)
			next = kbd.istage;
		if(next == kbd.ir)
			break;
		*kbd.iw = buf[i];
		kbd.iw = next;
	}
	iunlock(&kbd.lockputc);
	return 0;
}

But is the buf[3] enough? UTFMAX is 4 so we could possibly overflow no?
Shouldn't it be buf[UTFMAX] ?

Re: [9fans] possible buffer overflow in devcons.c?

[erik quanstrom]

> But is the buf[3] enough? UTFMAX is 4 so we could possibly overflow no?
> Shouldn't it be buf[UTFMAX] ?

yes, it should.  i thought i'd caught all of those.

- erik

Re: [9fans] possible buffer overflow in devcons.c?

[andrey mirtchovski]

used to be 3 :)

"UTFmax, defined as 3 in <libc.h>, is the maximum number of bytes
required to represent a rune."

http://man.cat-v.org/plan_9/2/rune

Re: [9fans] possible buffer overflow in devcons.c?

[erik quanstrom]

On Wed Jun 18 13:36:09 EDT 2014, mirtchovski@gmail.com wrote:
> used to be 3 :)
>
> "UTFmax, defined as 3 in <libc.h>, is the maximum number of bytes
> required to represent a rune."

which is exactly why this should have been caught.
this one's my fault.

- erik

Re: [9fans] possible buffer overflow in devcons.c?

[Yoann Padioleau]

Well, hard to grep for '3' in a big codebase. They should have used UTFMAX in the first place.

I see though that currently in libc.h I have

enum
{
	UTFmax		= 4,		/* maximum bytes per rune */
	Runesync	= 0x80,		/* cannot represent part of a UTF sequence (<) */
	Runeself	= 0x80,		/* rune and UTF sequences are the same (<) */
	Runeerror	= 0xFFFD,	/* decoding error in UTF */
	Runemax		= 0x10FFFF,	/* 21-bit rune */
	Runemask	= 0x1FFFFF,	/* bits used by runes (see grep) */
};

so Runemax seems to indicate we never produce rune using more than 3 bytes no?
So maybe buf[3] is safe?

On Jun 18, 2014, at 10:36 AM, erik quanstrom <quanstro@quanstro.net> wrote:

> On Wed Jun 18 13:36:09 EDT 2014, mirtchovski@gmail.com wrote:
>> used to be 3 :)
>> 
>> "UTFmax, defined as 3 in <libc.h>, is the maximum number of bytes
>> required to represent a rune."
> 
> which is exactly why this should have been caught.
> this one's my fault.
> 
> - erik
> 

Re: [9fans] possible buffer overflow in devcons.c?

[erik quanstrom]

On Wed Jun 18 14:15:14 EDT 2014, pad@fb.com wrote:
> Well, hard to grep for '3' in a big codebase. They should have used UTFMAX in the first place.
>

it's not hard.  i did this once.  i think i did something like this

	g 'char[ 	]+[a-z]+\[(3|4|3\+1)\]' /sys/src

- erik

Re: [9fans] possible buffer overflow in devcons.c?

[erik quanstrom]

On Wed Jun 18 14:20:02 EDT 2014, quanstro@quanstro.net wrote:
> On Wed Jun 18 14:15:14 EDT 2014, pad@fb.com wrote:
> > Well, hard to grep for '3' in a big codebase. They should have used UTFMAX in the first place.
> >
>
> it's not hard.  i did this once.  i think i did something like this
>
> 	g 'char[ 	]+[a-z]+\[(3|4|3\+1)\]' /sys/src

er, better to have

	g '( |	|^)char[ 	]+[a-z]+\[(3|3 *\+ *1)\]' /sys/src

it looks like 9atom is clean.  i haven't checked sources.  but that
reminds me i have to kill that private copy of devcons in omap.

- erik