Liquidsoap

External processes inherit opened file descriptor, including opened sockets.

Details

  • Type: Bug Bug
  • Status: Open Open
  • Priority: Critical Critical
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: Liquidsoap
  • Labels:
    None

Description

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

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Samuel Mimram added a comment -
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...)
Show
Samuel Mimram added a comment - 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...)
Hide
Permalink
David Baelde added a comment -
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.
Show
David Baelde added a comment - 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.
Hide
Permalink
David Baelde added a comment -
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...
Show
David Baelde added a comment - 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...
Hide
Permalink
Romain Beauxis added a comment -
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..
Show
Romain Beauxis added a comment - 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..
Hide
Permalink
Romain Beauxis added a comment -
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..
Show
Romain Beauxis added a comment - 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..
Hide
Permalink
Romain Beauxis added a comment -
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..
Show
Romain Beauxis added a comment - 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..
Hide
Permalink
Samuel Mimram added a comment -
Could you post a link to the OCaml BTS if you reported this there?
Show
Samuel Mimram added a comment - Could you post a link to the OCaml BTS if you reported this there?
Hide
Permalink
Romain Beauxis added a comment -
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..
Show
Romain Beauxis added a comment - 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..
Hide
Permalink
David Baelde added a comment -
This won't be fixed in 1.0.
Show
David Baelde added a comment - This won't be fixed in 1.0.
Hide
Permalink
Romain Beauxis added a comment -
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
Show
Romain Beauxis added a comment - 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
Hide
Permalink
Romain Beauxis added a comment -
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.
Show
Romain Beauxis added a comment - 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.

People

  • Assignee:
    Romain Beauxis
    Reporter:
    Romain Beauxis
Vote (1)
Watch (1)

Dates

  • Created:
    Updated: