Обсуждение: "double free" segfault back in pyscopg2 2.5

On Thu, 2013-06-20 at 19:34 +0530, Gangadharan S.A. wrote:
> Hi,
>
>
> It looks like the "double free" segfault from pyscopg2 2.0.8
> ( http://comments.gmane.org/gmane.comp.python.db.psycopg.devel/4964 )
> is back in version 2.5:
[...]
> As before, the problem seems to be that when de-allocing the
> connection, we are calling conn_close before untracking the object.
> conn_close allows other threads to run and call the garbage collector,
> which ends up running dealloc a second time on this object. So we free
> the same memory a second time and hence the double free error.
>
>
> The fix would be to call conn_close after untracking the object in
> connection_type.c:connection_dealloc().

I see: sorry, I've broken it in commit 5aafe38f. Looks like in that
commit I've fixed the cursor and broken the connection.


> The script used to reproduce the issue back in
> http://comments.gmane.org/gmane.comp.python.db.psycopg.devel/4964 will
> not reproduce the issue any more because we don't rollback the
> in-progress transaction during connection close anymore. The only way
> I reliably could reproduce the issue in a test script was to introduce
> a sleep in the C code at connection_int.c:conn_close() after
> Py_BEGIN_ALLOW_THREADS and then run garbage collector from another
> thread during the sleep.
>
>
> Thanks,
> Gangadharan

Thank *you* very much. Open ticket #166. I had in mind to release a new
version in the next days: will include this correction too.


--
Daniele

On Thu, 2013-06-20 at 19:34 +0530, Gangadharan S.A. wrote:

> The fix would be to call conn_close after untracking the object in
> connection_type.c:connection_dealloc().

Pushed correction at
https://github.com/dvarrazzo/psycopg/commit/889b1d826e9cf0ecbc9af938bad72937592af5e4

Is this enough to consider the issue fixed?

I've checked other destructors and we untrack the object before any
other operation in all the objects.

Thanks


--
Daniele

Thanks! Yes, this change should fix the issue - and it did in my testing.

Do we know of a rough esitmate on when this will be released as a version?


On 20-Jun-2013, at 9:12 PM, Daniele Varrazzo <daniele.varrazzo@gmail.com> wrote:

> On Thu, 2013-06-20 at 19:34 +0530, Gangadharan S.A. wrote:
>
>> The fix would be to call conn_close after untracking the object in
>> connection_type.c:connection_dealloc().
>
> Pushed correction at
> https://github.com/dvarrazzo/psycopg/commit/889b1d826e9cf0ecbc9af938bad72937592af5e4
>
> Is this enough to consider the issue fixed?
>
> I've checked other destructors and we untrack the object before any
> other operation in all the objects.
>
> Thanks
>
>
> --
> Daniele
>
>
>
> --
> Sent via psycopg mailing list (psycopg@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/psycopg

On Thu, 2013-06-20 at 21:17 +0530, Gangadharan S.A. wrote:
> Thanks! Yes, this change should fix the issue - and it did in my testing.
>
> Do we know of a rough esitmate on when this will be released as a version?

I'll make a release across the weekend. You should have a 2.5.1 by
Monday.

Thank you again for fixing the same issue twice at 4 years distance. I'm
very curious about your setup: if you could spend two words on it we may
put together some stressing rig.

Cheers,

--
Daniele

On Thu, 2013-06-20 at 22:18 +0530, Gangadharan S.A. wrote:

> 1. We subclass the connection class and pass it in as
> connection_factory argument to psycopg2.connect() . Subclassing is
> necessary to make the garbage collector worry about this object having
> cyclic references and try tp_clear on it. This is how we end up in a
> place where the reference counting based destruction of the object on
> one thread coincides with the garbage collector trying to free the
> same object from another thread. The subclass definition of the
> connection class can simply be a "pass" statement and need not
> implement any actual functionality.

Do you mean a plain Python subclass or a C type subclass? A failed
attempt at creating connection/cursor C subclasses some times ago led me
to review the objects GC interaction (and re-introduce the bug).


> 2. We have 100s of  threads connecting to 100s of databases rationed
> out by a connection pool manager library. Connections are closed and
> opened frequently.

I guessed that was the minimum :)


> Please let me know if further information is needed.

How long have you been running 2.5 in production? Any other issue to
report that may be fixed for 2.5.1?


--
Daniele

> Do you mean a plain Python subclass or a C type subclass?

Plain python subclass. And as I was saying, simply having a subclass
would be enough. It need not have any additional functionailty. It can
just have a "pass" statement in the subclass body. This is
demonstrated in the test script associated with the first segfault
mail thread from 4 years ago.

> How long have you been running 2.5 in production?

Nearly 2 weeks in production. The issue would show up only at
production scale and not in our test environments. Failure would have
happen roughly a couple of times per day per daemon process.

> Any other issue to
> report that may be fixed for 2.5.1?

Nothing else so far.

On 20-Jun-2013, at 11:18 PM, Daniele Varrazzo
<daniele.varrazzo@gmail.com> wrote:

> On Thu, 2013-06-20 at 22:18 +0530, Gangadharan S.A. wrote:
>
>> 1. We subclass the connection class and pass it in as
>> connection_factory argument to psycopg2.connect() . Subclassing is
>> necessary to make the garbage collector worry about this object having
>> cyclic references and try tp_clear on it. This is how we end up in a
>> place where the reference counting based destruction of the object on
>> one thread coincides with the garbage collector trying to free the
>> same object from another thread. The subclass definition of the
>> connection class can simply be a "pass" statement and need not
>> implement any actual functionality.
>
> Do you mean a plain Python subclass or a C type subclass? A failed
> attempt at creating connection/cursor C subclasses some times ago led me
> to review the objects GC interaction (and re-introduce the bug).
>
>
>> 2. We have 100s of  threads connecting to 100s of databases rationed
>> out by a connection pool manager library. Connections are closed and
>> opened frequently.
>
> I guessed that was the minimum :)
>
>
>> Please let me know if further information is needed.
>
>
>
>
> --
> Daniele
>

