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

[Reimar Doeffinger]

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.

[...]
> 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).

> +    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.

[...]
> +    if (!unrar_executable || access(unrar_executable, X_OK)) return 0;
> +    if (access(rarfile, R_OK)) return 0;

As said before, I don't really see an advantage in further checks, firstly
fork() should be really cheap on most systems where we use it, secondly
it only happens when someone explictly gives a binary that does not work...

> +    *output = (char*)malloc(bufsize);

No pointless casts please.

> +    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.

> +    while ((bytesread=fread(*output+*size, 1, bufsize-*size, rar_pipe)) > 0) {
> +        *size += bytesread;
> +        if (bytesread == ALLOC_INCR) {

This condition makes no sense to me. Shouldn't it something like *size >= bufsize?

> +            char *p;
> +            bufsize += ALLOC_INCR;
> +            p = (char*)realloc(*output, bufsize);

Pointless cast.

> +    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)
?

> +    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...

> +        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
int state = PARSE_NAME;
...
if (!*p) {
  state = PARSE_NAME;
  continue;
}
if (state == PARSE_PROPS && sscanf(...) != 8)
  state = PARSE_NAME;
if (state == PARSE_NAME)
  strcpy(fname, p);
...

> +            new = (ArchiveList_struct *)calloc(1, sizeof(ArchiveList_struct));

pointless cast.

Greetings,
Reimar Döffinger