This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Unchecked PyErr_WarnPy3k return value in Objects/typeobject.c
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 2.7
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: mark.dickinson, ncoghlan, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2010-05-05 14:30 by mark.dickinson, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
issue8627.patch serhiy.storchaka, 2014-10-02 17:34 review
Messages (20)
msg105041 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-05 14:30
Lines 3884 and 3890 of Objects/typeobject.c (trunk, r80782), which check for a subclass overriding __eq__ (or __cmp__) but not __hash__, call PyErr_WarnPy3k but don't check the return value.  [This was reported when compiling Python with clang.] This can lead to an "XXX undetected error", if the warning actually raised an exception:

newton:trunk dickinsm$ ./python.exe -3
Python 2.7b1+ (trunk:80783, May  5 2010, 15:25:42) 
[GCC 4.2.1 (Apple Inc. build 5659)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import warnings
[35022 refs]
>>> warnings.filterwarnings("error")
[35046 refs]
>>> class A(object):
...     def __eq__(self, other):
...         return False
... 
XXX undetected error
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x
[35139 refs]

Nick, I think this came from a checkin of yours: r65642, related to issue 2235.  Any suggestions about how to fix it?  It's a bit tricky, since the function the check occurs in (inherit_slots) has return type 'void'.  Is it okay if I change inherit_slots to return an int here?  If so, I'll produce a patch.

Also, can we remove the check related to __hash__ and __cmp__?  With __cmp__ gone in 3.1, and 3.0 considered defunct, I don't think it's relevant any more.
msg105043 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-05 14:49
Hmm.  Fixing this isn't easy:  a simple 'if (inherit_slots(...) < 0) goto error;' produces a huge reference leak in PyType_Ready.
msg105071 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2010-05-05 18:52
I just came across the warning myself (after ignoring all the PyType_INIT() warnings; what to do about those?) and came to the same conclusion: it's a pain to fix.

One option is to do a PyErr_Occurred() check at inherit_slots() call sites. Would that mitigate the leak? If I remember correctly inherit_slots() is not called constantly (only when a type is being created?) so having the expensive PyErr_Occurred() call should not hurt performance.
msg105092 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-05-05 22:07
I haven't looked at this in a while, but I do remember it was hard to get working at all without a ridiculous number of false alarms - type initialisation isn't the most straightforward thing in the world.

Agreed the warning for __cmp__ should just go away - I think we nuked the actual check in 3.1 when __cmp__ was removed, but must have missed this at the time.

For the refleak, I'm wondering if you may be seeing a problem with statically allocated type objects. type_new does a DECREF on the allocated object if PyType_Ready fails, so everything should be getting cleaned up at that point, but that won't happen for a statically allocated type.

Looking at type_clear, I suggest sticking a "Py_CLEAR(type->tp_mro);" in before the goto error line and see if that eliminates the leak. If that actually works, then I'll have my doubts about the correctness of the other "goto error" lines in PyType_Ready that happen after the call to mro_internal().
msg105093 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-05-05 22:09
(Not sure how relevant my second last paragraph is - I meant to take that out after noticing the MRO details).
msg105316 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-05-08 16:50
I also have my doubts about the other 'goto error' lines in PyType_Ready, but I haven't figured out how to trigger those gotos in normal code.

Trying to figure out how to clean up properly on error in PyType_Ready is making my head hurt.

As a quick fix, we could simply do a PyErr_Clear() if the PyErr_WarnPy3k returns -1;  this clearly isn't right, but at least it gets rid of the "XXX undetected error".
msg107133 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-06-05 12:39
Removed the __cmp__ warning in r81736, and added a PyErr_Clear() for the __eq__ warning in r81740.  I'll leave this open in case anyone wants to figure out how to propagate the warning exception properly.
msg107134 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2010-06-05 12:40
Not sure why I added 3.1 and 3.2 to the versions.
msg175775 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-11-17 17:01
A quick search through typeobject.c in 2.7 didn't show any PyErr_WarnPy3k calls that didn't have their return value checked or propagated.
msg175787 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-11-17 17:33
There's still a case where the return value isn't properly propagated:  it's the one marked by an 'XXX' in the source.
msg175789 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-11-17 17:37
I can't find it, Mark, after searching through every XXX marker in a 2.7 checkout of Objects/typeobject.c .
msg175790 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-11-17 17:39
It's line 3912 in the current source.  The warning is always cleared, which is the wrong thing to do if warnings should be turned into exceptions.
msg175794 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-11-17 18:07
To clarify, here's the bug:  the following code should raise an exception, but doesn't:

iwasawa:cpython mdickinson$ ./python.exe -3
Python 2.7.3+ (2.7:333fe4c4897a, Nov 17 2012, 18:01:00) 
[GCC 4.2.1 (Apple Inc. build 5664)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import warnings; warnings.filterwarnings("error")
>>> class A(object):
...     def __eq__(self, other):
...         return False
... 


Without "warnings.filterwarnings("error")", the warning gets issued as expected:

iwasawa:cpython mdickinson$ ./python.exe -3
Python 2.7.3+ (2.7:333fe4c4897a, Nov 17 2012, 18:01:00) 
[GCC 4.2.1 (Apple Inc. build 5664)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class A(object):
...     def __eq__(self, object):
...         return False
... 
__main__:1: DeprecationWarning: Overriding __eq__ blocks inheritance of __hash__ in 3.x

Brett, is it okay to re-open this?  Perhaps a change of title would help?  Or I can open a new issue for the remaining problem, if you think that's better.
msg175815 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2012-11-17 22:29
Go ahead and reopen. I just couldn't find the lines anymore when I closed
it.
On Nov 17, 2012 3:07 PM, "Mark Dickinson" <report@bugs.python.org> wrote:

>
> Mark Dickinson added the comment:
>
> To clarify, here's the bug:  the following code should raise an exception,
> but doesn't:
>
> iwasawa:cpython mdickinson$ ./python.exe -3
> Python 2.7.3+ (2.7:333fe4c4897a, Nov 17 2012, 18:01:00)
> [GCC 4.2.1 (Apple Inc. build 5664)] on darwin
> Type "help", "copyright", "credits" or "license" for more information.
> >>> import warnings; warnings.filterwarnings("error")
> >>> class A(object):
> ...     def __eq__(self, other):
> ...         return False
> ...
>
>
> Without "warnings.filterwarnings("error")", the warning gets issued as
> expected:
>
> iwasawa:cpython mdickinson$ ./python.exe -3
> Python 2.7.3+ (2.7:333fe4c4897a, Nov 17 2012, 18:01:00)
> [GCC 4.2.1 (Apple Inc. build 5664)] on darwin
> Type "help", "copyright", "credits" or "license" for more information.
> >>> class A(object):
> ...     def __eq__(self, object):
> ...         return False
> ...
> __main__:1: DeprecationWarning: Overriding __eq__ blocks inheritance of
> __hash__ in 3.x
>
> Brett, is it okay to re-open this?  Perhaps a change of title would help?
>  Or I can open a new issue for the remaining problem, if you think that's
> better.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue8627>
> _______________________________________
>
msg228244 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-10-02 17:34
May be this patch will fix the issue.
msg228327 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-10-03 13:12
Is there any cleanup to worry about as the comment suggests? If not then the patch LGTM if it does lead to any memory leak.
msg228355 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-10-03 17:47
How did you detect a leak Mark?
msg228358 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2014-10-03 17:54
> How did you detect a leak Mark?

Um.  It was four years ago.  I have no idea. :-)

I suspect I was just looking at the C code and being my usual nasty suspicious self.
msg228359 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2014-10-03 17:55
Whoops; make that two years ago, not four.  The rest stands, though.
msg361328 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2020-02-03 21:28
Let's close. This was only ever an issue in Python 2.
History
Date User Action Args
2022-04-11 14:57:00adminsetgithub: 52873
2020-02-03 21:28:50mark.dickinsonsetstatus: open -> closed
resolution: out of date
messages: + msg361328

stage: patch review -> resolved
2020-01-24 23:27:41brett.cannonsetnosy: - brett.cannon
2017-01-03 06:33:50serhiy.storchakalinkissue29138 superseder
2014-10-03 17:55:41mark.dickinsonsetmessages: + msg228359
2014-10-03 17:54:57mark.dickinsonsetmessages: + msg228358
2014-10-03 17:47:05serhiy.storchakasetmessages: + msg228355
2014-10-03 13:12:49brett.cannonsetmessages: + msg228327
2014-10-02 17:34:57serhiy.storchakasetfiles: + issue8627.patch

type: behavior
components: + Interpreter Core
versions: - Python 2.6
keywords: + patch
nosy: + serhiy.storchaka

messages: + msg228244
stage: patch review
2012-11-18 10:34:18mark.dickinsonsetstatus: closed -> open
resolution: out of date -> (no value)
2012-11-17 22:29:48brett.cannonsetmessages: + msg175815
2012-11-17 18:07:01mark.dickinsonsetmessages: + msg175794
2012-11-17 17:39:49mark.dickinsonsetmessages: + msg175790
2012-11-17 17:37:20brett.cannonsetmessages: + msg175789
2012-11-17 17:33:20mark.dickinsonsetmessages: + msg175787
2012-11-17 17:01:46brett.cannonsetstatus: open -> closed
resolution: out of date
messages: + msg175775
2010-06-05 12:40:55mark.dickinsonsetmessages: + msg107134
versions: - Python 3.1, Python 3.2
2010-06-05 12:39:54mark.dickinsonsetmessages: + msg107133
2010-05-08 16:50:09mark.dickinsonsetmessages: + msg105316
2010-05-05 22:09:51ncoghlansetmessages: + msg105093
2010-05-05 22:07:38ncoghlansetmessages: + msg105092
2010-05-05 18:52:53brett.cannonsetnosy: + brett.cannon
messages: + msg105071
2010-05-05 14:49:01mark.dickinsonsetmessages: + msg105043
2010-05-05 14:30:55mark.dickinsoncreate