classification
Title: os.chown: OverflowError: Python int too large to convert to C long
Type: behavior Stage: committed/rejected
Components: None Versions: Python 3.4, Python 3.3, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: larry Nosy List: Arfrever, Mark.Shannon, Scott.Leerssen, benjamin.peterson, do1, georg.brandl, haypo, larry, loewis, mark.dickinson, ned.deily, pitrou, python-dev, serhiy.storchaka, skrah, terry.reedy
Priority: normal Keywords: patch

Created on 2012-07-09 02:15 by do1, last changed 2013-08-11 06:18 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
larry.chown.unsigned.uid.gid.1.diff larry, 2012-07-15 01:14 review
posix_uid_gid_conv_tests.patch serhiy.storchaka, 2013-02-14 11:24 Extended tests for *chown() review
posix_uid_gid_conv_index.patch serhiy.storchaka, 2013-02-14 11:33 Use __index__ for uid and gid conversion review
larry.chown.unsigned.uid.gid.2.diff larry, 2013-04-15 10:01 Second cut at a patch, building on Serhiy's second patch. review
larry.chown.unsigned.uid.gid.3.diff larry, 2013-04-16 12:35 Updated patch with Serhiy's changes from Rietveld. review
larry.chown.unsigned.uid.gid.4.diff larry, 2013-04-22 19:07 Latest patch incorporating Serihy's comments. review
larry.chown.unsigned.uid.gid.5.diff larry, 2013-04-22 19:10 New patch which might even be good enough for trunk. review
larry.chown.unsigned.uid.gid.6.diff larry, 2013-04-23 23:32 Another day, another patch for uid_converter. review
Messages (42)
msg165053 - (view) Author: (do1) Date: 2012-07-09 02:15
os.chown() can not change uid/gid to valid but high number.

# chown 4294967294.4294967294 a
# ls -l a
-rw-r--r-- 1 4294967294 4294967294 0 Jul  9 05:22 a

