[RFC] Making talloc_reference() safer.

[Stefan (metze) Metzmacher]

Am 26.10.2011 18:37, schrieb Jeremy Allison:
> On Wed, Oct 26, 2011 at 08:45:23PM +1100, Andrew Bartlett wrote:
>> On Mon, 2011-10-24 at 16:41 +1030, Rusty Russell wrote:
>>> Hi all,
>>>
>>>         git://git.samba.org/rusty/samba.git #talloc-reference-check-wip
>>>
>>>         I had the fun of re-arguing talloc_reference() safety with
>>> Tridge last Thursday.  We agree that talloc_reference fills a real need,
>>> but we can make it safer by expanding the checks which differentiate
>>> normal from refcounted objects.
>>>
>>> Firstly, note that talloc_reference() has several real use cases:
>>> anywhere that reference counts would normally be used.  The patterns
>>> I've seen are:
>>>
>>> 1) A "cache" of objects, where the cache may evict objects.  The cache
>>>    wants to hold a reference, as do the callers, and the object is freed
>>>    if it's evicted from the cache *and* noone else has referenced it.
>>>
>>> 2) A "dealer" of single objects, such as the tdb_wrap code which will
>>>    keeps track of all tdbs and avoids reopening the same tdb by handing
>>>    back a referenced tdb.
>>
>> Which of these do the python bindings fit into?
>>
>> All samba talloc objects can be exposed into python, and in particular
>> NDR objects are used in python a lot.  The current talloc modal works
>> well here, as we take a talloc reference each time we expose the object
>> into python, and we take a talloc reference each time we hold an
>> python-originated object into a long-term C structure (such as
>> gensec_start needing credentials).  This ensures that the object, which
>> in C may be held by implicit rules of 'you would be mad to free *that*
>> before this finishes' isn't taken away by python's garbage collector.
>>
>> The particular challenge is that any object we have a python binding for
>> needs this behaviour, and we expose a *lot* of objects to python (every
>> IDL struct, and many other interfaces besides).
>>
>> I'm concerned that the discussion so far has not addressed this
>> important use case, yet Samba's python bindings have become an integral
>> part of the project, and cannot be ignored. 
> 
> Is this the pytalloc_reference() call you mean ? I'm trying to
> understand the usage case here.
> 
> Other code exposes C data into python without using internal
> reference counts - in fact doesn't python have just this
> method via the Py_INCREF() function (Warning: I am not a python
> programmer, just someone reading the docs :-).
> 
> Look at the 1.10.1. Reference Counting in Python
> section here:
> 
> http://docs.python.org/extending/extending.html
> 
> As most users of pytalloc_reference are in auto generated
> code, can't this be changed to use the "normal" python
> method here ? (Another Warning: I am not a python programmer,
> just someone reading the docs :-).

I'd also prefer if we use python ref counting in the python bindings
instead of talloc_reference().

That might be needed anyway in order to fix this bug:
https://bugzilla.samba.org/show_bug.cgi?id=7902
We didn't get the ref counting right in all cases.

For things like cli_credentials and gensec_setting I'd you a model
like for tdb_wrap.

We would split a cli_credentials_state from cli_credentials.

struct cli_credentials {
	struct cli_credentials_state *state;
}

Where struct cli_credentials would be the variable the callers see.
struct cli_credentials_state is private. struct cli_credentials
would have a reference to struct cli_credentials_state
and we would have a

struct cli_credentials *cli_credentials_reference(TALLOC_CTX *mem_ctx,
						  struct cli_credentials *creds)
{
	struct cli_credentials *c;

	c = talloc(mem_ctx, struct cli_credentials);
	if (c == NULL) {
		return NULL;
        }

        c->state = talloc_reference(c, creds->state);
	if (c->state == NULL) {
		talloc_free(c);
		return NULL;
	}

	return c;
}

This way the caller can do what every he wants with the returned pointer,
but has no chance to call talloc_free() explicitly on the ref counted state.

metze

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20111026/fa7373c2/attachment.pgp>