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

[Reimar Doeffinger]

Hello,
On Fri, Dec 07, 2007 at 12:31:53PM +0800, Ulion wrote:
> 2007/12/7, Reimar Doeffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> > 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.
> 
> That's windows' problem, we have to work with it, right? And it looks
> not strange so much, because '\' is a valid path seperator on windows,
> so they can only use other char as escape char, they choosed '^'. The
> only strange thing is that '^' does not support escape space, but
> spaces can be quoted, that's all the rules.

No, it is not. There is at least one additional rule, mentioned in the function
that converts command line to argv: 2n '\' followed by '"' convert to n '\' and the
'"' used for quoting, 2n+1 '\' followed by '"' result in n '\' followed by a literal
'"' and '\' not followed by '"' remains unchanged.

> > [...]
> > > 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).
> 
> I don't care there are no support to mingw, but it sounds it will also
> never get chance to pass in the future because windows does not
> document it clearly. How about make an alarm in manpage to notify
> users to take risks of using this feature?

Actually for CreateProcess it is quite clearly documented that unescaping and parsing
into argc/v can be done with the function I mentioned before, so there should be at
least a reliable way to confirm we did the escaping right, but that needs a rework
of the code...

> > > +    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.
> 
> Fixed, should the stdin be closed also?

No, none of them need to be closed. All you need to do is replace stderr instead of
closing.
So something like
> fd = open("/dev/null", O_WRONLY);
> if (fd < 3) _exit(EXIT_FAILURE);
> dup2(fd, 2);
> close(fd);
should do it.
Note that this will cause MPlayer to fail if it was started without
stdin, stdout or stderr itself (though I don't think we really need
to care about that).

> > [...]
> > > +    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...
> 
> One thing to make using popen calls safer is to make sure the
> executable is an existed file, then the shell will not search the
> executable in its search PATH, and it also make the executable set by
> user harder be some strange hack string if it possiblly be hacked.

The check then should be at least in the same function as popen...

[...]
> > > +    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)
> > ?
> 
> ECHILD maybe a valid result, because the 'MPlayer' already eaten the
> exit status somewhere by other waitpid or sighandler.

Well, we still can not check the exit status in that case, simply assuming
that the exec worked is a bit weird way to handle it.
Not to mention that under Linux MPlayer is not multithreaded so there is no
other code that can do waitpid (ignoring the sighandler which can easily be
checked), and if it was multithreaded it should _never_ call waitpid without
a pid.

> > > +    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...
> 
> The realloc could already called in the loop for several times, so one
> more realloc to save memory for following playing may help more I
> think.

Yes, you're probably right on that, esp. since it only reduces the size which is
not that costly.

Greetings,
Reimar Döffinger