# python
Python 2.7.3 (default, Jun 24 2012, 06:19:44)
[GCC 4.1.2 20080704 (Red Hat 4.1.2-52)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.chown("a", 4294967294, 4294967294)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: Python int too large to convert to C long
msg165054 - (view) Author: (do1) Date: 2012-07-09 02:26
I can add that system is x86_32. Same code in x86_64 result in no error even on older Python version 2.6.6
msg165425 - (view) Author: Mark Shannon (Mark.Shannon) * Date: 2012-07-13 23:22
Larry, you're the expert on this.

Looks to me like the format character in PyArg_ParseTuple in posix_fchown and kin should be 'k' (unsigned long) rather than 'l' (signed long).
msg165427 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2012-07-13 23:34
Possible duplicate of Issue4591?
msg165428 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-07-14 00:15
I don't think "k" is the right answer.

POSIX defines the user and group parameters as uid_t and gid_t respectively; in turn, uid_t and gid_t are defined as "integers", pointedly omitting the either of the words "signed" or "unsigned".  So POSIX makes no guarantee that uid_t and gid_t are unsigned.

On the other hand: chown takes special values defined as "(uid_t)-1" and "(gid_t)-1" to indicate "don't change this value".  If we change chown to use "k" for uid and gid, then in order to use this value you'd have to pass in the equivalent unsigned value.  And that'd require you to know the size of an int on the local platform.  Yuck!

If this is a genuine problem, then I think we have to take "O" (or "O!") and convert the long by hand.  If it's -1, use "(uid_t)-1" (etc), otherwise convert as unsigned.  On the other hand, if this is only a theoretical problem then I'd prefer to wontfix and keep the code simple.

p.s. find uid_t / gid_t here: http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/types.h.html
chown here:
http://pubs.opengroup.org/onlinepubs/009695399/functions/chown.html
msg165429 - (view) Author: (do1) Date: 2012-07-14 00:22
Problem emerged when well-known backup software (rdiff-backup) is 'crashed' (as in no backups). So I think this is not that theoretical.
msg165430 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-07-14 00:37
Thinking about it some more, maybe we need to be smart about whether uid_t/gid_t are signed or unsigned.

I'm no good with configure hackery--could anyone here enhance the configure script so it could tell us whether or not uid_t and gid_t were signed or unsigned?
msg165431 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2012-07-14 00:40
There is a proposed patch by haypo with Issue4591.
msg165433 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-07-14 06:17
That patch from Victor looks pretty good.  I'll try to get it in for beta 2, however I'm on the road right now.  I'm back Sunday night and will try to do it then.

If someone else wants to jump in that's fine with me.
msg165477 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-07-15 00:48
It looks like #1747858 is another duplicate.  Though arguably all these "dups" aren't really dups because I rewrote chown's argument parsing for 3.3.
msg165481 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2012-07-15 01:14
Here's a first cut at a patch.  It's really just an update of Victor's patch to #4591 (done with his blessing).  I tweaked it slightly; the parsing functions are now O& converter functions.

I also added a reasonable unit test.  Note however that the unit test is only run if you run the test as root.

I don't think this needs any documentation, just a Misc/NEWS entry.

If someone can give me quick turnaround on a review (or two) maybe we can get this in before beta 2 tomorrow.

I'm adding Antoine and MvL as they were in on the earliest known bug of this class.
msg182088 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-14 11:24
Similar test was committed for issue4591 and it fixes this issue. There are only two differences with Larry's patch: tests and handling of non-integers.

Here is a patch based on Larry's patch which extends tests for *chown().
msg182089 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-14 11:33
And here is a patch which uses __index__ for uid and gid conversion. decimal.Decimal should be added to the list of wrong types in the test.

I'm not sure about the last patch, whether to consider it as a bugfix or as a new feature?
msg182533 - (view) Author: Roundup Robot (python-dev) Date: 2013-02-20 17:53
New changeset 9b37e53838eb by Serhiy Storchaka in branch '2.7':
Issue #15301: Enhance os.*chown() testing.  Based on patch by Larry Hastings.
http://hg.python.org/cpython/rev/9b37e53838eb

New changeset a0baf5347cd1 by Serhiy Storchaka in branch '3.2':
Issue #15301: Enhance os.*chown() testing.  Based on patch by Larry Hastings.
http://hg.python.org/cpython/rev/a0baf5347cd1

New changeset e97b6394848b by Serhiy Storchaka in branch '3.3':
Issue #15301: Enhance os.*chown() testing.  Based on patch by Larry Hastings.
http://hg.python.org/cpython/rev/e97b6394848b

New changeset d4bf997a34e9 by Serhiy Storchaka in branch 'default':
Issue #15301: Enhance os.*chown() testing.  Based on patch by Larry Hastings.
http://hg.python.org/cpython/rev/d4bf997a34e9
msg186862 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2013-04-13 21:58
+1 on using PyNumber_Index.
msg186909 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-14 10:54
Thank you, Mark. There is only one question. For what version is it appropriate? Only for 3.4 or for all maintained?
msg186913 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-04-14 11:30
Oh...!  Serhiy, I thought you already checked in the AsIndex stuff.  Guess I was asleep at the switch.

Certainly the patch should go in for trunk.  I'd be comfortable with it going in for 3.3 as a bugfix but that's ultimately Georg's call.

I'll give you a comment or two on the diff.
msg186970 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-04-15 10:01
Okay, I got inspired and (in the words of Barry Warsaw) JFDI.  Attached is my revised patch.  I took Serhiy's patch and reworked it quite a bit:

* I think it's now easier to follow.  In particular:
* The most common case (no overflow) is now first.  In Serhiy's patch
  the most common case is buried in the middle of the second "if".
* I removed some extraneous tests.
* I changed the error messages to call the values "uid" and "gid", to match the names of the parameters.
* I noticed that _fd_converter had the same bad-idea PyFloat_Check, so I changed it to use PyNumber_Index instead.

In the process I also noticed that Serhiy's approach had a resource leak: it never decref'd the result of PyNumber_Index() when successful.

To make sure I duplicated Serhiy's semantics, I had a test harness that ran both his and mine and ensured they returned the same thing (or both threw an error).  Obviously I removed all that before cutting the patch.
msg186971 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2013-04-15 10:53
To answer Serhiy's question:  I'd say that this level of cleanup is probably only appropriate for 3.4.  Larry?
msg186974 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-04-15 11:17
See my comment above (dated 2013-04-14 04:30).  I'm passing the buck.
msg187019 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-15 19:26
>* The most common case (no overflow) is now first.  In Serhiy's patch
>  the most common case is buried in the middle of the second "if".

It's because my implementation is a simplified version of more complicated patch for issue2005, which supports signed uid_t and sizeof(uid_t) > sizeof(long). And this support can be added if necessary.
msg187075 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-04-16 12:36
Whoops, forgot to write something here.  Updated patch (in previous edit to the issue) incorporating Serhiy's suggestions from Rietveld.
msg187576 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-04-22 18:05
Can I get an LGTM?
msg187579 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-22 18:33
New tests needed (with Decimal and Fraction). And what is purpose of changing Py_TYPE(o) to o->ob_type?
msg187580 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-04-22 19:07
Latest patch incorporating Serihy's comments.
msg187581 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-04-22 19:10
Whoops, meant to throw in a Fraction too.  Added that.  Also hoisted the imports out of the function call, just to be a good guy.
msg187677 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-04-23 23:32
Serhiy: Oh, I get it, your critique was about the "Note:" part at the bottom.  It would have been helpful if you had inserted your feedback there, instead of at the top.  Yeah, you're right, the note was incorrect.

I considered adding a note saying that the code was wrong if sizeof(uid_t) > sizeof(long) but thought better of it ;-)

