[MPlayer-dev-eng] [PATCH] Use unrar for open vobsubs if available

[Rich Felker]

On Thu, Dec 06, 2007 at 11:58:59PM +0100, Reimar Doeffinger wrote:
> Hello,
> On Thu, Dec 06, 2007 at 11:20:14PM +0800, Ulion wrote:
> [...]
> > I really don't like writing windows api code, anyone can save me? I'd
> > like to use popen on mingw if no one want to write that part of
> > CreateProcess code for it. On the other hand, once the executable is a
> > pre-tested existed file and command line parameters are correctly
> > escaped, there's no obvious security problem to use popen on win32.
> 
> The escaping rules for Windows are rather weird, and at least I don't easily
> feel confident to say that I know all the small hacks they introduced.

Agree, that's why I think using the Win API function to convert back
to argv[] form and test for correctness would be smart.. Especially
with the bad security history this patch has.

> [...]
> > Here's the updated version, if there are not objects, I will commit it
> > in 2 days.
> 
> I'd prefer if you could throw out the Mingw stuff for the first version, it
> is really hard to review, also since I have not found a really sufficient
> documentation concerning proper escaping from Microsoft (though I did not
> look too hard).

Considering that Ulion is a Windows user I doubt this is an option..

> > +    pid = fork();
> > +    if (pid == 0) {
> > +        /* This is the child process. Execute the unrar executable. */
> > +        close(mypipe[0]);
> > +        dup2(mypipe[1], 1);
> > +        close(2); /* suppress stderr messages */
> 
> This looks like a horrible idea, the next file that the unrar binary opens
> can get 2 as file descriptor, and any error messages will end up in some
> "random" file.

Oh, I missed that. This is a bad bug. Always open /dev/null if you
close one of the standard file descriptors.

> > +    *output = (char*)malloc(bufsize);
> 
> No pointless casts please.

Indeed. This should be added to our mplayer lint script if it's not
checked already (casts on return value of malloc). Worst of all, it
hides the serious bug if you happen to forget to prototype malloc!

> > +    if (!*output) goto byebye;
> 
> After having to deal with out RTjpeg implementation, I don't find
> such goto labels not funny anymore.
> err_out or something similar are used elsewhere I think.

I don't care how labels are named, but whatever.

> > +    if (!*output || !*size || (pid == -1 && errno != ECHILD) ||
> > +            (pid > 0 && status)) {
> 
> What's the advantage of this over a simple
> > if (!*output || !*size || pid == -1 || status)
> ?

Agreed.

> > +    if (bufsize > *size) {
> > +        char *p = realloc(*output, *size);
> > +        if (p)
> > +            *output = p;
> > +    }
> 
> Not sure if this really matters. On the one hand it saves a bit of memory,
> on the other hand realloc can be quite slow...

Realloc to reduce size should be O(1). If it's not then your malloc
implementation sucks.

> > +        while (*p && strchr(" \t\n\r\v\f", *p))
> > +            ++p;
> > +        if (!*p)
> > +            fname[0] = '\0';
> > +
> > +        if (!fname[0] || sscanf(p, "%d %d %d%% %d-%d-%d %d:%d",
> > +                                &unpsize, &packsize, &ratio,
> > +                                &day, &month, &year, &hour, &min) != 8)
> > +            strcpy(fname, p);
> > +        else {
> 
> I really dislike how fname[0] is misused as a state variable here. I think
> the code would be much clearer as

No opinion here.

Rich