[lucy-dev] Hosts, refcounts and destructors

[Marvin Humphrey <ma...@rectangular.com>]

Lucifers,

In the Python bindings I've been working on, Clownfish objects *are*
Python objects.

    struct cfish_Obj {
        PyObject_HEAD
        cfish_Class *klass;
    };

This presents some challenges with how we manage object allocations at
present.

First, we require that each host binding implement object allocation via
Class_Make_Obj_IMP, but the Clownfish core provides an implementation of
Obj_Destroy_IMP which always calls `free`.  This causes problems because
under Python the objects will be allocated using Python's custom
allocator and it won't be valid to `free` them.

As a remedy, I propose that each host binding be required to provide
Obj_Destroy_IMP.

Second, under Python, Clownfish objects use the Python refcount -- which
Python of course accesses directly, rather than going through the
Clownfish Inc_RefCount, Dec_RefCount and Get_RefCount wrappers.  This
presents problems for immortal classes like Class, Method, and BoolNum
which override those refcount manipulation methods to do nothing.  A
incref from Clownfish-space followed by a decref from Python-space will
cause the refcount to decrease, eventually triggering an immortal
object's destructor while valid references still exist.

To address this issue, I think what we ought to do is take refcount
manipulation outside of method calls, instead manipulating refcounts
directly using macros which wrap either concrete or `static inline`
functions.  Immortal types can have their refcount set absurdly high on
object creation, and can have their destructors reset the refcount
rather than invoke deallocation.

Under that system, refcounts will be exceedingly unlikely to fall to 0
under normal circumstances.  However, we still have to take care not to
introduce a security vulnerability which happens when a destructor fires
on an immortal object in case someone figures out how to make that
happen artificially -- so I'd like to know if anybody sees problems now,
and I'd like to request that any commits I eventually supply receive
above-average scrutiny.

Marvin Humphrey

Re: [lucy-dev] Hosts, refcounts and destructors

[Nick Wellnhofer <we...@aevum.de>]

On 04/02/2015 21:31, Marvin Humphrey wrote:
> On Wed, Feb 4, 2015 at 3:14 AM, Nick Wellnhofer <we...@aevum.de> wrote:
>
>> So a similar scheme could also work for the Python bindings: Call Py_INCREF
>> instead of Obj_Inc_RefCount when passing a Clownfish object to Python space.
>> This makes sure that the refcount is increased even for "immortal" objects.
>
> That's an interesting idea, though I think it would get ugly in the details.
> Say that a method returns an "incremented" object: we'd have to Py_INCREF
> then CFISH_DECREF the return value before passing it to Python.

Just like we do in the Perl bindings :)

> I was really
> hoping that having Clownfish objects which actually *are* Python objects would
> allow us to avoid such gymnastics.

I was just thinking that our Perl approach might work for Python as well.

> In any case, I don't think we should be offering method-based overrideable
> refcount manipulation as a user-level Clownfish feature.  This stuff is really
> hard to get right for the core alone across multiple hosts -- we are nowhere
> near the point where we can design and document for extensibility.

+1

>> But to rely on the GIL is a bad proposal anyway. We want Clownfish projects
>> to be able to release the GIL and then the same thread-safety guarantees
>> should hold for all host languages.
>
> I don't think it's feasible for Clownfish code to release the GIL except for
> contained operations where we can guarantee no python code will run (e.g. some
> specific I/O operation).  Because there are many opportunities to override
> Clownfish code with host code, in general the only safe assumption is that for
> any block of arbitrary Clownfish, some Python code may run.  And that means
> that any such arbitrary block needs to have the GIL.
>
> Put another way, a symbiotic object system is not the kind of extension that
> can release the GIL, because its operation is closely interleaved with the
> host.  The kind of extension that can release the GIL is one which is cleanly
> separated from the host.

I didn't mean to make Clownfish release the GIL automatically but to allow 
authors to release it manually. (OTOH, Clownfish could simply reacquire the 
GIL when calling back into host space. But it seems that releasing the GIL 
only makes sense for longer running operations, so it should be controlled by 
the extension writer.)

> (Side note: setting the cached host object under the Perl bindings should use
> `Atomic_cas_ptr` for immortal object types.)

Good catch.

Nick

Re: [lucy-dev] Hosts, refcounts and destructors

[Marvin Humphrey <ma...@rectangular.com>]

On Wed, Feb 4, 2015 at 3:14 AM, Nick Wellnhofer <we...@aevum.de> wrote:

> So a similar scheme could also work for the Python bindings: Call Py_INCREF
> instead of Obj_Inc_RefCount when passing a Clownfish object to Python space.
> This makes sure that the refcount is increased even for "immortal" objects.

That's an interesting idea, though I think it would get ugly in the details.
Say that a method returns an "incremented" object: we'd have to Py_INCREF
then CFISH_DECREF the return value before passing it to Python.  I was really
hoping that having Clownfish objects which actually *are* Python objects would
allow us to avoid such gymnastics.

