[issue11145] '%o' % user-defined instance

Changes by Serhiy Storchaka : -- resolution: -> fixed stage: patch review -> resolved status: open -> closed ___ Python tracker

[issue11145] '%o' % user-defined instance

Roundup Robot added the comment: 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 -- nosy: +python-dev ___ Python

[issue11145] '%o' % user-defined instance

Armin Rigo added the comment: Looks ok now! -- ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue11145] '%o' % user-defined instance

Serhiy Storchaka added the comment: 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. -- Added file: http://bugs.python.org/file45701/issue11145_5.patch

[issue11145] '%o' % user-defined instance

Armin Rigo added the comment: 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

[issue11145] '%o' % user-defined instance

Serhiy Storchaka added the comment: 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. -- assignee: -> serhiy.storchaka ___

[issue11145] '%o' % user-defined instance

Changes by Ethan Furman et...@stoneleaf.us: -- nosy: -ethan.furman ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145 ___ ___ Python-bugs-list

[issue11145] '%o' % user-defined instance

Ethan Furman added the comment: 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. --

[issue11145] '%o' % user-defined instance

Serhiy Storchaka added the comment: It is easy to find a bug than reproduce it. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145 ___ ___

[issue11145] '%o' % user-defined instance

Serhiy Storchaka added the comment: I there are no objection's, I'll commit the last patch. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145 ___

[issue11145] '%o' % user-defined instance

Serhiy Storchaka added the comment: Sorry, I didn't find any issues with the last patch. Could you please point on them? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145 ___

[issue11145] '%o' % user-defined instance

Changes by Ethan Furman et...@stoneleaf.us: -- nosy: +ethan.furman ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145 ___ ___ Python-bugs-list

[issue11145] '%o' % user-defined instance

Serhiy Storchaka added the comment: Hey, Armin. Do you want to continue bug hunting? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145 ___

[issue11145] '%o' % user-defined instance

Armin Rigo added the comment: 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. -- ___ Python tracker rep...@bugs.python.org

[issue11145] '%o' % user-defined instance

Serhiy Storchaka added the comment: 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

[issue11145] '%o' % user-defined instance

Armin Rigo added the comment: If buf contains -00 and the type is 'o', then it will be modified in-place even if the string is shared. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145

[issue11145] '%o' % user-defined instance

Serhiy Storchaka added the comment: Heh, it's getting really funny. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145 ___ ___

[issue11145] '%o' % user-defined instance

Serhiy Storchaka added the comment: Could anyone please make a review of the patch? -- keywords: +needs review ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145 ___

[issue11145] '%o' % user-defined instance

Armin Rigo added the comment: 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

[issue11145] '%o' % user-defined instance

Francis MB added the comment: 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? --

[issue11145] '%o' % user-defined instance

Serhiy Storchaka added the comment: Thank you for your review Arigo. Here is updated patch. Note that the patch also fixes a reference leak if llen INT_MAX. -- Added file: http://bugs.python.org/file37213/issue11145_2.patch ___ Python tracker

[issue11145] '%o' % user-defined instance

Serhiy Storchaka added the comment: 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. -- ___ Python tracker rep...@bugs.python.org

[issue11145] '%o' % user-defined instance

Armin Rigo added the comment: +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.?? -- ___

[issue11145] '%o' % user-defined instance

Serhiy Storchaka added the comment: If the refcount is not equal to 1, we will copy the content to new null- terminated string. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145 ___

[issue11145] '%o' % user-defined instance

Armin Rigo added the comment: 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

[issue11145] '%o' % user-defined instance

Serhiy Storchaka added the comment: 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

[issue11145] '%o' % user-defined instance

Changes by Serhiy Storchaka storch...@gmail.com: Added file: http://bugs.python.org/file37216/issue11145_3_simpler.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145 ___

[issue11145] '%o' % user-defined instance

Francis MB added the comment: 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): ...

[issue11145] '%o' % user-defined instance

Serhiy Storchaka added the comment: Here is a patch which fixes this issue. Now ValueError with relevant message is raised instead of SystemError or crash. -- nosy: +serhiy.storchaka stage: - patch review type: behavior - crash ___ Python tracker

[issue11145] '%o' % user-defined instance

Francis MB added the comment: Do have I overseen the patch? or may be doing something wrong? or isn't anything uploaded? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145 ___

[issue11145] '%o' % user-defined instance

Serhiy Storchaka added the comment: Oh, sorry. I again forgot to upload a patch. -- keywords: +patch Added file: http://bugs.python.org/file36800/issue11145.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145

[issue11145] '%o' % user-defined instance

Armin Rigo ar...@users.sourceforge.net added the comment: 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

[issue11145] '%o' % user-defined instance

Changes by Antoine Pitrou pit...@free.fr: -- nosy: +benjamin.peterson ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145 ___ ___ Python-bugs-list

[issue11145] '%o' % user-defined instance

Benjamin Peterson benja...@python.org added the comment: It's a magic method in 2.7 but not 3.x. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145 ___

[issue11145] '%o' % user-defined instance

Éric Araujo mer...@netwok.org added the comment: 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__. -- nosy: +eric.araujo

[issue11145] '%o' % user-defined instance

New submission from Armin Rigo ar...@users.sourceforge.net: 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

[issue11145] '%o' % user-defined instance

Amaury Forgeot d'Arc amaur...@gmail.com added the comment: 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 -- nosy:

[issue11145] '%o' % user-defined instance

Changes by Eric Smith e...@trueblade.com: -- nosy: +eric.smith ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue11145 ___ ___ Python-bugs-list