Details
-
Type:
Bug
-
Status:
Open
-
Priority:
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...
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
Hide
Samuel Mimram
added a comment -
Permalink
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...)
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?
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..
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..
This won't be fixed in 1.0.
Show
David Baelde
added a comment - This won't be fixed in 1.0.
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
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
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.
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.