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
32-bits unsigned user/group identifier #48841
Comments
On Fedora 8 and 10 using Python 2.5.1 and 2.5.2 (64 bit): $ grep nfsnobody /etc/passwd
nfsnobody:x:4294967294:4294967294:Anonymous NFS
User:/var/lib/nfs:/sbin/nologin So the UID of nfsnobody is 4294967294 (-2 if viewed as signed 32-bit int). >>> import pwd, os
>>> print pwd.getpwnam('nfsnobody').pw_uid
4294967294
>>> os.chown('some file', pwd.getpwnam('nfsnobody').pw_uid,
pwd.getpwnam('nfsnobody').pw_gid)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
OverflowError: signed integer is greater than maximum The reason for this error is that os.chown uses the "i" format to The bug does not occur on 32 bit Fedora since there the uid and gid of |
This was already corrected by r61540, and will be released with 2.5.3 BUT -- fchown() was not modified. patch is similar to r61540, but I Index: posixmodule.c --- posixmodule.c (revision 67068)
+++ posixmodule.c (working copy)
@@ -1902,9 +1902,10 @@
static PyObject *
posix_fchown(PyObject *self, PyObject *args)
{
- int fd, uid, gid;
+ int fd;
+ long uid, gid;
int res;
- if (!PyArg_ParseTuple(args, "iii:chown", &fd, &uid, &gid))
+ if (!PyArg_ParseTuple(args, "ill:fchown", &fd, &uid, &gid))
return NULL;
Py_BEGIN_ALLOW_THREADS
res = fchown(fd, (uid_t) uid, (gid_t) gid); |
2^31-2 doesn't fit in long on 32 bits CPU. On my Linux box, uid_t is an int chown(const char *path, uid_t owner, gid_t group); |
I'm sure you meant 2^32-2 ;-). The "fix" to use long doesn't seem right to me either. unsigned int is |
There is not only chown! There are also lchown(), fchown(), stat() and Attached patch:
Note: *chown() accepts -1 for an argument, eg. chown("file", 10, -1) |
I tested my patch (for Python trunk) on Ubuntu Gutsy with a 32 bits |
Anyone available for a patch review? Do you need anything else: tests, |
The documentation for PyArg_ParseTuple states that the "I" code doesn't |
pitrou> The documentation for PyArg_ParseTuple states that the I'm sorry, but you're right :-) My patch was wrong. New patch |
Oops, i forgot to ignore space changes in my diff... new patch. |
New patch using PyNumber_Index()+PyLong_AsLong() for parse_uid() and Changes between python trunk and posix_unsigned_uid-3.patch:
|
I'm not sure hardcoding "2^32 - 1" in the error message is a good idea. I also think it would be nice to have tests for the "-1" special value:
|
See also bpo-3586: "pwd.getpwnam('nobody') produces incorrect results if |
Here is a simplified version of patch from bpo-2005. Support of uid_t/gid_t larger than unsigned long dropped. Signed uid_t/gid_t support dropped. The converters moved to Objects/longobject.c. Some tests added. |
I don't think _Py_Uid_Converter and _Py_Gid_Converter belong in Objects/longobject.c. Is there a better place to put these? (Though _PyLong_FromUid and _PyLong_FromGid seem fine.) |
Hmm. I take that back. I don't think any of this really belongs in Objects/longobject.c. Right now that module contains entirely portable C code that knows almost nothing about operating systems. In particular, the knowledge that -1 has special meaning doesn't really have a place in the longobject implementation. Is it possible to just use PyLong_FromLong / PyLong_FromLongLong etc. depending on the precision of uid_t / gid_t? |
I am also not happy with that. Originally they were in Modules/posixmodule.c. However where should we place the declarations? We have not posixmodule.h. PyLong_FromPid()/PyLong_AsPid() placed right in Include/longobject.h. Are you have any suggestions?
No. uid_t/gid_t can be large than maximal signed long, and long long can be not defined. See bpo-2005 where more complex patch proposed. |
Perhaps we should add a new header file for declarations of converters to/from all platform specific integers (pid_t, time_t, uid_t, gid_t). |
Do we know of any systems where none of int, long, long long (if defined) or their unsigned variants match gid_t / uid_t? |
A new header file sounds reasonable. We could then move the PID stuff out of longobject.h |
No, I don't. |
The value -1 is used is some functions like chown() (to only change the
|
Patch updated. The converters declaration moved to the new file Modules/posixmodule.h (Modules/grpmodule.c, Modules/posixmodule.c, Modules/pwdmodule.c, and Modules/signalmodule.c use it) and the implementation moved to Modules/posixmodule.c. |
Yes, there is a special case for -1. Look at the code. |
Are there any objections, nitpicks or suggestions? |
No objections here. |
New changeset b322655a4a88 by Serhiy Storchaka in branch '3.3': New changeset 94256de0aff0 by Serhiy Storchaka in branch 'default': |
Looks like this broke some buildbots: ====================================================================== Traceback (most recent call last):
File "/usr/home/buildbot/buildarea/3.3.krah-freebsd/build/Lib/test/test_shutil.py", line 1209, in test_chown
shutil.chown(filename, 3.14)
File "/usr/home/buildbot/buildarea/3.3.krah-freebsd/build/Lib/shutil.py", line 1024, in chown
os.chown(path, _user, _group)
PermissionError: [Errno 1] Operation not permitted: '/tmp/tmp3f82m0/tmp_ttyx5' |
New changeset 4ef048f4834e by Serhiy Storchaka in branch '3.3': New changeset 3fdcffdfd3e6 by Serhiy Storchaka in branch 'default': |
Not technically the topic of this issue, but should we just lock This is still accepted: os.setuid(Decimal("1000.2")) |
The right way to do it should be to use PyNumber_Index(). (perhaps we need as PyNumber_AsLongIndex() to do the direct conversion |
+1, but drop the 'Index' part of the name, to match the existing PyNumber_AsSsize_t. |
There's already PyNumber_Long() (which doesn't use __index__), hence the |
Hmm; good point. Well, +1 to the functionality, anyway; I'll leave the discussion about the name. |
There is a problem with test_pwd on 64-bit platform. It expects KeyError on pwd.getpwuid(sys.maxsize). Actually the test is not looks robust. On 32-bit platform sys.maxsize = 2**31-1 < 2**32 and this value only by chance was not in the user database. On 64-bit platform sys.maxsize > 2**32 and in old code it was wrapped to 2**31-1 C unsigned int value and this value only by chance was not in the user database. New code doesn't wrap the value to C unsigned int and raises an overflow exception. What should I change, a test or a raised exception? |
+1 for PyIndex_AsLong() |
In this case we need PyNumber_AsLongAndOverflowIndex() and PyNumber_AsUnsignedLongIndex(). And a lot of other parallel functions for other cases. This can exponentially increase a number of functions. |
I just copied this code from PyArg_ParseTuple*() for 'l' format.
Any C implemented function which parses an integer argument with PyArg_ParseTuple*() accepts decimals. >>> chr(65.2)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: integer argument expected, got float
>>> chr(Decimal('65.2'))
'A' I think this is an offtopic for this issue. |
Serhiy Storchaka <report@bugs.python.org> wrote:
I know that this behavior wasn't introduced by you. It's perfectly fine
I just happen to think that PyArg_ParseTuple() is wrong here and we shouldn't |
Fixed in changesets a0983e46feb1 and 1e9fa629756c: Raise KeyError instead of OverflowError when getpwuid's argument is out of uid_t range. |
I don't think it will be exponential :-) |
New changeset 3893ab574c55 by Serhiy Storchaka in branch '3.2': New changeset 035cbc654889 by Serhiy Storchaka in branch '2.7': |
Revision 035cbc654889 added useless '#ifndef Py_LIMITED_API' check in 2.7 branch. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: