[Python-Dev] Proposal for a new function "open_noinherit" to avoid problems with subprocesses and security risks

[Henning von Bargen]

> "Martin v. Löwis" wrote:

>> Yes, I have a patch implemented in pure Python.
>>
>> I got the code on my workplace PC (now I am writing from home,
>> that's why I said I'll post the code later).
>>
>> The patch uses os.fdopen ( os.open (..., ...), ...).
>> It translates IOError into OSError then to raise the same class
>> of exception aso open().
>
> Hmm. I don't think I could accept such an implementation
> (whether in Python or in C). That's very hackish.

Well, if this is your opinion...
Take a look at the fopen implementation in stdio's fopen.c:
#  I found it via Google Code Search in the directory 
src/libc/ansi/stdio/fopen.c
# of http://clio.rice.edu/djgpp/win2k/djsr204_alpha.zip

FILE *
fopen(const char *file, const char *mode)
{
  FILE *f;
  int fd, rw, oflags = 0;
  ...
  fd = open(file, oflags, 0666);
  if (fd < 0)
    return NULL;

  f->_cnt = 0;
  f->_file = fd;
  f->_bufsiz = 0;
  ...
  }
  ...
  return f;
}

As you can see, at the C level, basically "fopen" is "open" with a little 
code
around it to parse flags etc. It's the same kind of hackish code.
And (apart from the ignored bufsize argument) the prototype is working fine.
I have to admit, though, that I am only using it on regular files.

Anyway, I don't want to argue about the implementation of a patch.
The fact is that until now the python programmer does not have an
easy, platform-independent option to open files non-inheritable.
As you mentioned yourself, the only way to work around it
in a platform-independent manner, IS VERY HACKISH.
So, shouldn't this hackish-ness better be hidden in the library
instead of leaving it as an execise to the common programmer?

The kind of errors I mentioned ("permission denied" errors that
seem to occur without an obvious reason) have cost me at least
two weeks of debugging the hard way (with ProcessExplorer etc)
and caused my manager to loose his trust in Python at all...
I think it is well worth the effort to keep this trouble away from
the Python programmers if possible.

And throughout the standard library modules, "open" is used,
causing these problems as soon as sub-processes come into play.

Apart from shutil.copyfile, other examples of using open that can cause
trouble are in socket.py (tell me any good reason why socket handles
should be inherited to child processes) and even in logging.py.

For example, I used RotatingFileHandler for logging my daemon
program activity. Sometimes, the logging  itself caused errors,
when a still-running child process had inherited the log file handle
and log rotation occured.

>
>> AFAIK in the C stdlib implementation, fopen is implemented
>> based on open anyway.
>
> Sure - and in turn, open is implemented on CreateFile.
> However, I don't think I would like to see an fopen
> implementation in Python. Python 3 will drop stdio entirely;
> for 2.x, I'd be cautious to change things because that
> may break other things in an unexpected manner.

Yeah, if you think it should not be included in 2.x,
then the handle inheritance problem should at least be considered
in the PEPs [(3116, "New I/O"), (337, "Logging Usage in the Standard 
Modules")]

>
>> On MS Windows, AFAIK (correct me if I am wrong), you can
>> only choose either to inherit handles or not *as a whole*
>> - including stdin, stdout and stderr -, when calling CreateProcess.
>
> I'm not sure. In general, that seems to be true. However,
> according to the ReactOS sources at
>
> http://www.reactos.org/generated/doxygen/dd/dda/dll_2win32_2kernel32_2process_2create_8c-source.html#l00624
>
> Windows will duplicate stdin,stdout,stderr from the parent
> process even if bInheritHandles is false, provided that
> no handles are specified in the startupinfo, and provided
> that the program to be started is a console (CUI) program.
>
>> Each handle has a security attribute that specifies whether the
>> handle should be inherited or not - but this has to be specified
>> when creating the handle (in the Windows CreateFile API internally).
>
> Not necessarily. You can turn on the flag later, through
> SetHandleInformation.

So do you think that a working "close_fds" could be implemented
for Windows as well?
Explicitly turning off the inheritance flag for all child handles except
stdin, stdout and stderr in subprocess / popen (the equivalent to
what close_fds does for Posix) - that's what I call hackish.
And I doubt that it is possible at all, for two reasons:
- you have to KNOW all the handles.
- due to the different process creation in Windows (there's no fork),
  you had to set the inheritance flags afterwards
- all this is not thread-safe.

>
>> Thus, on MS Windows, you can either choose to inherit all
>> files opened with "open" + [stdin, stdout, stderr],
>> or to not inherit any files (meaning even stdin, stdout and stderr
>> will not be inherited).
>>
>> In a platform-independent program, close_fds is therefore not an option.
>
> ... assuming you care about whether stdin,stdout,stderr are inherited
> to GUI programs. If the child process makes no use of stdin/stdout, you
> can safely set close_fds to true.

Hmm...
In the bug 1663329 I posted ("subprocess/popen close_fds perform poor if 
SC_OPEN_MAX is hi"),
you suggested:
"""
- you should set the FD_CLOEXEC flag on all file descriptors you don't
  want to be inherited, using fnctl(fd, F_SETFD, 1)
"""

Apart from the fact that this is not possible on MS Windows, it won't solve 
the problem!
(Because then I couldn't use all those standard modules that use open 
*without* FD_CLOEXEC).

The fact is that the combination ("multi-threading", "subprocess creation", 
"standard modules")
simply *does not work* flawlessly and produces errors that are hard to 
understand.
And probably most progammers are not even aware of the problem.
That's the main reason why I posted here.

And, in my experience, programs tend to get more complex and in the future
I expect to see more multi-threaded Python-programs.
So the problem will not vanish - we will see it more often than we like...

Regards,
Henning