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

[Ulion]

2007/12/7, Reimar Doeffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> 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.

Oh, I missed it, escape function update.

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

dup2() will close target fd if it's openen. So current code looks ok for me:
        /* suppress stderr messages */
        close(2);
        fd = open("/dev/null", O_WRONLY);
        if(fd != 2) {
            if (dup2(fd, 2))
                _exit(EXIT_FAILURE);
            close(fd);
        }

Is it ok for you?

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

Moved in.

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

That's why check !*size in rar_get as a failure. For rar_list, I
updated the code, so if nothing read from the pipe, we will got a
file_num less than 0.

Here's updated patch.

-- 
Ulion
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: unrar_exec11.diff
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20071207/3344c4ca/attachment.txt>