New patch attached.
msg187689 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-24 07:03
Yes, it is what I had in mean. Sorry for indistinctness.

As far as you change _fd_converter() it will be good to add also a test for float/decimal/fractional fd. Otherwise the patch LGTM.
msg192193 - (view) Author: Scott Leerssen (Scott.Leerssen) Date: 2013-07-02 12:50
Regarding whether or not to include the fix in 2.7, I'd like to suggest that it be included there as well.  It will be quite some time before the project on which I work moves to Python 3, and I just hit the same issue.
msg194534 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-06 11:56
Let's push.
msg194645 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-08-08 06:44
Benjamin, do you want my elaborate fix in for 2.7?  It means adding two new converters, one for pid and one for gid, and switching everything that takes pid/gid arguments to use those.
msg194646 - (view) Author: Roundup Robot (python-dev) Date: 2013-08-08 07:23
New changeset f871f8662509 by Larry Hastings in branch 'default':
Issue #15301: Parsing fd, uid, and gid parameters for builtins
http://hg.python.org/cpython/rev/f871f8662509
msg194647 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-08-08 07:23
Now fixed in trunk.  I am waiting to hear from Georg and the only-recently-pinged Benjamin to see if they want these fixes in 3.3 or 2.7.
msg194649 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-08-08 07:32
I think we should leave 2.7 at rest for the moment.
msg194650 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-08 07:35
The original issue was fixed in issue4591. Larry's patch only adds support of PyNumber_Index() and refactors already existing uid/gid converters.
msg194654 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2013-08-08 08:39
Looks like an unnecessary change for the maintenance releases then.
msg194655 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-08-08 09:34
Okay then, closing.
msg194763 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-08-09 20:15
The Windows buildbots are broken:


======================================================================
ERROR: test_chown_uid_gid_arguments_must_be_index (test.test_os.MakedirTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "E:\Data\buildslave\cpython\3.x.snakebite-win2k3r2sp2-x86\build\lib\test\test_os.py", line 875, in test_chown_uid_gid_arguments_must_be_index
    self.assertRaises(TypeError, os.chown, support.TESTFN, value, gid)
AttributeError: 'module' object has no attribute 'chown'
msg194857 - (view) Author: Roundup Robot (python-dev) Date: 2013-08-11 00:59
New changeset 9b0e9e9812f8 by Terry Jan Reedy in branch 'default':
Issue #15301: skip new test method so Windows builtbots stop failing.
http://hg.python.org/cpython/rev/9b0e9e9812f8
msg194858 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-08-11 01:03
This was the only failure on 2 of 4 buildbots when I last looked.

test_os has several methods like this:

    def test_dup2(self):
        if hasattr(os, "dup2"):
            self.check(os.dup2, 20)

Should the hasattr be moved to skipUnless for all of these?
They now 'pass' without the needed os method.
msg194860 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-08-11 01:55
Likely they were written before the invention of unittest test
skpping. They could be converted.

2013/8/10 Terry J. Reedy <report@bugs.python.org>:
>
> Terry J. Reedy added the comment:
>
> This was the only failure on 2 of 4 buildbots when I last looked.
>
> test_os has several methods like this:
>
>     def test_dup2(self):
>         if hasattr(os, "dup2"):
>             self.check(os.dup2, 20)
>
> Should the hasattr be moved to skipUnless for all of these?
> They now 'pass' without the needed os method.
>
> ----------
> nosy: +terry.reedy
> status: open -> closed
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue15301>
> _______________________________________
msg194865 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-11 06:18
> Likely they were written before the invention of unittest test
skpping. They could be converted.

See issue18702.
History
Date User Action Args
2013-08-11 06:18:31serhiy.storchakasetmessages: + msg194865
2013-08-11 01:55:05benjamin.petersonsetmessages: + msg194860
2013-08-11 01:03:15terry.reedysetstatus: open -> closed
nosy: + terry.reedy
messages: + msg194858

2013-08-11 00:59:19python-devsetmessages: + msg194857
2013-08-09 20:15:18pitrousetstatus: closed -> open

messages: + msg194763
2013-08-08 09:34:00larrysetstatus: open -> closed
resolution: fixed
messages: + msg194655

stage: patch review -> committed/rejected
2013-08-08 08:39:59georg.brandlsetmessages: + msg194654
2013-08-08 07:35:03serhiy.storchakasetmessages: + msg194650
2013-08-08 07:32:29benjamin.petersonsetmessages: + msg194649
2013-08-08 07:23:59larrysetmessages: + msg194647
2013-08-08 07:23:02python-devsetmessages: + msg194646
2013-08-08 06:44:55larrysetnosy: + benjamin.peterson
messages: + msg194645
2013-08-06 11:56:51serhiy.storchakasetmessages: + msg194534
2013-07-02 12:50:34Scott.Leerssensetnosy: + Scott.Leerssen
messages: + msg192193
2013-04-24 07:03:22serhiy.storchakasetmessages: + msg187689
2013-04-23 23:32:46larrysetfiles: + larry.chown.unsigned.uid.gid.6.diff

messages: + msg187677
2013-04-22 19:10:17larrysetfiles: + larry.chown.unsigned.uid.gid.5.diff

messages: + msg187581
2013-04-22 19:07:19larrysetfiles: + larry.chown.unsigned.uid.gid.4.diff

messages: + msg187580
2013-04-22 18:33:40serhiy.storchakasetmessages: + msg187579
2013-04-22 18:05:48larrysetmessages: + msg187576
2013-04-16 12:36:13larrysetmessages: + msg187075
2013-04-16 12:35:32larrysetfiles: + larry.chown.unsigned.uid.gid.3.diff
2013-04-15 19:26:24serhiy.storchakasetmessages: + msg187019
2013-04-15 11:17:46larrysetmessages: + msg186974
2013-04-15 10:53:50mark.dickinsonsetmessages: + msg186971
2013-04-15 10:01:47larrysetfiles: + larry.chown.unsigned.uid.gid.2.diff

messages: + msg186970
2013-04-14 11:30:26larrysetnosy: + georg.brandl
messages: + msg186913
2013-04-14 10:54:38serhiy.storchakasetmessages: + msg186909
2013-04-13 21:58:30mark.dickinsonsetnosy: + mark.dickinson
messages: + msg186862
2013-02-20 17:53:44python-devsetnosy: + python-dev
messages: + msg182533
2013-02-18 21:57:07serhiy.storchakasetstage: patch review
2013-02-14 11:33:08serhiy.storchakasetfiles: + posix_uid_gid_conv_index.patch
versions: + Python 3.2, Python 3.3, Python 3.4
nosy: + skrah

messages: + msg182089
2013-02-14 11:24:36serhiy.storchakasetfiles: + posix_uid_gid_conv_tests.patch
nosy: + serhiy.storchaka
messages: + msg182088

2012-07-15 01:14:41larrysetfiles: + larry.chown.unsigned.uid.gid.1.diff

nosy: + loewis, pitrou
messages: + msg165481

keywords: + patch
2012-07-15 00:48:08larrysetmessages: + msg165477
2012-07-14 06:17:23larrysetassignee: larry
messages: + msg165433
2012-07-14 00:40:00ned.deilysetmessages: + msg165431
2012-07-14 00:37:53larrysetmessages: + msg165430
2012-07-14 00:29:46Arfreversetnosy: + Arfrever
2012-07-14 00:22:00do1setmessages: + msg165429
2012-07-14 00:15:51larrysetmessages: + msg165428
2012-07-13 23:34:22ned.deilysetnosy: + haypo, ned.deily
messages: + msg165427
2012-07-13 23:22:08Mark.Shannonsetnosy: + Mark.Shannon, larry
type: behavior
messages: + msg165425
2012-07-09 02:26:35do1setmessages: + msg165054
2012-07-09 02:15:39do1create