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`.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:43 | admin | set | github: 49011 |
2012-03-10 22:24:20 | serhiy.storchaka | set | messages:
+ msg155358 |
2012-03-10 18:31:45 | benjamin.peterson | set | status: open -> closed
messages:
+ msg155340 |
2012-03-10 17:49:37 | georg.brandl | set | status: closed -> open
messages:
+ msg155327 |
2012-03-10 17:11:54 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages:
+ msg155325
|
2011-02-26 14:01:17 | pitrou | set | nosy:
loewis, georg.brandl, pitrou, giampaolo.rodola, christian.heimes, benjamin.peterson, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall messages:
+ msg129548 |
2011-02-26 13:56:30 | giampaolo.rodola | set | nosy:
loewis, georg.brandl, pitrou, giampaolo.rodola, christian.heimes, benjamin.peterson, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall messages:
+ msg129545 |
2011-02-25 23:26:45 | pitrou | set | status: 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 -> resolved |
2011-01-20 21:25:44 | giampaolo.rodola | set | nosy:
+ giampaolo.rodola
|
2011-01-20 08:38:05 | rosslagerwall | set | files:
+ 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:07 | pitrou | link | issue4489 dependencies |
2010-12-22 13:10:56 | rosslagerwall | set | files:
+ 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:20 | georg.brandl | set | nosy:
loewis, georg.brandl, pitrou, christian.heimes, benjamin.peterson, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall messages:
+ msg124501 |
2010-12-22 11:53:39 | rosslagerwall | set | files:
+ 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:28 | georg.brandl | set | nosy:
+ georg.brandl messages:
+ msg124497
|
2010-12-22 04:29:38 | rosslagerwall | set | files:
+ i4761_v3.patch nosy:
loewis, pitrou, christian.heimes, benjamin.peterson, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall messages:
+ msg124487
|
2010-12-21 20:40:39 | benjamin.peterson | set | nosy:
+ benjamin.peterson messages:
+ msg124457
|
2010-12-21 20:36:25 | rosslagerwall | set | files:
+ i4761_v2.patch nosy:
loewis, pitrou, christian.heimes, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall messages:
+ msg124453
|
2010-12-21 18:28:42 | pitrou | set | nosy:
loewis, pitrou, christian.heimes, daniel.urban, anacrolix, Chris.Gerhard, rosslagerwall messages:
+ msg124444 |
2010-12-21 17:51:10 | pitrou | set | nosy:
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:08 | rosslagerwall | set | files:
+ i4761.patch
nosy:
+ rosslagerwall messages:
+ msg124432
keywords:
+ patch |
2010-12-20 12:33:43 | anacrolix | set | nosy:
+ anacrolix
|
2010-09-15 19:26:08 | daniel.urban | set | nosy:
+ daniel.urban
|
2010-09-13 14:27:03 | Chris.Gerhard | set | nosy:
+ Chris.Gerhard
|
2010-08-09 03:12:19 | terry.reedy | set | stage: test needed versions:
+ Python 3.2, - Python 3.1, Python 2.7 |
2008-12-28 21:17:29 | loewis | set | messages:
+ msg78433 |
2008-12-28 20:54:40 | christian.heimes | set | nosy:
+ christian.heimes messages:
+ msg78431 |
2008-12-28 20:26:06 | loewis | set | messages:
+ msg78430 |
2008-12-28 14:58:38 | pitrou | create | |