[MPlayer-dev-eng] [PATCH] fix SAMI parsing

[Reimar Döffinger]

On Mon, Jun 07, 2010 at 12:29:00AM -0700, Howard Chu wrote:
> Howard Chu wrote:
> >Reimar Döffinger wrote:
> >>On Sun, Jun 06, 2010 at 07:24:46PM -0400, hyc at highlandsun.com wrote:
> >>>On Mon, Jun 07, 2010 at 12:26:33AM +0200, Reimar D?ffinger wrote:
> >>>>On Sun, Jun 06, 2010 at 02:51:29PM -0700, Howard Chu wrote:
> >>>>
> >>>>>+		uint32_t c = strtol(s+2,&s, 0);
> >>>>>+		uint8_t tmp;
> >>>>>+		PUT_UTF8(c, tmp, *p++ = tmp;)
> >>>>>+		if (*s == ';') s++; }
> >>>>
> >>>>I'm sorry, but I think you'll have to somehow "prove"
> >>>>this is not a security issue.
> >>>>Checking and documenting that PUT_UTF8 will never write
> >>>>more than we read might be possible.
> >>>>Or just "blindly" checking we still have at least 8 bytes
> >>>>free should do as well.
> >>>
> >>>Not necessary. Decimal numbers encode only 3.25 bits per byte, while UTF-8 encodes 7 bits per byte. This conversion will always fit.
> >>
> >>Given that 3.25 bits just don't exist, it obviously is not that easy.
> >>And you forgot another case as well: negative numbers.
> >>This kind of thing just can't be done hand-wavy like that, or you'll
> >>always miss a case.
> 
> >Of course, we could just check for a '-' in the string and drop this entity.
> >Nobody should be using negative character codes.
> 
> Did that. Also had to check for " " in addition to " ".

And how do you know that you/we did not yet miss another case?
Especially since no base is specified in the strtol call.
This code is simply too complex for security-critical code
where a mistake will allow writing arbitrary values onto the stack.
Just add a
if (p - text >= sizeof(text) - 8)
break;
or so.