| msg78407 - (view) |
Author: Antoine Pitrou (pitrou) *  |
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) * |
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) * |
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) * |
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) |
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) * |
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) |
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) * |
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) |
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) * |
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) |
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) * |
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) |
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) |
Date: 2011-01-20 08:38 |
| Fixed small #ifdef error with fstatat. |
|
|
| msg129469 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-02-25 23:26 |
| I have committed the patch in r88624. Thank you! |
|
|
| msg129545 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2011-02-26 13:56 |
| What about Doc/whatsnew/3.3.rst? |
|
|
| msg129548 - (view) |
Author: Antoine Pitrou (pitrou) * |
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) * |
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) * |
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) * |
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) * |
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`. |
|
|