[pywin32-bugs] [ pywin32-Patches-3440738 ] DECREF INCREF Problem (probably severe)

[SourceForge.net <no...@so...>]

Patches item #3440738, was opened at 2011-11-21 05:54
Message generated for change (Comment added) made by mhammond
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=551956&aid=3440738&group_id=78018

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
>Status: Closed
>Resolution: Fixed
Priority: 9
Private: No
Submitted By: kxroberto (kxroberto)
Assigned to: Nobody/Anonymous (nobody)
Summary: DECREF INCREF Problem (probably severe)

Initial Comment:
See also bug #1590399

I still have nasty problems with Python objects magically changing / loosing attributes shortly after creation, while using win32ui and heavy threading. Mainly on machines with many Cores/CPUs. 

Patch below: I found 2 strongly smelling locations in win32ui, which are probably responsible: where objects are DECREF'ed  and possibly INCREF'ed soon after - while "hoping" that they do not disappear. But I guess this is not valid (today). A new Python object somewhere may use this memory meanwhile and then the object may be stolen back and going hybrid ...

(Unfortunately I don't have the MS full version compilers to compile current MFC based stuff.  (Or is this possible somehow with VC Express versions?)  I could make quick tests of win32ui.pyd   (py2.3 and py2.6)


diff -ur --strip _orig/win32assoc.cpp ./win32assoc.cpp
--- _orig/win32assoc.cpp	2009-03-04 11:52:00 +0000
+++ ./win32assoc.cpp	2011-11-21 13:10:42 +0000
@@ -228,11 +228,11 @@
 	// So set the instance to NULL _before_ we decref it!
 	PyObject *old = pAssoc->virtualInst;
 	pAssoc->virtualInst = NULL;
-	XDODECREF(old);
 	if (ob!=Py_None) {
 		pAssoc->virtualInst = ob;
 		DOINCREF(ob);
 	}
+	XDODECREF(old);
 	RETURN_NONE;
 }
 
diff -ur --strip _orig/win32cmd.cpp ./win32cmd.cpp
--- _orig/win32cmd.cpp	2009-01-08 22:33:00 +0000
+++ ./win32cmd.cpp	2011-11-21 13:10:26 +0000
@@ -208,17 +208,20 @@
 		RETURN_ERR("The parameter must be a callable object or None");
 
 	void *oldMethod = NULL;
-	// note I maybe decref, then maybe incref.  I assume object wont be destroyed
-	// (ie, ref go to zero) between the 2 calls!)
+	// we need incref's early in order to avoid a ref erronously going to zero during DEDECREF
+	if (method!=Py_None) {
+		Py_INCREF(method);
+		Py_INCREF(hookedObject);
+	}
 	if (pList->Lookup(message, oldMethod)) {
 		pList->RemoveKey(message);
 		// oldMethod is returned - don't drop its reference.
 		DODECREF(hookedObject);
 	}
 	if (method!=Py_None) {
-		Py_INCREF(method);
+		// already done above: Py_INCREF(method);
 		pList->SetAt(message,method);
-		Py_INCREF(hookedObject);
+		// already done above: Py_INCREF(hookedObject);
 	}
 	if (oldMethod)
 		return (PyObject *)oldMethod;


----------------------------------------------------------------------

>Comment By: Mark Hammond (mhammond)
Date: 2012-01-01 17:44

Message:
Thanks!  I think that first patch could also be done by checking the new
object isn't the same as the old, but I did it your way :)  The second one
I made simpler - just always incref to add a temp ref and always decref at
the end - I've attached that patch below - please let me know if you think
it will not work correctly for some reason.  Checked in as rev
4173:95b4f896b100

diff --git a/Pythonwin/win32cmd.cpp b/Pythonwin/win32cmd.cpp
--- a/Pythonwin/win32cmd.cpp
+++ b/Pythonwin/win32cmd.cpp
@@ -208,8 +208,10 @@
                RETURN_ERR("The parameter must be a callable object or
None");

        void *oldMethod = NULL;
-       // note I maybe decref, then maybe incref.  I assume object wont be
destroyed
-       // (ie, ref go to zero) between the 2 calls!)
+       // note I maybe decref, then maybe incref.  To ensure the object
will
+       // not be destroyed (ie, ref go to zero) between the 2 calls), I
+       // add a temporary reference first.
+       DOINCREF(hookedObject);
        if (pList->Lookup(message, oldMethod)) {
                pList->RemoveKey(message);
                // oldMethod is returned - don't drop its reference.
@@ -220,6 +222,7 @@
                pList->SetAt(message,method);
                Py_INCREF(hookedObject);
        }
+       DODECREF(hookedObject); // remove temp reference added above.
        if (oldMethod)
                return (PyObject *)oldMethod;
        else


----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=551956&aid=3440738&group_id=78018