classification
Title: 32-bits unsigned user/group identifier
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.3, Python 3.2, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, haypo, pitrou, sjoerd
Priority: normal Keywords: needs review, patch

Created on 2008-12-08 12:22 by sjoerd, last changed 2011-06-26 19:33 by terry.reedy.

Files
File name Uploaded Description Edit
posix_unsigned_uid-2.patch haypo, 2009-03-25 00:36 review
posix_unsigned_uid-3.patch haypo, 2009-03-25 01:26 review
Messages (13)
msg77301 - (view) Author: Sjoerd Mullender (sjoerd) * (Python committer) Date: 2008-12-08 12:22
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
convert the second and third arguments.  But the valued do not fit in a
32-bit signed integer.  uid_t and gid_t are defined as unsigned
quantities on this system.

The bug does not occur on 32 bit Fedora since there the uid and gid of
nfsnobody are 65534.
msg77306 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-12-08 13:56
This was already corrected by r61540, and will be released with 2.5.3
and 2.6.2.

BUT -- fchown() was not modified. patch is similar to r61540, but I
don't know how to test:

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);
msg77355 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2008-12-08 22:35
2^31-2 doesn't fit in long on 32 bits CPU. On my Linux box, uid_t is an 
unsigned integer (32 bits unsigned integer). Why not using "unsigned 
int"? chown prototype:

   int chown(const char *path, uid_t owner, gid_t group);
msg77393 - (view) Author: Sjoerd Mullender (sjoerd) * (Python committer) Date: 2008-12-09 08:25
I'm sure you meant 2^32-2 ;-).

The "fix" to use long doesn't seem right to me either.  unsigned int is
a better match with uid_t and gid_t.
msg77399 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2008-12-09 10:09
There is not only chown! There are also lchown(), fchown(), stat() and 
lstat().

Attached patch:
 - use "unsigned int" to store Windows st_uid/st_gid in the win32_stat 
structure
 - use PyLong to store an unsigned group in the stat result (because 
2^32-2 doesn't fit in a PyInt)
 - use "unsigned int" (instead of long) for uid/gid in chown, lchown 
and fchown

Note: *chown() accepts -1 for an argument, eg. chown("file", 10, -1) 
only changes the user group.
msg77400 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2008-12-09 10:13
I tested my patch (for Python trunk) on Ubuntu Gutsy with a 32 bits 
CPU: chown(), lchwon(), fchown(), stat() and lstat() works as expected 
(support identifier > 2**31-1).
msg84128 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2009-03-24 23:46
Anyone available for a patch review? Do you need anything else: tests, 
documentation updates, etc.?
msg84130 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-03-25 00:03
The documentation for PyArg_ParseTuple states that the "I" code doesn't
do any overflow checking. Does it mean the input parameters can be
silently truncated?
msg84134 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2009-03-25 00:34
pitrou> The documentation for PyArg_ParseTuple states that the 
pitrou> "I" code doesn't do any overflow checking. Does it mean 
pitrou> the input parameters can be silently truncated?

I'm sorry, but you're right :-) My patch was wrong. New patch 
introduces functions parse_uid() and parse_gid() doing the right thing 
to get an unsigned uid and gid. I reused the parsing code from 
posix_setgroups().
msg84135 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2009-03-25 00:36
Oops, i forgot to ignore space changes in my diff... new patch.
msg84140 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2009-03-25 01:26
New patch using PyNumber_Index()+PyLong_AsLong() for parse_uid() and 
parse_gid() as proposed by antoine (on IRC).

Changes between python trunk and posix_unsigned_uid-3.patch:

- os.chown() and os.fchown() accepts uid and gid > 2^31: identifiers 
can be in [-1; 2^32-1] (-1 means: don't change uid/gid)

- fix os.*stat(): st_uid and st_gid are unsigned long integers (in [0; 
2^32-1]) instead of signed integers

- os.chown(), os.fchown() and os.setgroups() accepts any objects 
implementing __index__() method (instead of just int/long) and display 
the valid range on error ("group id is to big" => "group id is not in 
range [-1; 2^32-1]")
msg84157 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-03-25 10:58
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:
- check that chown(-1, current_gid) succeeds (and leaves the uid intact)
- check that chown(current_uid, -1) succeeds (and leaves the gid intact)
msg85650 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2009-04-06 16:19
See also #3586: "pwd.getpwnam('nobody') produces incorrect results if
sizeof(uid_t) < sizeof(long)" (now closed).
History
Date User Action Args
2011-06-26 19:33:04terry.reedysetversions: - Python 2.6
2011-06-26 19:32:54terry.reedysetversions: + Python 3.2, Python 3.3, - Python 3.0, Python 3.1
2009-04-06 16:19:34hayposetmessages: + msg85650
2009-03-25 10:58:50pitrousetmessages: + msg84157
versions: - Python 2.5
2009-03-25 01:26:59hayposetfiles: + posix_unsigned_uid-3.patch

messages: + msg84140
2009-03-25 00:36:33hayposetfiles: + posix_unsigned_uid-2.patch

messages: + msg84135
2009-03-25 00:36:08hayposetfiles: - posix_unsigned_uid-2.patch
2009-03-25 00:35:37hayposetfiles: - posix_unsigned_uid-2.patch
2009-03-25 00:35:34hayposetfiles: + posix_unsigned_uid-2.patch
2009-03-25 00:34:58hayposetfiles: - posix_unsigned_uid.patch
2009-03-25 00:34:51hayposetfiles: + posix_unsigned_uid-2.patch

messages: + msg84134
2009-03-25 00:03:07pitrousetnosy: + pitrou
messages: + msg84130
2009-03-24 23:46:08hayposetmessages: + msg84128
2008-12-09 10:13:26hayposetmessages: + msg77400
2008-12-09 10:09:31hayposetfiles: + posix_unsigned_uid.patch
title: uid/gid problem in os.chown -> 32-bits unsigned user/group identifier
messages: + msg77399
versions: + Python 2.6, Python 3.0, Python 3.1, Python 2.7
2008-12-09 08:25:11sjoerdsetmessages: + msg77393
2008-12-08 22:35:50hayposetnosy: + haypo
messages: + msg77355
2008-12-08 13:56:35amaury.forgeotdarcsetpriority: normal
keywords: + patch, needs review
messages: + msg77306
nosy: + amaury.forgeotdarc
2008-12-08 12:22:10sjoerdcreate