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

[Reimar Döffinger]

On Mon, Jun 07, 2010 at 03:29:20PM -0700, Howard Chu wrote:
> >>>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)
> >>
> >>Maximum length for UTF-8 is 6 bytes.
> >
> >Where did you get that from? To my knowledge the longest valid UTF-8
> >is 4 bytes (which is without any relevance here though),
> 
> Try the original spec, RFC2279.
> http://www.ietf.org/rfc/rfc2279.txt
> 
> Section 2. UTF-8 definition
> >>>>
> In UTF-8, characters are encoded using sequences of 1 to 6 octets.
> <<<<

None of the legal Unicode values result in more than 4 octets.

> >the maximum length for PUT_UTF8 is 7 bytes, and we need a
> >0-termination.
> 
> If PUT_UTF8 can output 7 bytes then it's broken.

If it didn't it would need an extra check and error handling code
to handle any values > 0x7fffffff. Since it doesn't attempt
to detect invalid codes anyway there's not really much point in it.
Also I think the documentation is quite clear on it.

> I respect your attention to detail and security-consciousness. I
> don't respect your treatment of this topic with superstition,
> instead of facts.
> I gave you solid facts - a binary number read from a base16 string
> cannot overflow the original space - and you're the one handwaving
> them away.
> 
> You gave a completely valid criticism - negative numbers need to be
> checked. Unicode is only defined for 0-0x7fffffff, i.e., positive
> only, and I've addressed that. The rest is just superstition.

You've addressed one specific issue I gave, without changing your
argument one bit despite it having failed before.
Where do you suddenly get the confidence that your way of arguing
should suddenly work now since it failed once?
I agree you are very, very, very likely right, but repeating an
argument that has already been shown to be insufficient as "proof"
does not make it a proof.
And since I suspect nobody really wants to waste the time doing it
properly (e.g. I am not certain what strange strtoul implementations
may exist in addition, probably they keep to the specification on
all supported systems or deviate in a way that does not matter here,
who knows), I suggested a rather simple solution.