Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

open() does not able to set flags, such as O_CLOEXEC #56314

Closed
socketpair mannequin opened this issue May 18, 2011 · 49 comments
Closed

open() does not able to set flags, such as O_CLOEXEC #56314

socketpair mannequin opened this issue May 18, 2011 · 49 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@socketpair
Copy link
Mannequin

socketpair mannequin commented May 18, 2011

BPO 12105
Nosy @birkenfeld, @amauryfa, @pitrou, @vstinner, @socketpair, @akheron
Superseder
  • bpo-12797: io.FileIO and io.open should support openat
  • Files
  • os_cloexec.diff: patch adding O_CLOEXEC to os module
  • os_cloexec_1.diff
  • open_cloexec.py
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2011-10-29.15:57:50.014>
    created_at = <Date 2011-05-18.19:11:28.652>
    labels = ['type-bug', 'library']
    title = 'open() does not able to set flags, such as O_CLOEXEC'
    updated_at = <Date 2013-01-03.15:24:48.325>
    user = 'https://github.com/socketpair'

    bugs.python.org fields:

    activity = <Date 2013-01-03.15:24:48.325>
    actor = 'vstinner'
    assignee = 'neologix'
    closed = True
    closed_date = <Date 2011-10-29.15:57:50.014>
    closer = 'neologix'
    components = ['Library (Lib)']
    creation = <Date 2011-05-18.19:11:28.652>
    creator = 'socketpair'
    dependencies = []
    files = ['22031', '22032', '22078']
    hgrepos = []
    issue_num = 12105
    keywords = ['patch']
    message_count = 49.0
    messages = ['136259', '136261', '136264', '136327', '136328', '136329', '136333', '136353', '136356', '136358', '136525', '136527', '136529', '136535', '136537', '136560', '136561', '136562', '136570', '136572', '136573', '136607', '136608', '136609', '136612', '136615', '136616', '136618', '136619', '136623', '136624', '136696', '136707', '136759', '143590', '143591', '143594', '143595', '143599', '143600', '143740', '143743', '146757', '146760', '146761', '146766', '146768', '178942', '178950']
    nosy_count = 10.0
    nosy_names = ['georg.brandl', 'amaury.forgeotdarc', 'pitrou', 'vstinner', 'neologix', 'socketpair', 'python-dev', 'sbt', 'petri.lehtinen', 'alexey-smirnov']
    pr_nums = []
    priority = 'normal'
    resolution = 'duplicate'
    stage = 'resolved'
    status = 'closed'
    superseder = '12797'
    type = 'behavior'
    url = 'https://bugs.python.org/issue12105'
    versions = ['Python 3.3']

    @socketpair
    Copy link
    Mannequin Author

    socketpair mannequin commented May 18, 2011

    In Python3 I'm not able to use open('xxx', 're') as it does not call fopen() anymore. What about extra flags like 'e'=O_CLOEXEC?

    P.S.
    see bpo-12103 for list of needed flags

    P.P.S
    does 2to3 tool properly detect such cases?

    @socketpair socketpair mannequin added type-bug An unexpected behavior, bug, or error stdlib Python modules in the Lib dir labels May 18, 2011
    @amauryfa
    Copy link
    Member

    These "needed" flags are actually nonstandard extensions of the libc fopen() function on some platforms.

    In python3, one can still use fcntl(f.fileno(), FD_SET, FD_CLOEXEC), but a "flags" parameter would be more convenient.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 18, 2011

    In python3, one can still use fcntl(f.fileno(), FD_SET, FD_CLOEXEC)

    Note that it's not atomic.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 19, 2011

    Here's a patch adding O_CLOEXEC to the os module, with test. This patch makes it possible to open and set a FD CLOEXEC atomically.
    O_CLOEXEC is part of POSIX.1-2008, supported by the Linux kernel since 2.6.23 and has been committed recently to FreeBSD.
    Note that I'm not sure that adding this flag to built-in open() is necessarily a good idea, because it's not portable and low-level.
    The same functionality can be more or less achieved with:
    f = os.fdopen(os.open('/etc/fstab', os.O_RDONLY|os.O_CLOEXEC))

    @vstinner
    Copy link
    Member

    Using spawn_python() to check that os.O_CLOEXEC flag is correctly set seems overkill. Why not just testing fcntl.fcntl(f.fileno(), fcntl.F_GETFL) & FD_CLOEXEC)? I don't think that there are OSes with O_CLOEXEC but without fcntl(F_GETFL).

    Note that I'm not sure that adding this flag to built-in open()
    is necessarily a good idea

    I agree.

    open() documentation may explain the os.fdopen(os.open()) "trick" to use low-level options like O_SYNC or O_CLOEXEC.

    @amauryfa
    Copy link
    Member

    +1 for the patch!
    Note that fdopen is now a simple call to open(), so theses lines are equivalent:

    python2.7: open(filename, 're')
    python3.3: open(os.open(filename, os.O_RDONLY|os.O_CLOEXEC))

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 19, 2011

    Using spawn_python() to check that os.O_CLOEXEC flag is correctly set seems
    overkill. Why not just testing fcntl.fcntl(f.fileno(), fcntl.F_GETFL) &
    FD_CLOEXEC)?

    Because I couldn't find a place where the CLOEXEC flag was fully
    tested (I mean, checking in the child process that the FD was
    correctly closed), so I took the opportunity to test it thoroughly
    here.
    But you're right it's maybe a little bit overkill, so here's a patch
    using just F_GETFL. Pick up whichever you like.

    > Note that I'm not sure that adding this flag to built-in open()
    > is necessarily a good idea

    I agree.

    OK.

    open() documentation may explain the os.fdopen(os.open()) "trick" to use
    low-level options like O_SYNC or O_CLOEXEC.

    Why not, but I leave it to someone more comfortable with documentation
    than me :-)

    @socketpair
    Copy link
    Mannequin Author

    socketpair mannequin commented May 20, 2011

    Why not to implement 'e' letter in py3k ?

    In systems where O_CLOEXEC is not supported in open(), flag should be set non-atomically using fcntl.

    To exclude races (in concurrent threads), this two ops should be done under lock (GIL?)

    @vstinner
    Copy link
    Member

    Why not to implement 'e' letter in py3k ?

    In systems where O_CLOEXEC is not supported in open(), flag should be set non-atomically using fcntl.

    Having an atomic or non-atomic behaviour depending on the OS is not a
    good idea.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 20, 2011

    To exclude races (in concurrent threads), this two ops should be done under lock (GIL?)

    That won't work, because open(), like other slow syscalls, is called without the GIL held. Furthermore, it wouldn't be atomic anyway (imagine a fork is done from a C extension without the GIL held).
    So we would end up tricking people into using a 'e' flag that, contrarily to GNU fopen(), would not be atomic.
    Since the fopen() 'e' flag is only available on platform supporting O_CLOEXEC, you're exactly as portable using the fdopen() trick. And you're sure of what's happening.

    @vstinner
    Copy link
    Member

    @charles-francois.natali: Your patch is ok, you can commit it into 3.1, 3.2, 3.3. But you may wait after 3.2.1.

    @georg Brandl: Should we wait after Python 3.2.1 for this issue?

    @vstinner
    Copy link
    Member

    > open() documentation may explain the os.fdopen(os.open()) "trick"
    > to use low-level options like O_SYNC or O_CLOEXEC.

    Why not, but I leave it to someone more comfortable with
    documentation than me :-)

    Issue bpo-12103 should be fine for this idea.

    @vstinner
    Copy link
    Member

    Oh, by the way: it would also be nice to add os.O_CLOEXEC to Python 2.7.

    For example, tempfile._mkstemp_inner() uses:
    fd = _os.open(file, flags, 0600)
    _set_cloexec(fd) # fcntl(fd, F_SETFD, flags | FD_CLOEXEC)
    which is not atomic.

    @birkenfeld
    Copy link
    Member

    One moment -- adding a new value to the os module looks like a new feature to me. Is there any convincing reason why this needs to go to 3.2? (And it most definitely shouldn't go to 3.1.)

    @pitrou
    Copy link
    Member

    pitrou commented May 22, 2011

    No reason. I think this is definitely 3.3 material.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 22, 2011

    New changeset f1c544245eab by Charles-François Natali in branch 'default':
    Issue bpo-12105: Add O_CLOEXEC to the os module.
    http://hg.python.org/cpython/rev/f1c544245eab

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 22, 2011

    I've committed the patch to 3.3.
    Since the documentation aspect is traced in Issue bpo-12103, I'm closing this issue.
    Марк, thanks for reporting this!

    @neologix neologix mannequin closed this as completed May 22, 2011
    @pitrou
    Copy link
    Member

    pitrou commented May 22, 2011

    And apparently some buildbot doesn't like it:
    http://www.python.org/dev/buildbot/all/builders/x86%20Gentoo%20Non-Debug%203.x/builds/57/

    ======================================================================
    FAIL: test_oscloexec (test.test_posix.PosixTester)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/var/lib/buildslave/3.x.murray-gentoo-wide/build/Lib/test/test_posix.py", line 315, in test_oscloexec
        self.assertTrue(fcntl.fcntl(fd, fcntl.F_GETFD) & fcntl.FD_CLOEXEC)
    AssertionError: 0 is not true

    @vstinner
    Copy link
    Member

    One moment -- adding a new value to the os module looks like a new
    feature to me. Is there any convincing reason why this needs to go to
    3.2? (And it most definitely shouldn't go to 3.1.)

    Python doesn't suppose atomic open+CLOEXEC anymore, I consider this as a
    regression from Python 2 (which support open("re") with the GNU libc).
    Because the patch is simple, I think that it can go in 3.1 and 3.2. Am I
    wrong? But... it tooks some years until someone noticed this regression.

    Can we add new features to old releases?

    @pitrou
    Copy link
    Member

    pitrou commented May 22, 2011

    Python doesn't suppose atomic open+CLOEXEC anymore, I consider this as a
    regression from Python 2 (which support open("re") with the GNU libc).

    It has never been documented (nor supported) so, no, I wouldn't consider
    it a regression.

    But... it tooks some years until someone noticed this regression.

    Which means it's certainly unimportant.

    Can we add new features to old releases?

    Well, you already know the answer, don't you? :)

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 22, 2011

    And apparently some buildbot doesn't like it

    Linux-2.6.22-vs2.2.0.7-gentoo-i686-Intel-R-_Xeon-TM-_CPU_2.80GHz-with-gentoo-2.0.1 little-endian

    O_CLOEXEC support was added to Linux 2.6.23: this means that the libc defines it while the kernel doesn't support it (it's probably defined as 0).
    I could either add some voodoo configure checks to ensure that O_CLOEXEC is indeed supported, or just skip the test on Linux kernels older than 2.6.23. I prefer the later option, it's much simpler and less error-prone. Sure, it will make O_CLOEXEC available while the kernel doesn't support it on some configurations, but since it's a libc issue...

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 23, 2011

    New changeset bff9265d677d by Victor Stinner in branch 'default':
    Issue bpo-12105: test_posix, add the value of O_CLOEXEC in the error message
    http://hg.python.org/cpython/rev/bff9265d677d

    @vstinner
    Copy link
    Member

    or just skip the test on Linux kernels older than 2.6.23

    I like this solution, but I don't know how to test that the kernel doesn't support O_CLOEXEC. My commit bff9265d677d will tell use the value of O_CLOEXEC on the "Linux-2.6.22-vs2.2.0.7-gentoo-i686-Intel-R-_Xeon-TM-_CPU_2.80GHz-with-gentoo-2.0.1 little-endian" buildbot.

    @pitrou
    Copy link
    Member

    pitrou commented May 23, 2011

    I like this solution, but I don't know how to test that the kernel
    doesn't support O_CLOEXEC. My commit bff9265d677d will tell use the
    value of O_CLOEXEC on the
    "Linux-2.6.22-vs2.2.0.7-gentoo-i686-Intel-R-_Xeon-TM-_CPU_2.80GHz-with-gentoo-2.0.1 little-endian" buildbot.

    If O_CLOEXEC is a #defined constant in the glibc, it can't be different
    from other kernels.

    @vstinner
    Copy link
    Member

    Story of O_CLOEXEC in the GNU libc, by Ulrich Drepper: "Secure File Descriptor Handling"
    http://udrepper.livejournal.com/20407.html

    --

    I could either add some voodoo configure checks to ensure
    that O_CLOEXEC is indeed supported

    Hum, if I patch tempfile._mkstemp_inner() to use os.O_CLOEXEC if available, whereas this flag has no effect, the function will be less secure on Linux 2.6.22 (if the GNU libc exposes O_CLOEXEC).

    Check O_CLOEXEC is configure is not safe if the host compiling Python uses a different kernel that the host running Python (e.g. think of Linux distro: Python is compiled on a different computer).

    We need maybe a flag to indicate that O_CLOEXEC is supported by the running kernel or not. Or maybe a function to check it at least?

    --

    O_CLOEXEC was added to Ruby and then removed because the flag is sometimes ignored:
    http://redmine.ruby-lang.org/issues/1291
    http://redmine.ruby-lang.org/projects/ruby-19/repository/revisions/31643

    "because boron chkbuild test result says, An old linux kernel ignore O_CLOEXEC silently instead of return an error. It may lead to bring new security risk. So, we have to be pending it until finish to implement proper fallback logic."

    --

    Read also the discussion about O_CLOEXEC on bugs-findutils mailing list:
    "it's not a guarantee that the O_CLOEXEC actually had an effect"

    Their patch uses :

    static int
    fd_is_cloexec (int fd)
    {
      const int flags = fcntl (fd, F_GETFD);
      return (flags & FD_CLOEXEC) || (fcntl (fd, F_GETFL) & O_CLOEXEC);
    }
    
    static bool
    o_cloexec_works (void)
    {
      bool result = false;
      int fd = open ("/", O_RDONLY|O_CLOEXEC);
      if (fd >= 0)
        {
          result = fd_is_cloexec (fd);
          close (fd);
        }
      return result;
    }

    --

    Oh, there is also SOCK_CLOEXEC, which may be useful for bpo-5715.

    @vstinner
    Copy link
    Member

    Another idea is to write a best-effort function to open a file with CLOEXEC flag:

    • use O_CLOEXEC if available
    • use fcntl(fd, F_SETFD, flags | FD_CLOEXEC) if O_CLOEXEC is missing or was silently ignored by the kernel (by open)

    Attached open_cloexec.py is an implementation.

    --

    Usage of "CLOEXEC" in the Python standard library:

    • subprocess: create pipe. use pipe2() or pipe()+fcntl(FD_CLOEXEC)
    • test_socket: create a socket. use SOCK_CLOEXEC. The test is skipped if the kernel is Linux < 2.6.28. It has a nice linux_version() which should be moved to the platform module.
    • test_posix: check open(O_CLOEXEC).
    • test_tempfile: test "cloexec" behaviour using os.spawnl()
    • xmlrpclib: use FD_CLOEXEC on the socket

    You may also add pipe_cloexec() to os, and socket_cloexec() to socket?

    @vstinner
    Copy link
    Member

    My commit bff9265d677d will tell use the value of O_CLOEXEC on the
    "Linux-2.6.22-vs2.2.0.7-gentoo-..." buildbot.

    Here you have:
    "AssertionError: 0 is not true : CLOEXEC flag not set (O_CLOEXEC=0x80000)"

    It's the same value on my Debian Sid. So we cannot test O_CLOEXEC value, we have to check the kernel version.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 23, 2011

    The real issue is that the libc defines O_CLOEXEC, but kernels prior to 2.6.23 don't support it: instead of returning EINVAL, the socket syscall silently ignores the flag (don't know why I made the comment about this flag being defined to 0...).
    IMHO this is really a distribution packaging bug (kernel/libc mismatch), and a quite serious one.
    I don't think we should be fixing this in Python, but that we should merely skip this test on kernels older than 2.6.23.
    People should complain to their distributions vendors instead (I tested this on RHEL4, RHEL6 and Debian Squeeze without problem).

    I personally don't like the idea of a best-effort O_CLOEXEC implementation: providing a O_CLOEXEC flag which is not atomic feels really wrong to me.

    @vstinner
    Copy link
    Member

    The real issue is that the libc defines O_CLOEXEC, but kernels prior
    to 2.6.23 don't support it: instead of returning EINVAL, the socket
    syscall silently ignores the flag (don't know why I made the comment
    about this flag being defined to 0...).

    This is a kernel bug, not a bug in the GNU libc (ask Ulrich if you are not sure ;-)). An host can have multiple kernel versions (and choose at boot time using GRUB/LILO/...), but it has usually only one C library. You cannot change the version of the libc depending on the kernel.

    If you really want to fix this problem, you will have to patch kernel < 2.6.23. Good luck!

    Or we can workaround kernel bugs providing a documentation and/or functions for that.

    @vstinner
    Copy link
    Member

    test_socket: ...has a nice linux_version() which should be moved to
    the platform module

    I created the issue bpo-12158 for that.

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 23, 2011

    This is a kernel bug, not a bug in the GNU libc (ask Ulrich if you are not sure ;-)).

    Kernels prior to 2.6.23 didn't know about the O_CLOEXEC flag: to catch this kind of problem, every syscall would have to check every bit when it's passed a combination of flags. This would be clumsy, error-prone and slow.
    It's not a libc bug either.
    The problem is really a distribution issue: using a libc defining a flag unsupported by the kernel is really calling for trouble.

    An host can have multiple kernel versions (and choose at boot time using GRUB/LILO/...)

    It's possible, but it's definitely a bad idea, because of such API mismatch. For example nothing prevents a syscall from being removed/modified from one kernel version to another. If your libc doesn't follow, you're up for trouble.
    Try using futexes with a kernel not supporting them :-)

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 23, 2011

    @victor
    Since linux_version() won't be added to the platform module, could you add it to test.support so that it can be used in the O_CLOEXEC test?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 23, 2011

    New changeset 9a16fa0c9548 by Victor Stinner in branch 'default':
    Issue bpo-12105: test_posix skips test_oscloexec() on Linux < 2.6.23
    http://hg.python.org/cpython/rev/9a16fa0c9548

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented May 24, 2011

    Checking the kernel version did the trick, the test now run fines on the buildbots.
    Thanks Victor.
    Re-closing.

    @neologix neologix mannequin closed this as completed May 24, 2011
    @socketpair
    Copy link
    Mannequin Author

    socketpair mannequin commented Sep 6, 2011

    O_CLOEXEC is not linux-only. Windows has the same flag. In file-opening functions there is lpSecurityAttributes argument. And there is bInheritHandle member of corresponding structure.

    http://msdn.microsoft.com/en-us/library/aa379560(v=VS.85).aspx :
    bInheritHandle
    A Boolean value that specifies whether the returned handle is inherited when a new process is created. If this member is TRUE, the new process inherits the handle.

    So, why not to implement 'e' letter in open()? it is true crossplatform. On platforms where inheritance does not work, flag should be ignored.

    P.S. Moreover, I think that all file-descriptor-crete functions (open, socket, pipe, dup, ...) should set CLOEXEC atomically

    @socketpair socketpair mannequin reopened this Sep 6, 2011
    @alexey-smirnov
    Copy link
    Mannequin

    alexey-smirnov mannequin commented Sep 6, 2011

    FreeBSD 8+ also supports O_CLOEXEC in open().

    @amauryfa
    Copy link
    Member

    amauryfa commented Sep 6, 2011

    O_CLOEXEC is not linux-only. Windows has the same flag.
    In file-opening functions there is lpSecurityAttributes argument

    How do you suggest to use it? Even on Windows, python calls open(). And lpSecurityAttributes is an argument of CreateFile (which is the win32 kernel function that open() calls)

    @vstinner
    Copy link
    Member

    vstinner commented Sep 6, 2011

    Windows provides a _get_osfhandle() function. There is not the opposite function? :-)

    Anyway, O_CLOEXEC is not available on all platforms. Even on FreeBSD and Linux, it depends on the OS/kernel version.

    @socketpair
    Copy link
    Mannequin Author

    socketpair mannequin commented Sep 6, 2011

    Some times ago, Python has used fopen() for open() implementation. Now, it uses OS-kernel native function to open files. AFAIK, open() in Windows is a wrapper around CreateFile, created to support some POSIX programs in Windows. Why not to use CreateFile() on Windows platform?

    @amauryfa
    Copy link
    Member

    amauryfa commented Sep 6, 2011

    Why not to use CreateFile() on Windows platform?
    Good idea! Please open a separate issue for it.

    @vstinner
    Copy link
    Member

    vstinner commented Sep 8, 2011

    See also issue bpo-12760 (Add create mode to open).

    @vstinner
    Copy link
    Member

    vstinner commented Sep 8, 2011

    > Why not to use CreateFile() on Windows platform?
    Good idea! Please open a separate issue for it.

    Done, issue bpo-12939.

    @neologix neologix mannequin closed this as completed Oct 29, 2011
    @socketpair
    Copy link
    Mannequin Author

    socketpair mannequin commented Nov 1, 2011

    Why it is closed as duplicate? nothing said about CLOEXEC in bpo-12797

    @neologix
    Copy link
    Mannequin

    neologix mannequin commented Nov 1, 2011

    Why it is closed as duplicate? nothing said about CLOEXEC in bpo-12797

    See http://bugs.python.org/issue12760#msg146686

    @amauryfa
    Copy link
    Member

    amauryfa commented Nov 1, 2011

    And to be explicit, you can now write:

    def open_cloexex(filename, mode='r'):
      return open(filename, mode,
                  opener=lambda path, mod: os.open(path, mod|os.O_CLOEXEC))

    @socketpair
    Copy link
    Mannequin Author

    socketpair mannequin commented Nov 1, 2011

    Well, I understand. So why not to add 'e' (and 'N', which is the same meaning) character, which:

    • use O_CLOEXEC in modern Linux
    • generate Exception if O_CLOEXEC is not supported (or does not work) on platform.

    Also, implement O_CLOEXEC for open() in Windows using appropriate securityattributes with CreateFile()

    ?

    @amauryfa
    Copy link
    Member

    amauryfa commented Nov 1, 2011

    So why not to add 'e' character
    You said it: because it can't be written consistently on all platforms.
    For example, python does not use CreateFile on Windows, see bpo-12939.

    @sbt
    Copy link
    Mannequin

    sbt mannequin commented Jan 3, 2013

    Note that on Windows there is an O_NOINHERIT flag which almost corresponds to O_CLOEXEC on Linux.

    I don't think there is a need to use the win32 api.

    @vstinner
    Copy link
    Member

    vstinner commented Jan 3, 2013

    Note that on Windows there is an O_NOINHERIT flag which
    almost corresponds to O_CLOEXEC on Linux.
    I don't think there is a need to use the win32 api.

    Ah yes. Because this issue is closed, I created the issue bpo-16850 which is more specific to open + close-and-exec.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants