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

create Python wrappers for openat() and others #49011

Closed
pitrou opened this issue Dec 28, 2008 · 21 comments
Closed

create Python wrappers for openat() and others #49011

pitrou opened this issue Dec 28, 2008 · 21 comments
Labels
extension-modules C modules in the Modules dir stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@pitrou
Copy link
Member

pitrou commented Dec 28, 2008

BPO 4761
Nosy @loewis, @birkenfeld, @pitrou, @giampaolo, @tiran, @benjaminp, @durban, @anacrolix, @serhiy-storchaka
Files
  • i4761.patch: Adds *at family of functions to the posix module.
  • i4761_v2.patch: Updated patch
  • i4761_v3.patch: Patch with doc update
  • i4761_v4.patch: Updated ref coutning, posix_error & version tags
  • i4761_v5.patch: Update doc
  • i4761_v6.patch: Fixed small #ifdef error with fstatat.
  • 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 2012-03-10.18:31:45.280>
    created_at = <Date 2008-12-28.14:58:38.326>
    labels = ['extension-modules', 'type-feature', 'library']
    title = 'create Python wrappers for openat() and others'
    updated_at = <Date 2012-03-10.22:24:20.044>
    user = 'https://github.com/pitrou'

    bugs.python.org fields:

    activity = <Date 2012-03-10.22:24:20.044>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = True
    closed_date = <Date 2012-03-10.18:31:45.280>
    closer = 'benjamin.peterson'
    components = ['Extension Modules', 'Library (Lib)']
    creation = <Date 2008-12-28.14:58:38.326>
    creator = 'pitrou'
    dependencies = []
    files = ['20130', '20133', '20135', '20137', '20138', '20460']
    hgrepos = []
    issue_num = 4761
    keywords = ['patch']
    message_count = 21.0
    messages = ['78407', '78430', '78431', '78433', '124432', '124444', '124453', '124457', '124487', '124497', '124498', '124501', '124503', '126594', '129469', '129545', '129548', '155325', '155327', '155340', '155358']
    nosy_count = 11.0
    nosy_names = ['loewis', 'georg.brandl', 'pitrou', 'giampaolo.rodola', 'christian.heimes', 'benjamin.peterson', 'daniel.urban', 'anacrolix', 'Chris.Gerhard', 'rosslagerwall', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue4761'
    versions = ['Python 3.3']

    @pitrou
    Copy link
    Member Author

    pitrou commented Dec 28, 2008

    Very recent POSIX versions have introduced a set of functions named
    openat(), unlinkat(), etc. (*) which allow to access files relatively to
    a directory pointed to by a file descriptor (rather than the
    process-wide current working directory). They are necessary to implement
    thread-safe directory traversal without any symlink attacks such as in
    bpo-4489. Providing Python wrappers for these functions would help creating
    higher-level abstractions for secure directory traversal on platforms
    that support it.

    (*) http://www.opengroup.org/onlinepubs/9699919799/functions/openat.html

    “The purpose of the openat() function is to enable opening files in
    directories other than the current working directory without exposure to
    race conditions. Any part of the path of a file could be changed in
    parallel to a call to open(), resulting in unspecified behavior. By
    opening a file descriptor for the target directory and using the
    openat() function it can be guaranteed that the opened file is located
    relative to the desired directory.”

    @pitrou pitrou added extension-modules C modules in the Modules dir stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Dec 28, 2008
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Dec 28, 2008

    There is a tradition that any POSIX calls are added to the POSIX module
    without much discussion, provided that
    a) there is an autoconf test testing for their presence, and
    b) they expose the API as-is, i.e. without second-guessing the designers
    of the original API.

    So contributions are welcome.

    @tiran
    Copy link
    Member

    tiran commented Dec 28, 2008

    The openat() functions sound useful indeed. However I'm concerned about
    the file descriptor requirment for the *at() POSIX functions. In Python
    file descriptors can lead to resource leaks because developers are used
    to automatic garbage collection. os.open() is the only way to get a file
    descriptor to a directory. The builtin open() doesn't open directories.
    Developers may think that the file descriptor is closed when the integer
    object gets out of scope.

    I propose the addition of opendir() for the purpose of the *at()
    functions. The opendir function should return a wrapper object around
    the DIR* pointer returned by opendir(). A function fileno() exposed the
    file descriptor of the DIR pointer via dirfd().

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Dec 28, 2008

    Developers may think that the file descriptor is closed when the integer
    object gets out of scope.

    I'm not concerned about that. When they use os.open, they will typically
    understand what a file handle is, and how it relates to

    I propose the addition of opendir() for the purpose of the *at()
    functions. The opendir function should return a wrapper object around
    the DIR* pointer returned by opendir(). A function fileno() exposed the
    file descriptor of the DIR pointer via dirfd().

    -1. This is exactly the second-guessing kind of thing that the POSIX
    module should avoid. openat(2) doesn't expect DIR objects, and neither
    should posix.openat.

    If desired, a layer can be added on top of this that makes it more
    "safe", e.g. in the os module; such a layer should then try to make it
    cross-platform before trying to make it safe.

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Dec 21, 2010

    Attached is a patch that adds:
    faccessat, fchmodat, fchownat, fstatat, futimesat, linkat, mkdirat, mknodat, openat, readlinkat, renameat, symlinkat, unlinkat, utimensat and mkfifoat.

    Each function has documentation and a unit test and is conditionally included only if the functions exist using autoconf testing. Most of the code for the functions and unit tests was taken from the corresponding non-at versions.

    Tested on Linux 2.6.35 and FreeBSD 8.1 (although FreeBSD 8.1 does not have utimensat).

    This should then allow a patch for bpo-4489 to be created.

    @pitrou
    Copy link
    Member Author

    pitrou commented Dec 21, 2010

    Thanks for the patch. A couple of comments:

    • the C code is misindented in some places (using 8 spaces rather than 4)
    • you should use support.unlink consistently in the tests (rather than sometimes os.unlink or posix.unlink)
    • when cleaning up in tests (through unlink() or rmdir()), it's better to use finally clauses so that cleaning up gets done even on error; or, alternatively, to use self.addCleanup() (see http://docs.python.org/dev/library/unittest.html#unittest.TestCase.addCleanup)

    (I haven't looked at the C code in detail since you say it's mostly copy/paste from existing code)

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Dec 21, 2010

    Attached is an updated patch which:

    • fixes badly indented C code
    • uses support.unlink consistently
    • cleans up tests better using finally

    @benjaminp
    Copy link
    Contributor

    The docs shouldn't use "[" to denote optional args. Rather, optional arguments can just be shown by their defaults.

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Dec 22, 2010

    Ok, attached is a patch with the documentation updated as per recommendation.

    @birkenfeld
    Copy link
    Member

    Reference counting is not always correct. For example, in unlinkat

        if (res < 0)
            return posix_error();
        Py_DECREF(opath);
        (return None)

    the DECREF should be before the error check. (Note that you can use the Py_RETURN_NONE macro to save INCREF'ing Py_None explicitly.)

    Sometimes you use posix_error, sometimes posix_error_with_allocated_filename; is that deliberate?

    Also, the documentation for each function should get ".. versionadded:: 3.3" tags.

    Otherwise, this looks good and ready for inclusion when py3k is open for new features again.

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Dec 22, 2010

    New patch *should* have fixed up reference counting and version tags.

    I standardized all the error calls to posix_error.

    @birkenfeld
    Copy link
    Member

    Thanks for the update! Three more comments:

    • the new constants doc should also get a versionadded

    • faccessat should check for EBADF, EINVAL and ENOTDIR and raise an error if they are returned, since these are input errors

      Or, alternately, a range of errnos should be whitelisted for both access and faccessat.

      However, this problem should be handled in a separate issue, I've opened bpo-10758 for that.

    • The octal modes in docstrings and docs should be specified in Python 3 octal syntax, e.g. 0o777.

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Dec 22, 2010

    This new patch has proper octal mode strings and another doc update.

    I'll leave faccessat until bpo-10758 has been resolved.

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Jan 20, 2011

    Fixed small #ifdef error with fstatat.

    @pitrou
    Copy link
    Member Author

    pitrou commented Feb 25, 2011

    I have committed the patch in r88624. Thank you!

    @pitrou pitrou closed this as completed Feb 25, 2011
    @giampaolo
    Copy link
    Contributor

    What about Doc/whatsnew/3.3.rst?

    @pitrou
    Copy link
    Member Author

    pitrou commented Feb 26, 2011

    What about Doc/whatsnew/3.3.rst?

    This is filled by Raymond (or other people) when the release nears.
    No need to care about it in regular commits.

    @serhiy-storchaka
    Copy link
    Member

    Why use a special value AT_FDCWD instead of None? It is not Pythonish. Clearly, when it is used in C, but in dynamically typed Python we are not limited to only one C-type.

    Such a large number of new functions littering the namespace. Why not just add additional arguments to existing functions? Instead 'fstatat(dirfd, path, flags=0)' let it be 'stat(path, *, dirfd=None, flags=0)' or even better 'stat(path, *, dirfd=None, followlinks=True)' (as in os.fwalk).

    @birkenfeld
    Copy link
    Member

    I agree about the constant AT_FDCWD. (At least, None should be allowed in addition.)

    Your other proposition would break the principle of very thin platform wrappers that we try to follow in posixmodule.c.

    @birkenfeld birkenfeld reopened this Mar 10, 2012
    @benjaminp
    Copy link
    Contributor

    Addition/Substitution of None I think should be in a new issue.

    @serhiy-storchaka
    Copy link
    Member

    Perhaps it is better not export these functions in the os module, leaving them in posix?

    In addition, stat and lstat is not very thin wrappers (especially on Windows) given that they are working with bytes and with str.

    @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
    extension-modules C modules in the Modules dir stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants