classification
Title: create Python wrappers for openat() and others
Type: enhancement Stage: committed/rejected
Components: Extension Modules, Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Chris.Gerhard, anacrolix, benjamin.peterson, christian.heimes, daniel.urban, georg.brandl, giampaolo.rodola, loewis, pitrou, rosslagerwall, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2008-12-28 14:58 by pitrou, last changed 2012-03-10 22:24 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
i4761.patch rosslagerwall, 2010-12-21 14:19 Adds *at family of functions to the posix module.
i4761_v2.patch rosslagerwall, 2010-12-21 20:36 Updated patch
i4761_v3.patch rosslagerwall, 2010-12-22 04:29 Patch with doc update
i4761_v4.patch rosslagerwall, 2010-12-22 11:53 Updated ref coutning, posix_error & version tags
i4761_v5.patch rosslagerwall, 2010-12-22 13:10 Update doc
i4761_v6.patch rosslagerwall, 2011-01-20 08:38 Fixed small #ifdef error with fstatat.
Messages (21)
msg78407 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-12-28 14:58
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
#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.”
msg78430 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-12-28 20:26
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.
msg78431 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-12-28 20:54
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().
msg78433 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-12-28 21:17
> 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.
msg124432 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2010-12-21 14:19
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 #4489 to be created.
msg124444 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-12-21 18:28
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)
msg124453 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2010-12-21 20:36
Attached is an updated patch which:
- fixes badly indented C code
- uses support.unlink consistently
- cleans up tests better using finally
msg124457 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2010-12-21 20:40
The docs shouldn't use "[" to denote optional args. Rather, optional arguments can just be shown by their defaults.
msg124487 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2010-12-22 04:29
Ok, attached is a patch with the documentation updated as per recommendation.
msg124497 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2010-12-22 10:10
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.
msg124498 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2010-12-22 11:53
New patch *should* have fixed up reference counting and version tags.

I standardized all the error calls to posix_error.
msg124501 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2010-12-22 12:45
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 #10758 for that.

* The octal modes in docstrings and docs should be specified in Python 3 octal syntax, e.g. 0o777.
msg124503 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2010-12-22 13:10
This new patch has proper octal mode strings and another doc update.

I'll leave faccessat until #10758 has been resolved.
msg126594 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2011-01-20 08:38
Fixed small #ifdef error with fstatat.
msg129469 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-02-25 23:26
I have committed the patch in r88624. Thank you!
msg129545 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-02-26 13:56
What about Doc/whatsnew/3.3.rst?
msg129548 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-02-26 14:01
> 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.
msg155325 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-03-10 17:11
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).
msg155327 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2012-03-10 17:49
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.
msg155340 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-03-10 18:31
Addition/Substitution of None I think should be in a new issue.
msg155358 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-03-10 22:24
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`.
History
Date User Action Args
2012-03-10 22:24:20serhiy.storchakasetmessages: + msg155358
2012-03-10 18:31:45benjamin.petersonsetstatus: open -> closed

messages: + msg155340
2012-03-10 17:49:37georg.brandlsetstatus: closed -> open

messages: + msg155327
2012-03-10 17:11:54serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg155325
2011-02-26 14:01:17pitrousetnosy: loewis, georg.brandl, pitrou, giampaolo.rodola, christian.heimes, benjamin.peterson, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall
messages: + msg129548
2011-02-26 13:56:30giampaolo.rodolasetnosy: loewis, georg.brandl, pitrou, giampaolo.rodola, christian.heimes, benjamin.peterson, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall
messages: + msg129545
2011-02-25 23:26:45pitrousetstatus: open -> closed
nosy: loewis, georg.brandl, pitrou, giampaolo.rodola, christian.heimes, benjamin.peterson, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall
messages: + msg129469

resolution: fixed
stage: patch review -> committed/rejected
2011-01-20 21:25:44giampaolo.rodolasetnosy: + giampaolo.rodola
2011-01-20 08:38:05rosslagerwallsetfiles: + i4761_v6.patch
nosy: loewis, georg.brandl, pitrou, christian.heimes, benjamin.peterson, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall
messages: + msg126594
2011-01-05 18:48:07pitroulinkissue4489 dependencies
2010-12-22 13:10:56rosslagerwallsetfiles: + i4761_v5.patch
nosy: loewis, georg.brandl, pitrou, christian.heimes, benjamin.peterson, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall
messages: + msg124503
2010-12-22 12:45:20georg.brandlsetnosy: loewis, georg.brandl, pitrou, christian.heimes, benjamin.peterson, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall
messages: + msg124501
2010-12-22 11:53:39rosslagerwallsetfiles: + i4761_v4.patch
nosy: loewis, georg.brandl, pitrou, christian.heimes, benjamin.peterson, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall
messages: + msg124498
2010-12-22 10:10:28georg.brandlsetnosy: + georg.brandl
messages: + msg124497
2010-12-22 04:29:38rosslagerwallsetfiles: + i4761_v3.patch
nosy: loewis, pitrou, christian.heimes, benjamin.peterson, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall
messages: + msg124487
2010-12-21 20:40:39benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg124457
2010-12-21 20:36:25rosslagerwallsetfiles: + i4761_v2.patch
nosy: loewis, pitrou, christian.heimes, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall
messages: + msg124453
2010-12-21 18:28:42pitrousetnosy: loewis, pitrou, christian.heimes, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall
messages: + msg124444
2010-12-21 17:51:10pitrousetnosy: loewis, pitrou, christian.heimes, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall
stage: test needed -> patch review
versions: + Python 3.3, - Python 3.2
2010-12-21 14:19:08rosslagerwallsetfiles: + i4761.patch

nosy: + rosslagerwall
messages: + msg124432

keywords: + patch
2010-12-20 12:33:43anacrolixsetnosy: + anacrolix
2010-09-15 19:26:08daniel.urbansetnosy: + daniel.urban
2010-09-13 14:27:03Chris.Gerhardsetnosy: + Chris.Gerhard
2010-08-09 03:12:19terry.reedysetstage: test needed
versions: + Python 3.2, - Python 3.1, Python 2.7
2008-12-28 21:17:29loewissetmessages: + msg78433
2008-12-28 20:54:40christian.heimessetnosy: + christian.heimes
messages: + msg78431
2008-12-28 20:26:06loewissetmessages: + msg78430
2008-12-28 14:58:38pitroucreate