The expression  '%o' % x,  where x is a user-defined instance, usually ignores a user-defined __oct__() method.  I suppose that's fine; assuming this is the expected behavior, then the present issue is about the "usually" in my previous sentence.

If 'x' is an instance of a subclass of 'long', then the __oct__() is called.  It must be specifically a 'long' -- not, say, 'int' or 'str' or 'object'.  Moreover, there is a test in test_format.py (class Foobar) that checks that if this __oct__() returns a non-string, then we get a nice TypeError.  That's already strange -- why is __oct__() called in the first place?  But trying out more I get (CPython 2.7.1):

>>> class X(long):
...   def __oct__(self):
...     return 'abc'
... 
>>> '%o' % X()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
SystemError: ../trunk/Objects/stringobject.c:4035: bad argument to internal function

And this code will crash a debug build:

>>> class X(long):
...     def __oct__(self):
...         return 'foo'.upper()
...
>>> '%o' % X()
Assertion failed: buf[sign] == '0', file ..\..\Objects\stringobject.c, line 4059

The doc for %o only says it returns the “signed octal value”; __oct__ is not a magic method (i.e. it’s not a name with special meaning), converting with oct() actually uses __index__.

Eric: that's wrong, it is a magic method.  See for example "__oct__" in Objects/typeobject.c.  I'm not sure I understand why you would point this out, though.  A "SystemError: bad argument to internal function" or an "Assertion failed" are both bugs anyway.

It's a magic method in 2.7 but not 3.x.

Just updating the type to 'behavior'.
 
I can still reproduce this issue:

$ python2.7
Python 2.7.8 (default, Sep  9 2014, 22:08:43) 
[GCC 4.9.1] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> class Y(long):
...   def __oct__(self):
...     return 'abc'
... 
>>> '%o' % Y()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
SystemError: ../Objects/stringobject.c:4045: bad argument to internal function
>>>

Here is a patch which fixes this issue. Now ValueError with relevant message is raised instead of SystemError or crash.

Do have I overseen the patch? or may be doing something wrong?
or isn't anything uploaded?

Oh, sorry. I again forgot to upload a patch.

Could anyone please make a review of the patch?

It's seriously obscure to call a user-defined __oct__ method and then mangle the resulting string in ways that only make sense if the __oct__ method returned something reasonable.

The patch is probably a little more complicated than it could be.  For example, I don't understand the special cases for `llen <= 1`: I don't think `PyString_FromStringAndSize(NULL, 1)` can return an existing object.  And if you cut off the final 'L' you don't replace it with '\0' any longer, which could in theory break some callers expecting to see a null-terminated string here.  PyErr_Format() should use `%.200s` for `tp_name`.

I'm not sure if it's relevant but in the patch you changed
previous 'assert(check)' with 'if (not check) goto error'.
But the new patch code adds 'assert(len == 0 || Py_REFCNT(r1) == 1);'

Just curious, is there a reason why couldn't be in the same way?

Thank you for your review Arigo.

Here is updated patch.

Note that the patch also fixes a reference leak if llen > INT_MAX.

> Just curious, is there a reason why couldn't be in the same way?

Old asserts depend on user code, new asserts check internal consistency.

+        if (Py_REFCNT(result) == 1)
+            buf[len] = '\0';

...and if the refcount is not equal to 1, then too bad, we won't null-terminate the string and hope that nobody crashes because of this.??

If the refcount is not equal to 1, we will copy the content to new null-
terminated string.

Ah, sorry.  Ok.  Now a different issue: the user-defined function can return an interned string.  If it has a refcount of 1, _PyString_FormatLong() will mutate it.  Then when we DECREF it, string_dealloc() will not find it any more in the interned dict and crash with a fatal error.

Note that I'm mostly having fun finding holes in delicate logic, like mutating strings in-place.  It would be much more simple to either (1) stop calling the user-defined functions and behave similarly to most other built-in types; or (2) stop trying to mutate that poor string in-place and always just create a new one. :-)

Good catch! Here is a patch which fixes this issue too.

> (1) stop calling the user-defined functions and behave similarly to most other built-in types;

This is done in 3.x.

> (2) stop trying to mutate that poor string in-place and always just create a new one.

The patch can be simplified by removing 6 lines. But this will slow down integer formatting by few (about 2-7) percents in worst case. Not a big deal.

If buf contains "-00" and the type is 'o', then it will be modified in-place even if the string is shared.

Heh, it's getting really funny.

Here is new patch. It first split string on areas: numnondigits (sign+"0x" if F_ALT is not set), skipped ("0x" if F_ALT is set), numdigits and optional "L" suffix, and then construct new string either in-place (if the string is not shared and result fits in original string) or in new string. It uses not more allocations than current code and should not add overhead for common cases.

Hey, Armin. Do you want to continue bug hunting?

Sorry, your patch still contains similar issues.  I postponed continuing to bounce it back and forth, but it seems that someone else needs to take my place now.

Sorry, I didn't find any issues with the last patch. Could you please point on them?

I there are no objection's, I'll commit the last patch.

Armin indicated in his last comment that the patch still has multiple issues.

Are there tests to catch the issues he previously found?  That seems the best method to verify that the current (and future) patches don't break 2.7.

It is easy to find a bug than reproduce it.

I read the code multiple times but still don't see any issues with the last path. If anybody know issues with it, please point on them. Otherwise I'll commit the patch.

I reviewed your patch again.  It does look good after all: I find only one issue---it seems I implied there were several ones but I can't find more.  The issue is that PyString_AsString(result) will succeed if 'result' is a unicode.  Then you call PyString_GET_SIZE(result), which gives nonsense for unicode objects.

Good catch Armin! Thank you for your review.

Updated patch uses PyString_AsStringAndSize() and adds a check that result is exact str before changing it in-place.

Looks ok now!

New changeset adb296e4bcaa by Serhiy Storchaka in branch '2.7':
Issue #11145: Fixed miscellaneous issues with C-style formatting of types
https://hg.python.org/cpython/rev/adb296e4bcaa