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

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

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

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

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

Sure.

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

Removed.

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

OK, changed to *size == bufsize, since > never happens.

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

Sure.

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

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

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

Changed to use state.


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