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

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

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

Possible duplicate of Issue4591?

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

Problem emerged when well-known backup software (rdiff-backup) is 'crashed' (as in no backups). So I think this is not that theoretical.

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?

There is a proposed patch by haypo with Issue4591.

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.

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.

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.

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

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?

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

+1 on using PyNumber_Index.

Thank you, Mark. There is only one question. For what version is it appropriate? Only for 3.4 or for all maintained?

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.

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.

To answer Serhiy's question:  I'd say that this level of cleanup is probably only appropriate for 3.4.  Larry?

See my comment above (dated 2013-04-14 04:30).  I'm passing the buck.

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

Whoops, forgot to write something here.  Updated patch (in previous edit to the issue) incorporating Serhiy's suggestions from Rietveld.

Can I get an LGTM?

New tests needed (with Decimal and Fraction). And what is purpose of changing Py_TYPE(o) to o->ob_type?

Latest patch incorporating Serihy's comments.

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.

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.

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.

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.

Let's push.

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.

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

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.

I think we should leave 2.7 at rest for the moment.

The original issue was fixed in issue4591. Larry's patch only adds support of PyNumber_Index() and refactors already existing uid/gid converters.

Looks like an unnecessary change for the maintenance releases then.

Okay then, closing.

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'

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

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.

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>
> _______________________________________

> Likely they were written before the invention of unittest test
skpping. They could be converted.

See issue18702.