avoid inheritance of file descriptors

  Hi,

I think there is a problem in calling external programs from plugins. If 
such a program takes some while for execution (even in background) it 
gets inherited all file descriptors of VDR. This prevents vdr from 
zapping to another channel or even from restarting properly. You will 
see messages like:

ERROR: /dev/dvb/adapter0/dvr0: Device or resource busy
or
ERROR (svdrp.c,84): Address already in use

Reason: By default unix inherits all file descriptors to child processes 
when calling exec*(...) or system(...). You can avoid this by setting 
FD_CLOEXEC on all file descriptors that should not be inherited.

Patch:


Comments, ideas? I would be happy to see this little patch applied to 
1.4 and 1.5 trunks of VDR.

Deti
  

On 12/10/07 15:28, Deti Fliegl wrote:
> Hi,
> 
> I think there is a problem in calling external programs from plugins. If 
> such a program takes some while for execution (even in background) it 
> gets inherited all file descriptors of VDR. This prevents vdr from 
> zapping to another channel or even from restarting properly. You will 
> see messages like:
> 
> ERROR: /dev/dvb/adapter0/dvr0: Device or resource busy
> or
> ERROR (svdrp.c,84): Address already in use
> 
> Reason: By default unix inherits all file descriptors to child processes 
> when calling exec*(...) or system(...). You can avoid this by setting 
> FD_CLOEXEC on all file descriptors that should not be inherited.
> 
> Patch:
> ...
> Comments, ideas?

Doesn't SystemExec() (see tools.c) take care of this?

Klaus
  

Klaus Schmidinger wrote:
> Doesn't SystemExec() (see tools.c) take care of this?
Yes you are right - it takes care internally but not for plugins like 
dvdswitch etc. In order to fix this problem you could patch every single 
plugin or just set the right file descriptor flag once. I think the 
latter does not cause any interference and should solves some issues.

Deti
  

Deti Fliegl wrote:
> Klaus Schmidinger wrote:
>> Doesn't SystemExec() (see tools.c) take care of this?
> Yes you are right - it takes care internally but not for plugins like 
> dvdswitch etc. In order to fix this problem you could patch every single 
> plugin or just set the right file descriptor flag once. I think the 
> latter does not cause any interference and should solves some issues.

For the record, the latter creates a small race condition: an external
program could be launched before FD_CLOEXEC is set on an fd.
  

Anssi Hannula wrote:
> Deti Fliegl wrote:
>> Klaus Schmidinger wrote:
>>> Doesn't SystemExec() (see tools.c) take care of this?
>> Yes you are right - it takes care internally but not for plugins like 
>> dvdswitch etc. In order to fix this problem you could patch every single 
>> plugin or just set the right file descriptor flag once. I think the 
>> latter does not cause any interference and should solves some issues.
> 
> For the record, the latter creates a small race condition: an external
> program could be launched before FD_CLOEXEC is set on an fd.
In VDR all file descriptors seem to be allocated at startup. IMHO it is 
not very likely that a race condition could happen.

Deti
  

Deti Fliegl wrote:
> Anssi Hannula wrote:
>> Deti Fliegl wrote:
>>> Klaus Schmidinger wrote:
>>>> Doesn't SystemExec() (see tools.c) take care of this?
>>> Yes you are right - it takes care internally but not for plugins like 
>>> dvdswitch etc. In order to fix this problem you could patch every single 
>>> plugin or just set the right file descriptor flag once. I think the 
>>> latter does not cause any interference and should solves some issues.
>> For the record, the latter creates a small race condition: an external
>> program could be launched before FD_CLOEXEC is set on an fd.
> In VDR all file descriptors seem to be allocated at startup. IMHO it is 
> not very likely that a race condition could happen.
... except on dvr devices, I know.

Deti
  

On 12/10/07 21:07, Deti Fliegl wrote:
> Klaus Schmidinger wrote:
>> Doesn't SystemExec() (see tools.c) take care of this?
> Yes you are right - it takes care internally but not for plugins like 
> dvdswitch etc. In order to fix this problem you could patch every single 
> plugin or just set the right file descriptor flag once. I think the 
> latter does not cause any interference and should solves some issues.

Plugins can call SystemExec() just as well when the want to execute
an external program.

IMHO it is no feasible solution to expect every file handle to
be opened with FD_CLOEXEC. Even if VDR itself would do this, there
could still be plugins that don't.

I'd say if some plugin wants to run an external program, it needs
to take care by itself that all unneeded file handles are closed.
That's what SystemExec() is for.

