chown broken on 64bit

[nbecker]

BPO	1747858
Nosy	@loewis, @gpshead, @pitrou
Files	posixmodule-chown-dynamic.patch: Dynamically figure out the size of uid_t/gid_t.

^{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:

assignee = 'https://github.com/gpshead'
closed_at = <Date 2008-03-18.19:26:32.509>
created_at = <Date 2007-07-04.14:21:37.000>
labels = ['type-bug', 'library']
title = 'chown broken on 64bit'
updated_at = <Date 2009-12-23.09:47:07.826>
user = 'https://bugs.python.org/nbecker'

bugs.python.org fields:

activity = <Date 2009-12-23.09:47:07.826>
actor = 'gregory.p.smith'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2008-03-18.19:26:32.509>
closer = 'gregory.p.smith'
components = ['Library (Lib)']
creation = <Date 2007-07-04.14:21:37.000>
creator = 'nbecker'
dependencies = []
files = ['9182']
hgrepos = []
issue_num = 1747858
keywords = ['64bit']
message_count = 13.0
messages = ['32445', '32446', '32447', '32448', '32449', '59991', '59992', '59995', '63770', '63964', '63971', '95916', '96832']
nosy_count = 6.0
nosy_names = ['loewis', 'gregory.p.smith', 'jafo', 'nbecker', 'pitrou', 'owsla']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue1747858'
versions = ['Python 3.0']

[nbecker]

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

[owsla]

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).

[loewis]

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?

[owsla]

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;

[owsla]

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;

[loewis]

I believe that patch would break on a system where uid_t is a 64-bit
value, yet unsigned int is 32 bits.

[jafo]

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.

[owsla]

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.

[gpshead]

i'll take a look at this during the sprint.

[gpshead]

fixed in trunk r61540.

I'm leaving this open until i backport it to release25-maint.

[gpshead]

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.

[pitrou]

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)?

[gpshead]

indeed, those were missed. fixed in trunk r77007 and release26-maint
r77008.