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

[Ulion]

2007/12/6, Rich Felker <dalias at aerifal.cx>:
> On Thu, Dec 06, 2007 at 08:46:48PM +0800, Ulion wrote:
> > 2007/12/6, Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> > > Hello,
> > > On Thu, Dec 06, 2007 at 07:20:54PM +0800, Ulion wrote:
> > > > With Rich's suggestion, patch updated:
> > > > 1. Remove access check code change to use access() call.
> > >
> > > What is the point of that though? And why not put it in the pipe_open
> > > function, together with the unrar_executable != NULL check?
> >
> > I meant use access() instead of stat(). As move them into pipe_open,
> > I'd like to, but current pipe_open are splited by MINGW macro and some
> > declaration at beginning of each part of #ifdef. Then that need two
> > copy of access check code also, so that looks not better than current
> > implement.
>
> I don't care where access(2) is called as long as it's used instead of
> the (incorrect; ignoring permissions) use of stat(2).
>
> > > And you also have a comment saying
> > > "/* This is the child process.  Execute the shell command. */"
> > > but there is no shell involved, you execute the file directly via execl...
> >
> > Oh, that comment comes from old patch code, I fix it. btw: does it
> > make any sense to run the executable by a shell?
>
> No, it's extremely bad to run things via the shell. Strong security
> considerations are a must and it's less efficient. Most secure coding
> policy forbids the use of the shell entirely (and thus forbids
> system(3) and popen(3) as well).
>
> > > About the Windows part: You could try using CommandLineToArgvW to make
> > > sure your escaping did not mess up (and neither does the windows code
> > > mess up undoing the escaping).
> >
> > I just checked the API you mentioned, it seems be used parsing whole
> > command line into argv list, it's not what we needed, if there's such
> > api match our needs, we should use it. By now, we still can only use
> > our escape function for that.
>
> Reimar meant using it after the conversion to convert back and make
> sure the composition of the two maps is the identity. This _should_ be
> a redundant check, but it was intended to prevent any security bugs
> from slipping through. I still think it would be nicest if you could
> remove the use of cmd.exe entirely.

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.

> > +    while (fgets(buf, sizeof(buf), rar_pipe)) {
> > +        char fname[1024];
> > +        int packsize, unpsize, ratio;
> > +        int day, month, year, hour, min;
> > +
> > +        if (sscanf(buf, "%1023s %d %d %d%% %d-%d-%d %d:%d",
> > +                   fname, &unpsize, &packsize, &ratio,
> > +                   &day, &month, &year, &hour, &min) == 9) {
>
> This code fails on spaces in filenames. Is there any way to handle
> them? Parsing from end-of-line to beginning, perhaps, but it would be
> rather ugly..

It did fail to read filenames with space. I rechecked unrar's command
line options, and found a command 'v' instead of 'l', it add
additional '\n' after the filename, so we can get the filename with
spaces in it.

Here's the updated version, if there are not objects, I will commit it
in 2 days.

-- 
Ulion
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: unrar_exec9.diff
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20071206/04ba5760/attachment.asc>