create Python wrappers for openat() and others

[pitrou]

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]

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.”

[loewis]

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]

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]

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]

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]

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]

Attached is an updated patch which:

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

[benjaminp]

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

[rosslagerwall]

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

[birkenfeld]

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]

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

I standardized all the error calls to posix_error.

[birkenfeld]

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]

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

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

[rosslagerwall]

Fixed small #ifdef error with fstatat.

[pitrou]

I have committed the patch in r88624. Thank you!

[giampaolo]

What about Doc/whatsnew/3.3.rst?

[pitrou]

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]

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]

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.

[benjaminp]

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

[serhiy-storchaka]

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.