classification
Title: 32-bits unsigned user/group identifier
Type: behavior Stage: resolved
Components: Extension Modules, Interpreter Core Versions: Python 3.4, Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Arfrever, amaury.forgeotdarc, eric.smith, haypo, mark.dickinson, pitrou, python-dev, serhiy.storchaka, sjoerd, skrah
Priority: normal Keywords: needs review, patch

Created on 2008-12-08 12:22 by sjoerd, last changed 2014-08-17 01:28 by Arfrever. This issue is now closed.

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
posix_uid_gid_conv_3.patch serhiy.storchaka, 2012-11-24 18:29 review
posix_uid_gid_conv_4.patch serhiy.storchaka, 2012-11-25 10:22 review
Messages (43)
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).
msg176303 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-24 18:29
Here is a simplified version of patch from issue2005. 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.
msg176309 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-11-24 19:41
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.)
msg176313 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-11-24 20:11
> 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?
msg176315 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-24 20:33
> Hmm.  I take that back.  I don't think any of this really belongs in Objects/longobject.c.

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?

> Is it possible to just use PyLong_FromLong / PyLong_FromLongLong etc. depending  on the precision of uid_t / gid_t?

No. uid_t/gid_t can be large than maximal signed long, and long long can be not defined. See issue2005 where more complex patch proposed.
msg176316 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-24 20:40
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).
msg176317 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-11-24 20:41
Do we know of any systems where none of int, long, long long (if defined) or their unsigned variants match gid_t / uid_t?
msg176318 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-11-24 20:42
A new header file sounds reasonable.  We could then move the PID stuff out of longobject.h
msg176320 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-24 20:47
> Do we know of any systems where none of int, long, long long (if defined) or their unsigned variants match gid_t / uid_t?

No, I don't.
msg176325 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-11-24 23:53
The value -1 is used is some functions like chown() (to only change the
user, only the group or none, to check if you are allowed or not). How do
you handle this value?
Le 24 nov. 2012 19:29, "Serhiy Storchaka" <report@bugs.python.org> a écrit :

>
> Serhiy Storchaka added the comment:
>
> Here is a simplified version of patch from issue2005. 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.
>
> ----------
> components: +Extension Modules, Interpreter Core -Library (Lib)
> nosy: +serhiy.storchaka
> stage:  -> patch review
> versions: +Python 3.4
> Added file: http://bugs.python.org/file28096/posix_uid_gid_conv_3.patch
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue4591>
> _______________________________________
>
msg176334 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-25 10:22
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.
msg176336 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-25 10:43
> The value -1 is used is some functions like chown() (to only change the
user, only the group or none, to check if you are allowed or not). How do
you handle this value?

Yes, there is a special case for -1. Look at the code.
msg181036 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-01-31 16:58
Are there any objections, nitpicks or suggestions?
msg181826 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2013-02-10 18:14
No objections here.
msg181847 - (view) Author: Roundup Robot (python-dev) Date: 2013-02-10 20:04
New changeset b322655a4a88 by Serhiy Storchaka in branch '3.3':
Issue #4591: Uid and gid values larger than 2**31 are supported now.
http://hg.python.org/cpython/rev/b322655a4a88

New changeset 94256de0aff0 by Serhiy Storchaka in branch 'default':
Issue #4591: Uid and gid values larger than 2**31 are supported now.
http://hg.python.org/cpython/rev/94256de0aff0
msg181852 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-02-10 20:34
Looks like this broke some buildbots:


======================================================================
ERROR: test_chown (test.test_shutil.TestShutil)
----------------------------------------------------------------------
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'
msg181856 - (view) Author: Roundup Robot (python-dev) Date: 2013-02-10 21:29
New changeset 4ef048f4834e by Serhiy Storchaka in branch '3.3':
Reject float as uid or gid.
http://hg.python.org/cpython/rev/4ef048f4834e

New changeset 3fdcffdfd3e6 by Serhiy Storchaka in branch 'default':
Reject float as uid or gid.
http://hg.python.org/cpython/rev/3fdcffdfd3e6
msg181860 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-02-10 22:38
Not technically the topic of this issue, but should we just lock
down _Py_Uid_Converter() to ints?

This is still accepted:

os.setuid(Decimal("1000.2"))
msg181861 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-02-10 22:46
> Not technically the topic of this issue, but should we just lock
> down _Py_Uid_Converter() to ints?

The right way to do it should be to use PyNumber_Index().

(perhaps we need as PyNumber_AsLongIndex() to do the direct conversion
to a C long, it would make things easier in many cases)
msg181877 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2013-02-11 07:29
> (perhaps we need as PyNumber_AsLongIndex() to do the direct conversion
> to a C long, it would make things easier in many cases)

+1, but drop the 'Index' part of the name, to match the existing PyNumber_AsSsize_t.
msg181878 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-02-11 07:33
> > (perhaps we need as PyNumber_AsLongIndex() to do the direct conversion
> > to a C long, it would make things easier in many cases)
> 
> +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
possible confusion.
Note there is also PyIndex_Check(), so we could call it
PyIndex_AsLong().
msg181883 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2013-02-11 08:21
Hmm;  good point.  Well, +1 to the functionality, anyway;  I'll leave the discussion about the name.
msg181886 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-11 09:21
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?
msg181899 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2013-02-11 15:21
+1 for PyIndex_AsLong()
msg181907 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-11 16:24
> (perhaps we need as PyNumber_AsLongIndex() to do the direct conversion
to a C long, it would make things easier in many cases)

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.
msg181908 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-11 16:33
> Not technically the topic of this issue, but should we just lock
> down _Py_Uid_Converter() to ints?

