talloc issues

Sam Liddicott sam at liddicott.com
Tue Jul 28 02:25:45 MDT 2009


* tridge wrote, On 28/07/09 03:02:

> I don't always know when you are proposing something seriously and
> when you are not. 

I recognize my fault here :-)

> If you are seriously proposing the above then I'd
> like to know how you plan to audit the more than 9k existing calls to
> talloc_free(), and how you plan on changing those calls to zero the
> callers pointer when the C language doesn't allow that with the
> existing syntax.

We can divide uses of talloc_free into
a. samba3 uses
b. samba4 uses,
c. combined uses (in lib dir)

I think can suppose that samba3 uses are always legitimate, and must
then consider the other two cases.

Here I check the libdir uses.

I find 245 talloc_free references there, 270 including talloc_steal:

$ echo `find ../lib -name '*.c' | xargs grep -ic talloc_free |\
grep -v source3 | grep ':[^0]' | sed '1i0
s/.*:/+ /' ` | bc

We must audit each use for each passed pointer is ever freed or stolen.

We can do a quick visual audit like this and filter out most harmless cases.
$ find ../lib -name '*.c' | \
xargs grep -i -n -C25 'talloc_free\|talloc_steal' |\
less -i -ptalloc

Most harmelss cases are:

1. free of something allocated in that function; e.g.
  talloc_zero...
  ..
  if (fail) { talloc_free.. return }
- these are all safe

2. free/steal of library data - freeing of structs allocated within the
library and not against a user context, or against a user context when
the allocation is never returned.
- these are all safe

3. a documented destructor, like file_unmap
- these are NOT all safe if callers are passing a sneaky reference which
they hope will be "freed first" to preserve the real parent.

We can also recognize other types of harmless case and produce a list of
remaining cases where it is difficult to say if we are taking undocument
ownership from user pointers.

Using this technique we can filter out simple uses in a couple of
seconds each; and examine the rest in detail.

The first uncertain use I came across is:
./lib/util/util_file.c-255-bool unmap_file(void *start, size_t size)
I can't easily make the judgement on that, but I read the map and unmap
functions and think that it is fine. We can consider that callers cannot
sanely have made a sneaky reference (see (3) above) because this would
not make sense for the MMAP variant.

The next one is:
./lib/util/util_file.c:292:    talloc_steal(ret, p);
and I see from the comment for that function that the use of steal is
documented. As steal only affect the parent, any incorrect use here will
not be because of talloc changes. We can also consider the talloc_free a
few lines above. Because of the talloc_steal uses we expect that the
caller did not pass a convenience talloc_reference to be free'd on
failure in order to preserve the parents ownership.

And that's all for that file.

I just keep paging down through "less" noting any non-simple cases.

I expect most of the lib uses to be safe because of the samba3 history.

More complicated will be the samba4 uses because there seem to be less
comments about whether or not a function call steals or frees; in some
cases the main author for a module will have to do the audit and add
these comments.

However it is only reasoning that can certify a use, so it can't be done
by machine.

I am volunteering to do some of the work.

I suggest we branch the master and on the wiki post the output of:
find . -name '*.c' | xargs grep -i -n  'talloc_free\|talloc_steal',
perhaps one page per file.

For each line, we can either dismiss it as simple and harmless, or note
concerns to be addressed later.

We will have to do something like this whatever talloc solution we chose.

> 
> You seem to be treating this problem as only one of what you would use
> for the semantics if you were creating talloc from scratch.  That is
> nice as a thought experiment, but please separate this from the
> problem of dealing with a large existing code base. I have seen
> nothing in your proposals on how to deal in any sane way with the
> existing code.

This is true; up till now because I had not imagined that talloc would
actually used this way. However, here we are, and I try to adopt a
pragmatic mindset.

>  > No; the original parent was ALWAYS special, we just refused to treat it
>  > as special consistently and that is where the problem came from.
>  > 
>  > It was always special externally because talloc_zero, talloc_steal
>  > always treated it specially, and talloc_free also but not so consistently.
> 
> It was treated asymmetrically previously. The changes I've put in now
> get rid of that. As far as I know, the only external way you can tell
> the difference between parents in the current code is with a
> talloc_parent() call, or by calling a talloc_report*() call to print
> out the hierarchy.

also indirectly by calling talloc_steal or talloc_free and then watching
for a dangling pointer later (that's how I found it).

> So right now we have a system where all parents are treated the
> same. I am reluctant to go back to a system where one parent is
> special.

That's fine as a position as long as we know it wasn't the specialness
that was the original problem, just inconsistent specialness. We don't
need to be afraid of specialness.

> To understand this reluctance I think you will need to look at how
> talloc_reference() has actually been used in the code. Have a look for
> example at libnet_RpcConnectSrv_recv(). That function takes a
> temporary samr pipe and uses a reference to make it a child of a long
> term context. When the temporary samr connection goes away, the long
> term parent remains. Thus the temporary connection is not special in
> any way except that it happened to be the first samr connection.

Yes, the original parent is only special when talloc_free is called
explicitly, or talloc_steal. I try to quarantine the specialness by
calling it a "default argument to talloc_free and talloc_steal" and
pretending otherwise that it is an implicit member of tc->refs.

Then the specialness is entirely confined to it's area of effect.

Sam


More information about the samba-technical mailing list