[issue3100] weakref subclass segfault

[Roundup Robot]

Roundup Robot added the comment:

New changeset 3d8109fe6d82 by Gregory P. Smith in branch 'default':
Correct a comment in the test referencing the wrong issue number (issue3100
https://hg.python.org/cpython/rev/3d8109fe6d82

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Amaury Forgeot d'Arc]

Amaury Forgeot d'Arc [EMAIL PROTECTED] added the comment:

This was fixed 4 months ago with r64309 (trunk, 2.6) and r64310 (2.5).

--
resolution:  - fixed
status: open - closed

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Amaury Forgeot d'Arc]

Amaury Forgeot d'Arc [EMAIL PROTECTED] added the comment:

Committed r64309, with the suggested whitespace change.
Will backport.

Concerning the INCREF patch: would a simple assert(refcnt0) be enough?

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Adam Olsen]

Adam Olsen [EMAIL PROTECTED] added the comment:

Unfortunately, Py_INCREF is sometimes used in an expression (followed by
a comma).  I wouldn't expect an assert to be valid there (and I'd want
to check ISO C to make sure it's portable, not just accepted by GCC).

I'd like if Py_INCREF and friends were made into static inline
functions, which *should* have identical performance (at least on GCC),
but that's a more significant change.

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Jesse Noller]

Changes by Jesse Noller [EMAIL PROTECTED]:


--
nosy: +roudkerk

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Adam Olsen]

Adam Olsen [EMAIL PROTECTED] added the comment:

Patch to add extra sanity checks to Py_INCREF (only if Py_DEBUG is set).
 If the refcount is 0 or negative if calls Py_FatalError.  This should
catch revival bugs such as this one a little more clearly.

The patch also adds a little more checking to _Py_ForgetReference, so
it's more likely to print an error rather than segfaulting.

--
keywords: +patch
Added file: http://bugs.python.org/file10614/python-incref-from-zero.diff

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Amaury Forgeot d'Arc]

Amaury Forgeot d'Arc [EMAIL PROTECTED] added the comment:

All the versions I tried (2.4, 2.5, 2.6, 3.0) crash with the given script

--
components: +Interpreter Core -Extension Modules
nosy: +amaury.forgeotdarc
versions: +Python 2.5

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Amaury Forgeot d'Arc]

Amaury Forgeot d'Arc [EMAIL PROTECTED] added the comment:

It seems enough to simply skip deleted weakrefs in PyObject_ClearWeakRefs.
Here is a tentative patch.

Added file: http://bugs.python.org/file10615/weakref_cycle.patch

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Amaury Forgeot d'Arc]

Changes by Amaury Forgeot d'Arc [EMAIL PROTECTED]:


Removed file: http://bugs.python.org/file10615/weakref_cycle.patch

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Amaury Forgeot d'Arc]

Amaury Forgeot d'Arc [EMAIL PROTECTED] added the comment:

A new version of the patch, which tests the case of multiple weakrefs on
the same object, that get deleted together:
tp_dealloc of one weakref calls tp_dealloc of the second weakref, which
calls tp_dealloc of the referenced object. Since the weakrefs are being
deleted, no callback should fire.

Added file: http://bugs.python.org/file10616/weakref_cycle.patch

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Adam Olsen]

Changes by Adam Olsen [EMAIL PROTECTED]:


--
nosy: +jnoller

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Adam Olsen]

Adam Olsen [EMAIL PROTECTED] added the comment:

Well, my attempt at a patch didn't work, and yours does, so I guess I
have to support yours. ;)

Can you review my python-incref-from-zero patch?  It verifies the
invariant that you need, that once an object hits a refcount of 0 it
won't get raised again.  (The possibility of __del__ makes me worry, but
it *looks* okay.)

gcmodule.c has an inline copy of handle_callbacks.  Is it possible a
collection could have the same problem we're fixing here?

Minor nit: you're asserting cbcalled, but you're not using the generic
callback, so it's meaningless.

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Adam Olsen]

Adam Olsen [EMAIL PROTECTED] added the comment:

Ahh, it seems gcmodule already considers the weakref to be reachable
when it calls the callbacks, so it shouldn't be a problem.

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Amaury Forgeot d'Arc]

Changes by Amaury Forgeot d'Arc [EMAIL PROTECTED]:


Removed file: http://bugs.python.org/file10616/weakref_cycle.patch

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Amaury Forgeot d'Arc]

Amaury Forgeot d'Arc [EMAIL PROTECTED] added the comment:

 you're asserting cbcalled, but you're not using the generic callback, 
 so it's meaningless.
The new patch corrects this

Added file: http://bugs.python.org/file10625/weakref_cycle.patch

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Adam Olsen]

Adam Olsen [EMAIL PROTECTED] added the comment:

Another minor nit: if(current-ob_refcnt  0) should have a space
after the if.  Otherwise it's looking good.

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Benjamin Peterson]

Changes by Benjamin Peterson [EMAIL PROTECTED]:


___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Adam Olsen]

Adam Olsen [EMAIL PROTECTED] added the comment:

Specific enough yet?  Seems the WeakValueDictionary and the module
clearing aren't necessary.

A subclass of weakref is created.  The target of this weakref is added
as an attribute of the weakref.  So long as a callback is present there
will be a crash on shutdown.  However, if the callback prints the
attribute, you get a crash right then.  The weakref claims to be dead,
which shouldn't be possible when the weakref's attributes have a strong
reference to the target.

--
title: segfault with WeakValueDictionary and module clearing - weakref 
subclass segfault
Added file: http://bugs.python.org/file10613/weakref_subclass_cycle.py

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Adam Olsen]

Changes by Adam Olsen [EMAIL PROTECTED]:


Removed file: http://bugs.python.org/file10612/inner.py

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Adam Olsen]

Changes by Adam Olsen [EMAIL PROTECTED]:


Removed file: http://bugs.python.org/file10611/outer.py

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Adam Olsen]

Adam Olsen [EMAIL PROTECTED] added the comment:

1. MyRef is released from the module as part of shutdown
2. MyRef's subtype_dealloc DECREFs its dictptr (not clearing it, as
MyRef is dead and should be unreachable)
3. the dict DECREFs the Dummy (MyRef's target)
4. Dummy's subtype_dealloc calls PyObject_ClearWeakRefs to notify of its
demise
5. the callback is called, with the dead MyRef as an argument
6. If MyRef's dict is accessed a segfault occurs.  Presumably just
calling the callback does enough damage to explain the segfault without
accessing MyRef's dict.

As I understand, a deleted weakref doesn't call its callback.  However,
subtype_dealloc doesn't call the base type's tp_dealloc until *after* it
does everything else.  Does it need a special case for weakrefs, or
maybe a tp_predealloc slot?

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue3100] weakref subclass segfault

[Adam Olsen]

Adam Olsen [EMAIL PROTECTED] added the comment:

Ahh, I missed a detail: when the callback is called the weakref has a
refcount of 0, which is ICNREFed to 1 when getting put in the args, then
drops down to 0 again when the args are DECREFed (causing it to get
_Py_ForgetReference to be called a second time, which segfaults.)

___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3100
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com