[jira] [Comment Edited] (PROTON-687) Memory corruption in proton-test

["Andrew Stitcher (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/PROTON-687?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14144356#comment-14144356 ] 

Andrew Stitcher edited comment on PROTON-687 at 9/23/14 5:22 AM:
-----------------------------------------------------------------

I think the problem here is that the python Delivery holds a pointer to the C pn_delivery. In that sort of case the Python object is supposed to own the C object, and there is not supposed to be any way for the object to be deleted before the Python object deletes it.

However here the delivery is also on the settled/unsettled list and this will delete the underlying delivery when it is no longer referred to.

So a fix seems possible on the python side:
{noformat}
diff --git a/proton-c/bindings/python/proton.py b/proton-c/bindings/python/proto
index ee3701c..f9d1d0f 100644
--- a/proton-c/bindings/python/proton.py
+++ b/proton-c/bindings/python/proton.py
@@ -2890,6 +2890,7 @@ class Delivery(object):
     return wrapper
 
   def __init__(self, dlv):
+    pn_object_incref(dlv)
     self._dlv = dlv
     pn_delivery_set_context(self._dlv, pn_py2void(self))
     self.local = Disposition(pn_delivery_local(self._dlv), True)
@@ -2898,6 +2899,7 @@ class Delivery(object):
 
   def __del__(self):
     pn_delivery_set_context(self._dlv, pn_py2void(None))
+    pn_object_decref(self._dlv)
 
   def _release(self):
     """Release the underlying C Engine resource."""
{noformat}

This fix also makes the same delivery event test fail as Ken's suggested fix above, which makes me think the test might be too dependent on the order of things being deleted.

This fix also adds leaks (although there do seem to be quite a few without).
These appear to be because the C pn_delivery context is the ref counted Python Delivery which means there is a potential circular reference cycle here which the ref counters can't resolve.


was (Author: astitcher):
I think the problem here is that the python Delivery holds a pointer to the C pn_delivery. In that sort of case the Python object is supposed to own the C object, and there is not supposed to be any way for the object to be deleted before the Python object deletes it.

However here the delivery is also on the settled/unsettled list and this will delete the underlying delivery when it is no longer referred to.

So a fix seems possible on the python side:
diff --git a/proton-c/bindings/python/proton.py b/proton-c/bindings/python/proto
index ee3701c..f9d1d0f 100644
--- a/proton-c/bindings/python/proton.py
+++ b/proton-c/bindings/python/proton.py
@@ -2890,6 +2890,7 @@ class Delivery(object):
     return wrapper
 
   def __init__(self, dlv):
+    pn_object_incref(dlv)
     self._dlv = dlv
     pn_delivery_set_context(self._dlv, pn_py2void(self))
     self.local = Disposition(pn_delivery_local(self._dlv), True)
@@ -2898,6 +2899,7 @@ class Delivery(object):
 
   def __del__(self):
     pn_delivery_set_context(self._dlv, pn_py2void(None))
+    pn_object_decref(self._dlv)
 
   def _release(self):
     """Release the underlying C Engine resource."""

This fix also makes the same delivery event test fail as Ken's suggested fix above, h=which makes me think the test might be too dependent on the order of things being deleted.

I don't think this fix is adding any leaks - there appear to be leaks present already in the test run.


> Memory corruption in proton-test
> --------------------------------
>
>                 Key: PROTON-687
>                 URL: https://issues.apache.org/jira/browse/PROTON-687
>             Project: Qpid Proton
>          Issue Type: Bug
>          Components: proton-c, python-binding
>    Affects Versions: 0.8
>            Reporter: Cliff Jansen
>            Assignee: Rafael H. Schloming
>            Priority: Blocker
>         Attachments: segv.patch
>
>
> proton-test will fail with memory violations.  Deallocated memory is accessed, usually with benign results but less often on Windows.
> The attached crude patch allows the bug to be "discovered" on Linux as well.
> It is not clear to me if the problem is with the swig wrapper, or whether this just exposes a reference counting bug in proton-c.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)