Thanks! Yes, this change should fix the issue - and it did in my testing.

Do we know of a rough esitmate on when this will be released as a version?


On 20-Jun-2013, at 9:12 PM, Daniele Varrazzo <daniele.varrazzo@gmail.com> wrote:

> On Thu, 2013-06-20 at 19:34 +0530, Gangadharan S.A. wrote:
>
>> The fix would be to call conn_close after untracking the object in
>> connection_type.c:connection_dealloc().
>
> Pushed correction at
> https://github.com/dvarrazzo/psycopg/commit/889b1d826e9cf0ecbc9af938bad72937592af5e4
>
> Is this enough to consider the issue fixed?
>
> I've checked other destructors and we untrack the object before any
> other operation in all the objects.
>
> Thanks
>
>
> --
> Daniele
>
>
>
> --
> Sent via psycopg mailing list (psycopg@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/psycopg

> Do you mean a plain Python subclass or a C type subclass?

Plain python subclass. And as I was saying, simply having a subclass
would be enough. It need not have any additional functionailty. It can
just have a "pass" statement in the subclass body. This is
demonstrated in the test script associated with the first segfault
mail thread from 4 years ago.

> How long have you been running 2.5 in production?

Nearly 2 weeks in production. The issue would show up only at
production scale and not in our test environments. Failure would have
happen roughly a couple of times per day per daemon process.

> Any other issue to
> report that may be fixed for 2.5.1?

Nothing else so far.

On 20-Jun-2013, at 11:18 PM, Daniele Varrazzo
<daniele.varrazzo@gmail.com> wrote:

> On Thu, 2013-06-20 at 22:18 +0530, Gangadharan S.A. wrote:
>
>> 1. We subclass the connection class and pass it in as
>> connection_factory argument to psycopg2.connect() . Subclassing is
>> necessary to make the garbage collector worry about this object having
>> cyclic references and try tp_clear on it. This is how we end up in a
>> place where the reference counting based destruction of the object on
>> one thread coincides with the garbage collector trying to free the
>> same object from another thread. The subclass definition of the
>> connection class can simply be a "pass" statement and need not
>> implement any actual functionality.
>
> Do you mean a plain Python subclass or a C type subclass? A failed
> attempt at creating connection/cursor C subclasses some times ago led me
> to review the objects GC interaction (and re-introduce the bug).
>
>
>> 2. We have 100s of  threads connecting to 100s of databases rationed
>> out by a connection pool manager library. Connections are closed and
>> opened frequently.
>
> I guessed that was the minimum :)
>
>
>> Please let me know if further information is needed.
>
>
>
>
> --
> Daniele
>