Klaus
  

I demand that Deti Fliegl may or may not have written...

> Anssi Hannula wrote:
>> Deti Fliegl wrote:
>>> Klaus Schmidinger wrote:
>>>> Doesn't SystemExec() (see tools.c) take care of this?
>>> Yes you are right - it takes care internally but not for plugins like 
>>> dvdswitch etc. In order to fix this problem you could patch every single 
>>> plugin or just set the right file descriptor flag once. I think the 
>>> latter does not cause any interference and should solves some issues.
>> For the record, the latter creates a small race condition: an external
>> program could be launched before FD_CLOEXEC is set on an fd.

> In VDR all file descriptors seem to be allocated at startup. IMHO it is 
> not very likely that a race condition could happen.

>From open(2):

  O_CLOEXEC (Since Linux 2.6.23)
        Enable the close-on-exec  flag  for  the  new  file  descriptor.
        Specifying  this  flag  permits a program to avoid an additional
        fcntl(2) F_SETFD operation to set the  FD_CLOEXEC  flag.   Addi-
        tionally,  use  of  this flag is essential in some multithreaded
        programs since using a separate fcntl(2)  F_SETFD  operation  to
        set  the  FD_CLOEXEC  flag does not suffice to avoid race condi-
        tions where one thread opens a file descriptor at the same  time
        as another thread does a fork(2) plus execve(2).

No use *now*, I know - too many people not yet using a new-enough kernel...
  

Klaus Schmidinger wrote:
> Plugins can call SystemExec() just as well when the want to execute
> an external program.
Yes, but would you point every single developer on this issue?

> IMHO it is no feasible solution to expect every file handle to
> be opened with FD_CLOEXEC. Even if VDR itself would do this, there
> could still be plugins that don't.
It is enough to let VDR set this flag on its own file handles - the main 
problem is that while an external script/program is running and because 
of inherited DVB handles
- zapping is blocked
- a restart of VDR is impossible.

> I'd say if some plugin wants to run an external program, it needs
> to take care by itself that all unneeded file handles are closed.
> That's what SystemExec() is for.
Whatever - if you don't see the point I cannot help.

Deti
  

On 12/11/2007 08:39 AM, Deti Fliegl wrote:
> Klaus Schmidinger wrote:
>> Plugins can call SystemExec() just as well when the want to execute
>> an external program.
> Yes, but would you point every single developer on this issue?
> 
>> IMHO it is no feasible solution to expect every file handle to
>> be opened with FD_CLOEXEC. Even if VDR itself would do this, there
>> could still be plugins that don't.
> It is enough to let VDR set this flag on its own file handles - the main 
> problem is that while an external script/program is running and because 
> of inherited DVB handles
> - zapping is blocked
> - a restart of VDR is impossible.
> 
>> I'd say if some plugin wants to run an external program, it needs
>> to take care by itself that all unneeded file handles are closed.
>> That's what SystemExec() is for.
> Whatever - if you don't see the point I cannot help.

Why is this suddenly such a big problem?
If a plugin wants to run an external program it can simply
use SystemExec().

Besides, as Darren Salt pointed out, this flag is apparently
only available in the very latest kernel version, which I'm
pretty sure is not that widely in use yet.

Klaus
  

I demand that Klaus Schmidinger may or may not have written...

[snip; FD_CLOEXEC]
> Why is this suddenly such a big problem?
> If a plugin wants to run an external program it can simply use
> SystemExec().

> Besides, as Darren Salt pointed out, this flag is apparently only available
> in the very latest kernel version, which I'm pretty sure is not that widely
> in use yet.

Er, no: FD_CLOEXEC has been available for years (see fcntl(2)). You're
confusing it with O_CLOEXEC, which is new.
  

On 12/11/2007 01:09 PM, Darren Salt wrote:
> I demand that Klaus Schmidinger may or may not have written...
> 
> [snip; FD_CLOEXEC]
>> Why is this suddenly such a big problem?
>> If a plugin wants to run an external program it can simply use
>> SystemExec().
> 
>> Besides, as Darren Salt pointed out, this flag is apparently only available
>> in the very latest kernel version, which I'm pretty sure is not that widely
>> in use yet.
> 
> Er, no: FD_CLOEXEC has been available for years (see fcntl(2)). You're
> confusing it with O_CLOEXEC, which is new.

Oh, sorry.

Nevertheless, SystemExec() (or something similar) is what a plugin
should use when launching an external program.

Klaus