In any case, I don't think we should be offering method-based overrideable
refcount manipulation as a user-level Clownfish feature.  This stuff is really
hard to get right for the core alone across multiple hosts -- we are nowhere
near the point where we can design and document for extensibility.

> But to rely on the GIL is a bad proposal anyway. We want Clownfish projects
> to be able to release the GIL and then the same thread-safety guarantees
> should hold for all host languages.

I don't think it's feasible for Clownfish code to release the GIL except for
contained operations where we can guarantee no python code will run (e.g. some
specific I/O operation).  Because there are many opportunities to override
Clownfish code with host code, in general the only safe assumption is that for
any block of arbitrary Clownfish, some Python code may run.  And that means
that any such arbitrary block needs to have the GIL.

Put another way, a symbiotic object system is not the kind of extension that
can release the GIL, because its operation is closely interleaved with the
host.  The kind of extension that can release the GIL is one which is cleanly
separated from the host.

(Side note: setting the cached host object under the Perl bindings should use
`Atomic_cas_ptr` for immortal object types.)

Marvin Humphrey

Re: [lucy-dev] Hosts, refcounts and destructors

[Nick Wellnhofer <we...@aevum.de>]

On 04/02/2015 04:14, Marvin Humphrey wrote:
> However, what I *am* proposing is to install a custom `tp_dealloc`
> for the "immortal" classes which resets the refcount and stops there.
>
> If there's a reason that won't work, I haven't discovered it yet.

Sounds good.

>> (Is our Perl implementation even safe?
>> Isn't it possible that Perl reuses the cached "inner object" SV if the Perl
>> refcount drops to zero?)
>
> We should be safe because I don't think that's possible.  Perl never gets at
> the inner object directly: we give Perl a new RV every time a Clownfish object
> needs to be accessed from Perl-space.  Creating a new RV causes the inner
> object's refcount to rise by 1; when the RV ultimately goes away, it causes
> the inner object's refcount to fall by 1, resulting in no net change.  And all
> these refcount manipulations are thread-safe because the we `SvSHARE` the
> inner object for "immortal" Clownfish types.
>
> In the current implementation, intercepting DESTROY should only come into play
> during Perl's global destruction.

Thanks for the clarification.

So a similar scheme could also work for the Python bindings: Call Py_INCREF 
instead of Obj_Inc_RefCount when passing a Clownfish object to Python space. 
This makes sure that the refcount is increased even for "immortal" objects.

>> I don't really like the "absurdly high refcount" idea for exactly that
>> reason. The actual problem with "immortal" objects is that they're shared
>> across threads and we want to avoid concurrent refcount manipulation. Maybe
>> this isn't an issue in the Python bindings due to the global interpreter
>> lock?
>
> Yes.  In Python, a C extension must explicitly *release* the GIL in order to
> enable concurrency.

But to rely on the GIL is a bad proposal anyway. We want Clownfish projects to 
be able to release the GIL and then the same thread-safety guarantees should 
hold for all host languages.

Nick

Re: [lucy-dev] Hosts, refcounts and destructors

[Marvin Humphrey <ma...@rectangular.com>]

On Tue, Feb 3, 2015 at 4:57 AM, Nick Wellnhofer <we...@aevum.de> wrote:
> On 03/02/2015 03:33, Marvin Humphrey wrote:
>>
>> Second, under Python, Clownfish objects use the Python refcount -- which
>> Python of course accesses directly, rather than going through the
>> Clownfish Inc_RefCount, Dec_RefCount and Get_RefCount wrappers.  This
>> presents problems for immortal classes like Class, Method, and BoolNum
>> which override those refcount manipulation methods to do nothing.  A
>> incref from Clownfish-space followed by a decref from Python-space will
>> cause the refcount to decrease, eventually triggering an immortal
>> object's destructor while valid references still exist.
>
>
> In the Perl bindings, the work-around is to intercept the destructor. I
> guess that's not possible in Python.

Actually, it is possible.

Python supports a hook for the destructor (`tp_dealloc`, which corresponds to
Clownfish's `Destroy` and is called by Py_DECREF and friends when the refcount
reaches 0):

    https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_dealloc

It also supports a separate hook for the code which frees the object
allocation (`tp_free`):

    https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_free

The complementary functions from object *creation* time are `tp_alloc`,
`tp_new`, and `tp_init`.

    http://eli.thegreenplace.net/2012/04/16/python-object-creation-sequence/

    `tp_alloc` is a low-level slot of the type object in CPython. It's not
    directly accessible from Python code, but should be familiar to C
    extension developers. A custom type defined in a C extension may override
    this slot to supply a custom memory allocation scheme for instances of
    itself...

