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
chown broken on 64bit #45149
Comments
python-2.5 on fedora fc7 x86_64: os.stat returns uid > 32bit: 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 |
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). |
Notice that the value really is -2. It's important to find out how uid_t is defined on the platform; Merely changing the variables to uid_t and gid_t is not enough, Is anybody interested in providing a patch? |
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; |
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; |
I believe that patch would break on a system where uid_t is a 64-bit |
I've reviewed the chown system call under Linux and I think the included >>> 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 I don't know of a way to do it in C off hand, but perhaps we could check I've attached a patch that dynamically tries to figure out the size to |
The idea of dynamic typing it seems quite heavy to me, but I'm not a Notice that os.stat() does "PyInt_FromLong((long)st->st_uid)" on the So maybe os.stat() has the answer: ignore the signed vs. unsigned int |
i'll take a look at this during the sprint. |
fixed in trunk r61540. I'm leaving this open until i backport it to release25-maint. |
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 The patch attached to this bug was rejected as too complex: If |
Looking at posixmodule.c, perhaps other instances of parsing an uid_t or |
indeed, those were missed. fixed in trunk r77007 and release26-maint |
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: