savonet/liquidsoap — pull request #128 by smimram (2013-11-13 15:01:33+00:00)
Title: RIP: external processes inherit opened file descriptor, including opened sockets.

Triaging branches.

@toots could this branch be merged or forgotten?

For more information, see [the original bug report](http://dev.sourcefabric.org/browse/LS-512), copied here:

@toots

When liquidsoap spawns a new process using Ocaml's open_process* functions, the new process is created using fork() and therefore inherits all opened file descriptors from liquidsoap.
This leads to many different type of issues, among which:
- If liquidsoap stops before an external process, any port opened by liquidsoap remains open until all external processes have terminated
- All external processes have access to the file/sockets opened by liquidsoap, in particular they may read a file whose content is supposed to be protected (password) or listen to network traffic (source password for instance)
The problem is not easy. There are several possibilities:
- Define our own implementation of Unix.open_process*
- Use some shell trickery to close the descriptors before invoking the new process. Something like:
````
"/bin/ls /dev/fd/ | while read i; do if test "$i" ge "3"; then exec "$i<&" 2>/dev/null; fi done; my_process
````
Convince OCaml's maintainer to apply some patch and wait for a new release of OCaml...

@smimram

A simple solution for the first problem would be to use a wrapper around open_process which would register alive processes into a list, so that we can kill them when Liq is shut down. The second point is not really an issue for now I think (people usually trust the programs they run...)

@dbaelde

Good point, it's an important problem to address. Solution #3 might be a good idea but it's a long call. Rewriting a better open_process should be relatively simple: there should be C code available online that we could easily translate... A quick googling didn't quite work but taught me that file descriptors can be set to be automatically closed upon exec (http://www.tek-tips.com/viewthread.cfm?qid=1333197&page=4) which probably not a simple solution for us.

@dbaelde

A better reference, wikipedia of course: http://en.wikipedia.org/wiki/Fork-exec
I'm realizing that this problem becomes more complicated when Windows has to be taken into account...

@toots

I agree that the security issue may not be that bad for now.. However, there is the possibility now to serve HTTP requests within liq so we should be cautious..
The POSIX code for open_process is relatively simple and it could be possible to duplicate it if we wanted so:
````
let open_proc cmd proc input output toclose =
let cloexec = List.for_all try_set_close_on_exec toclose in
match fork() with
0 -> if input <> stdin then begin dup2 input stdin; close input end;
if output <> stdout then begin dup2 output stdout; close output end;
if not cloexec then List.iter close toclose;
begin try execv "/bin/sh" [| "/bin/sh"; "-c"; cmd |]
with _ -> exit 127
end
id -> Hashtbl.add popen_processes proc id
````
For the windows case, I am not even sure that the problem occurs since fork is not implemented in windows. The code in this case is quite different..

@toots

Concerning closing opened process at shutdown, I forgot to mention one point.
In fact, all opened processes should be closed when shutting down properly. If not it should be a bug.
However, the problem is that we do not always have the possibility to exit properly. For instance, a segfault in lame (hapens..) or a SIGKILL sent to liquidsoap..

@toots

The issue has been forwarded to ocaml's maintainers but it does not seem that a fix will come any time soon..
I'm downgrading to Critical thus, as I think that even though it is a serious issue, it should be fixed in OCaml and does not prevent a release..

@smimram

Could you post a link to the OCaml BTS if you reported this there?

@toots

It was reported in a private bug report as I though it could have security implications (oscigen uses this mechanism for its CGI code for instance..)
I have to say that I am a bit disapointed with the way they handle this..

@dbaelde

This won't be fixed in 1.0.

@toots

I've pushed a proposed fix in LS-512 branch. Basically it consists in forcing all file descriptors >= 3 to be marked close-on-exec. Should be properly tested on win32 and other exotic OSes. Here's a way to reproduce:
Start this process:
````
liquidsoap 'output.harbor(%external("lame - -"),mount="foo",single("/Users/toots/Downloads/BB Seaton Anthology CD1/09 Forgive Them Lord.mp3"),format="audio/mpeg")'
````
Check without the patch that lame inherits the file descriptor for that opened file:
````
toots@zulu src % lsof | grep lame
lame 1492 toots cwd DIR 14,2 2890 8650344 /Users/toots/sources/savonet/liquidsoap/src
lame 1492 toots txt REG 14,2 369168 938271 /usr/local/Cellar/lame/3.98.4/bin/lame
lame 1492 toots txt REG 14,2 1054960 414150 /usr/lib/dyld
lame 1492 toots txt REG 14,2 235044864 55418780 /private/var/db/dyld/dyld_shared_cache_x86_64
lame 1492 toots 0 PIPE 0x08440770 16384 ->0x0b266af8
lame 1492 toots 1 PIPE 0x0b2665e4 16384 ->0x0b266a30
lame 1492 toots 2u CHR 16,3 0t4233500 629 /dev/ttys003
lame 1492 toots 3 PIPE 0x0a9a5c20 16384 ->0x0a9a64b8
lame 1492 toots 4 PIPE 0x0a9a64b8 16384 ->0x0a9a5c20
lame 1492 toots 5 PIPE 0x0a9a54b4 16384 ->0x0a9a4af0
lame 1492 toots 6 PIPE 0x0a9a4af0 16384 ->0x0a9a54b4
lame 1492 toots 7r REG 14,2 2260607 59683552 /Users/toots/Downloads/BB Seaton Anthology CD1/09 Forgive Them Lord.mp3
````
Check that it no longer inherits it after applying the patch:
````
toots@zulu src % lsof | grep lame
lame 9782 toots cwd DIR 14,2 2890 8650344 /Users/toots/sources/savonet/liquidsoap/src
lame 9782 toots txt REG 14,2 369168 938271 /usr/local/Cellar/lame/3.98.4/bin/lame
lame 9782 toots txt REG 14,2 1054960 414150 /usr/lib/dyld
lame 9782 toots txt REG 14,2 235044864 55418780 /private/var/db/dyld/dyld_shared_cache_x86_64
lame 9782 toots 0 PIPE 0x0b266e7c 16384 ->0x0b26606c
lame 9782 toots 1 PIPE 0x0b266af8 16384 ->0x0b266a30
lame 9782 toots 2u CHR 16,3 0t4284270 629 /dev/ttys003
````

@toots

To clarify: by tested I mean to check if:
1. it compiles fine
2. it does anything
The patch is intended for win32 to only 1) applies in this case.