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

[Rich Felker]

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.

Rich