I just copied this code from PyArg_ParseTuple*() for 'l' format.

> This is still accepted:
>
> os.setuid(Decimal("1000.2"))

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.
msg181910 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-02-11 16:58
Serhiy Storchaka <report@bugs.python.org> wrote:
> > Not technically the topic of this issue, but should we just lock
> > down _Py_Uid_Converter() to ints?
> 
> I just copied this code from PyArg_ParseTuple*() for 'l' format.

> > os.setuid(Decimal("1000.2"))

I know that this behavior wasn't introduced by you. It's perfectly fine
to use PyArg_ParseTuple() for guidance or to try to preserve the existing
behavior.

> >>> chr(Decimal('65.2'))
> 'A'

I just happen to think that PyArg_ParseTuple() is wrong here and we shouldn't
enable this "feature" in new code. Fixing these issues one by one is probably
the best way forward.
msg181920 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-11 18:37
> There is a problem with test_pwd on 64-bit platform.

Fixed in changesets a0983e46feb1 and 1e9fa629756c: Raise KeyError instead of OverflowError when getpwuid's argument is out of uid_t range.
msg181922 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-02-11 19:11
> 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 don't think it will be exponential :-)
msg181947 - (view) Author: Roundup Robot (python-dev) Date: 2013-02-12 07:36
New changeset 3893ab574c55 by Serhiy Storchaka in branch '3.2':
Issue #4591: Uid and gid values larger than 2**31 are supported now.
http://hg.python.org/cpython/rev/3893ab574c55

New changeset 035cbc654889 by Serhiy Storchaka in branch '2.7':
Issue #4591: Uid and gid values larger than 2**31 are supported now.
http://hg.python.org/cpython/rev/035cbc654889
msg225421 - (view) Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) * Date: 2014-08-17 01:28
Revision 035cbc654889 added useless '#ifndef Py_LIMITED_API' check in 2.7 branch.
Support for Py_LIMITED_API was introduced in Python 3.2.
History
Date User Action Args
2014-08-17 01:28:34Arfreversetnosy: + Arfrever
messages: + msg225421
2013-02-14 10:51:02serhiy.storchakalinkissue7365 superseder
2013-02-14 09:19:17serhiy.storchakalinkissue2005 dependencies
2013-02-14 09:15:27serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2013-02-12 07:36:40python-devsetmessages: + msg181947
2013-02-11 19:11:39pitrousetmessages: + msg181922
2013-02-11 18:37:40serhiy.storchakasetmessages: + msg181920
2013-02-11 16:58:12skrahsetmessages: + msg181910
2013-02-11 16:33:39serhiy.storchakasetmessages: + msg181908
2013-02-11 16:24:47serhiy.storchakasetmessages: + msg181907
2013-02-11 15:21:25eric.smithsetnosy: + eric.smith
messages: + msg181899
2013-02-11 09:21:10serhiy.storchakasetmessages: + msg181886
2013-02-11 08:21:36mark.dickinsonsetmessages: + msg181883
2013-02-11 07:33:44pitrousetmessages: + msg181878
2013-02-11 07:29:58mark.dickinsonsetmessages: + msg181877
2013-02-10 22:46:55pitrousetmessages: + msg181861
2013-02-10 22:38:47skrahsetnosy: + skrah
messages: + msg181860
2013-02-10 21:29:28python-devsetmessages: + msg181856
2013-02-10 20:34:23pitrousetmessages: + msg181852
2013-02-10 20:04:07python-devsetnosy: + python-dev
messages: + msg181847
2013-02-10 18:14:29mark.dickinsonsetmessages: + msg181826
2013-01-31 16:58:16serhiy.storchakasetassignee: serhiy.storchaka
messages: + msg181036
2012-11-25 10:43:34serhiy.storchakasetmessages: + msg176336
2012-11-25 10:22:04serhiy.storchakasetfiles: + posix_uid_gid_conv_4.patch

messages: + msg176334
2012-11-24 23:53:23hayposetmessages: + msg176325
2012-11-24 20:47:46serhiy.storchakasetmessages: + msg176320
2012-11-24 20:42:21mark.dickinsonsetmessages: + msg176318
2012-11-24 20:41:14mark.dickinsonsetmessages: + msg176317
2012-11-24 20:40:04serhiy.storchakasetmessages: + msg176316
2012-11-24 20:33:38serhiy.storchakasetmessages: + msg176315
2012-11-24 20:11:20mark.dickinsonsetmessages: + msg176313
2012-11-24 19:41:40mark.dickinsonsetmessages: + msg176309
2012-11-24 19:01:49pitrousetnosy: + mark.dickinson
2012-11-24 18:29:41serhiy.storchakasetfiles: + posix_uid_gid_conv_3.patch

components: + Extension Modules, Interpreter Core, - Library (Lib)
versions: + Python 3.4
nosy: + serhiy.storchaka

messages: + msg176303
stage: patch review
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