So, we could theoretically supply versions of `tp_alloc` and `tp_free` which
call `malloc` and `free` internally instead of using Python's allocator.
I have not taken this path because the default implementation of `tp_alloc`
does a bunch of other stuff under debugging builds which I don't want to
duplicate.

However, what I *am* proposing is to install a custom `tp_dealloc`
for the "immortal" classes which resets the refcount and stops there.

If there's a reason that won't work, I haven't discovered it yet.

> (Is our Perl implementation even safe?
> Isn't it possible that Perl reuses the cached "inner object" SV if the Perl
> refcount drops to zero?)

We should be safe because I don't think that's possible.  Perl never gets at
the inner object directly: we give Perl a new RV every time a Clownfish object
needs to be accessed from Perl-space.  Creating a new RV causes the inner
object's refcount to rise by 1; when the RV ultimately goes away, it causes
the inner object's refcount to fall by 1, resulting in no net change.  And all
these refcount manipulations are thread-safe because the we `SvSHARE` the
inner object for "immortal" Clownfish types.

In the current implementation, intercepting DESTROY should only come into play
during Perl's global destruction.

But maybe to be safer, immortal classes should have DESTROY implementations
which clear the cached host object, instead of the no-op DESTROY
implementations they have now.

>> To address this issue, I think what we ought to do is take refcount
>> manipulation outside of method calls, instead manipulating refcounts
>> directly using macros which wrap either concrete or `static inline`
>> functions.  Immortal types can have their refcount set absurdly high on
>> object creation, and can have their destructors reset the refcount
>> rather than invoke deallocation.
>
> Some of this was already proposed when discussing the "immortal" flag. I
> also have a half-finished branch lying around somewhere.

Here's the thread and the branch for reference:

    http://markmail.org/message/e7h3kgq62c24zeqe
    http://mail-archives.apache.org/mod_mbox/lucy-dev/201408.mbox/%3C53DF557D.9030705%40aevum.de%3E
    https://git1-us-west.apache.org/repos/asf?p=lucy-clownfish.git;a=shortlog;h=refs/heads/clone_class_registry

>> Under that system, refcounts will be exceedingly unlikely to fall to 0
>> under normal circumstances.  However, we still have to take care not to
>> introduce a security vulnerability which happens when a destructor fires
>> on an immortal object in case someone figures out how to make that
>> happen artificially -- so I'd like to know if anybody sees problems now,
>> and I'd like to request that any commits I eventually supply receive
>> above-average scrutiny.
>
> I don't really like the "absurdly high refcount" idea for exactly that
> reason. The actual problem with "immortal" objects is that they're shared
> across threads and we want to avoid concurrent refcount manipulation. Maybe
> this isn't an issue in the Python bindings due to the global interpreter
> lock?

Yes.  In Python, a C extension must explicitly *release* the GIL in order to
enable concurrency.

    https://docs.python.org/3/c-api/init.html#thread-state-and-the-global-interpreter-lock

Marvin Humphrey

Re: [lucy-dev] Hosts, refcounts and destructors

[Nick Wellnhofer <we...@aevum.de>]

On 03/02/2015 03:33, Marvin Humphrey wrote:
> Second, under Python, Clownfish objects use the Python refcount -- which
> Python of course accesses directly, rather than going through the
> Clownfish Inc_RefCount, Dec_RefCount and Get_RefCount wrappers.  This
> presents problems for immortal classes like Class, Method, and BoolNum
> which override those refcount manipulation methods to do nothing.  A
> incref from Clownfish-space followed by a decref from Python-space will
> cause the refcount to decrease, eventually triggering an immortal
> object's destructor while valid references still exist.

In the Perl bindings, the work-around is to intercept the destructor. I guess 
that's not possible in Python. (Is our Perl implementation even safe? Isn't it 
possible that Perl reuses the cached "inner object" SV if the Perl refcount 
drops to zero?)

> To address this issue, I think what we ought to do is take refcount
> manipulation outside of method calls, instead manipulating refcounts
> directly using macros which wrap either concrete or `static inline`
> functions.  Immortal types can have their refcount set absurdly high on
> object creation, and can have their destructors reset the refcount
> rather than invoke deallocation.

Some of this was already proposed when discussing the "immortal" flag. I also 
have a half-finished branch lying around somewhere.

> Under that system, refcounts will be exceedingly unlikely to fall to 0
> under normal circumstances.  However, we still have to take care not to
> introduce a security vulnerability which happens when a destructor fires
> on an immortal object in case someone figures out how to make that
> happen artificially -- so I'd like to know if anybody sees problems now,
> and I'd like to request that any commits I eventually supply receive
> above-average scrutiny.

I don't really like the "absurdly high refcount" idea for exactly that reason. 
The actual problem with "immortal" objects is that they're shared across 
threads and we want to avoid concurrent refcount manipulation. Maybe this 
isn't an issue in the Python bindings due to the global interpreter lock?

Nick