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

[Reimar Döffinger]

On Mon, Jun 07, 2010 at 10:40:54AM -0700, Howard Chu wrote:
> Reimar Döffinger wrote:
> >>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.
> 
> With base = 0 the only possibilities are base 8, 10, and 16. At most
> base 16 packs 4 bits per byte so again, there is no chance for
> overflow. Also keep in mind that there's 3 bytes of slop for free -
> the "&#;" are being dropped.

You said that the last time as well.
I see more complex, more risky code at a security-relevant point
for no benefit at all.

> >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),
the maximum length for PUT_UTF8 is 7 bytes, and we need a
0-termination.
Here's even a patch to do this way that avoids losing any data if possible...
Index: subreader.c
===================================================================
--- subreader.c (revision 31346)
+++ subreader.c (working copy)
@@ -170,6 +170,8 @@
 
        case 3: /* get all text until '<' appears */
            if (*s == '\0') break;
+           else if (p - text >= sizeof(text) - 8)
+               sami_add_line(current, text, &p);
            else if (!strncasecmp (s, "<br>", 4)) {
                 sami_add_line(current, text, &p);
                s += 4;