Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#115 DECREF INCREF Problem (probably severe)

closed-fixed
nobody
None
9
2012-01-02
2011-11-21
kxroberto
No

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;

Discussion

  • kxroberto
    kxroberto
    2011-11-21

    • priority: 5 --> 9
     
  • Mark Hammond
    Mark Hammond
    2012-01-02

    • status: open --> closed-fixed
     
  • Mark Hammond
    Mark Hammond
    2012-01-02

    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