classification
Title: chown broken on 64bit
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.0
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: gregory.p.smith, jafo, loewis, nbecker, owsla, pitrou
Priority: normal Keywords: 64bit

Created on 2007-07-04 14:21 by nbecker, last changed 2009-12-23 09:47 by gregory.p.smith. This issue is now closed.

Files
File name Uploaded Description Edit
posixmodule-chown-dynamic.patch jafo, 2008-01-16 08:37 Dynamically figure out the size of uid_t/gid_t.
Messages (13)
msg32445 - (view) Author: Neal D. Becker (nbecker) Date: 2007-07-04 14:21
python-2.5 on fedora fc7 x86_64:

os.stat returns uid > 32bit:
os.stat ('shit')
(33204, 4420723, 64768L, 1, 4294967294, 500, 0, 1183558507, 1183558507, 1183558517)

os.chown doesn't like that:
 os.chown ('shit', 4294967294, 4294967294) 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: signed integer is greater than maximum
msg32446 - (view) Author: Andrew Ferguson (owsla) Date: 2007-07-04 15:04
Indeed, in Modules/posixmodule.c::posix_chown(), the uid and gid variables are defined as type 'int'. They should be of type 'uid_t' and 'gid_t' which are mapped to 'unsigned int' on at least some Unix platforms (I haven't checked extensively.)

The wrinkle here is that chown() needs to be able to handle the case of uid/gid set to -1, which means to leave them as is. Therefore, os.chown(-1, -1) is valid, but so is os.chown(4294967294, 4294967294).
msg32447 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2007-07-08 10:29
Notice that the value really is -2.

It's important to find out how uid_t is defined on the platform; 
it may be that the return value of stat is already incorrect.

Merely changing the variables to uid_t and gid_t is not enough,
since then ParseTuple would stop working correctly.

Is anybody interested in providing a patch?
msg32448 - (view) Author: Andrew Ferguson (owsla) Date: 2007-07-08 13:51
No, the return value of stat is correct. For that it does: PyInt_FromLong((long)st->st_uid) in _pystat_fromstructstat(STRUCT_STAT *st) (same file, posixmodule.c). Fedora has been defining the UID of the nfsnobody user on 32-bit to be 65534 (USHRT_MAX - 1) and on 64-bit to be 4294967294 (UINT_MAX - 1), assuming 32-bit ints. So, yes, this absurdly high UID is real.

So that chown('foo', -1, -1) makes sense, the UID that would be "(uid_t) -1" is reserved. That's why Fedora went for "(uid_t) -2".

I think a patch should look something like:

$ diff -p posixmodule.c.orig posixmodule.c 
*** posixmodule.c.orig  Sun Jul  8 09:43:50 2007
--- posixmodule.c       Sun Jul  8 09:48:27 2007
*************** static PyObject *
*** 1826,1834 ****
  posix_chown(PyObject *self, PyObject *args)
  {
        char *path = NULL;
!       int uid, gid;
        int res;
!       if (!PyArg_ParseTuple(args, "etii:chown",
                              Py_FileSystemDefaultEncoding, &path,
                              &uid, &gid))
                return NULL;
--- 1826,1834 ----
  posix_chown(PyObject *self, PyObject *args)
  {
        char *path = NULL;
!       unsigned int uid, gid;
        int res;
!       if (!PyArg_ParseTuple(args, "etII:chown",
                              Py_FileSystemDefaultEncoding, &path,
                              &uid, &gid))
                return NULL;
msg32449 - (view) Author: Andrew Ferguson (owsla) Date: 2007-07-08 13:58
Reformatted sample patch:

--- posixmodule.c.orig  2007-07-08 09:43:50.000000000 -0400
+++ posixmodule.c       2007-07-08 09:48:27.000000000 -0400
@@ -1826,9 +1826,9 @@
 posix_chown(PyObject *self, PyObject *args)
 {
        char *path = NULL;
-       int uid, gid;
+       unsigned int uid, gid;
        int res;
-       if (!PyArg_ParseTuple(args, "etii:chown",
+       if (!PyArg_ParseTuple(args, "etII:chown",
                              Py_FileSystemDefaultEncoding, &path,
                              &uid, &gid))
                return NULL;
msg59991 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-01-16 07:40
I believe that patch would break on a system where uid_t is a 64-bit
value, yet unsigned int is 32 bits.
msg59992 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2008-01-16 08:37
I've reviewed the chown system call under Linux and I think the included
patch will resolve the problem.  With that patch on a Fedora 8 64-bit
system I'm getting:

>>> os.stat('/tmp/foo')
posix.stat_result(st_mode=33188, st_ino=5111823, st_dev=64769L,
st_nlink=1, st_uid=0, st_gid=0, st_size=0, st_atime=1200469633,
st_mtime=1200469633, st_ctime=1200469657)
>>> os.chown('/tmp/foo', 4294967294, -1)
>>> os.stat('/tmp/foo')
posix.stat_result(st_mode=33188, st_ino=5111823, st_dev=64769L,
st_nlink=1, st_uid=4294967294, st_gid=0, st_size=0, st_atime=1200469633,
st_mtime=1200469633, st_ctime=1200469683)
>>> 

However, it's unclear to me whether there are any platforms that might
define uid_t as signed, and if so whether this code would do the right
thing on those platforms.

I don't know of a way to do it in C off hand, but perhaps we could check
the exact type of the uid_t and gid_t types and see if they're signed or
unsigned and use that combined with sizeof(uid_t) to use exactly the
correct ParseTuple format string.

I've attached a patch that dynamically tries to figure out the size to
use.  Does this seem overly-heavy?  If it seems appropriate, the same
would need to be applied to the other chown functions.
msg59995 - (view) Author: Andrew Ferguson (owsla) Date: 2008-01-16 12:55
The idea of dynamic typing it seems quite heavy to me, but I'm not a
Python hacker, so I don't know what's the norm.

Notice that os.stat() does "PyInt_FromLong((long)st->st_uid)" on the
stat structure's st_uid field. On my platform (OS X 10.4), st_uid is
defined as a uid_t type.

So maybe os.stat() has the answer: ignore the signed vs. unsigned int
problem and just use a long. The actual chown() call in posix_chown()
casts the uid variable to a (uid_t) anyway. GCC doesn't seem to complain
when we cast a long to an unsigned int, even.
msg63770 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2008-03-17 20:58
i'll take a look at this during the sprint.
msg63964 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2008-03-18 19:06
fixed in trunk r61540.

I'm leaving this open until i backport it to release25-maint.
msg63971 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2008-03-18 19:26
backported to 2.5 in r61542 and r61544.

it'll go into py3k via the regular merges from trunk.

The fix just changed the int -> long and 'ii' -> 'll' and added unit
test coverage.

The patch attached to this bug was rejected as too complex:  If
conditional treatment of the types is ever needed it should be done at
build time via autoconf and not at runtime.
msg95916 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-12-02 20:17
Looking at posixmodule.c, perhaps other instances of parsing an uid_t or
a gid_t should have been fixed too (lchown, fchown for example)?
msg96832 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2009-12-23 09:47
indeed, those were missed.  fixed in trunk r77007 and release26-maint 
r77008.
History
Date User Action Args
2009-12-23 09:47:07gregory.p.smithsetmessages: + msg96832
2009-12-02 20:17:27pitrousetnosy: + pitrou
messages: + msg95916
2008-03-18 19:26:32gregory.p.smithsetstatus: open -> closed
keywords: - patch
messages: + msg63971
resolution: remind -> fixed
versions: - Python 2.5
2008-03-18 19:06:45gregory.p.smithsetresolution: remind
messages: + msg63964
versions: - Python 2.6
2008-03-17 20:58:36gregory.p.smithsetversions: + Python 2.6, Python 3.0
2008-03-17 20:58:25gregory.p.smithsetassignee: gregory.p.smith
messages: + msg63770
nosy: + gregory.p.smith
2008-01-16 12:55:27owslasetmessages: + msg59995
2008-01-16 08:37:15jafosetfiles: + posixmodule-chown-dynamic.patch
messages: + msg59992
2008-01-16 07:40:48loewissetmessages: + msg59991
2008-01-16 07:30:32jafosetmessages: - msg59990
2008-01-16 07:10:53jafosetkeywords: + patch, 64bit
nosy: + jafo
type: behavior
messages: + msg59990
2007-08-23 18:49:43georg.brandllinkissue1752703 superseder
2007-07-04 14:21:37nbeckercreate