talloc issues

[tridge at samba.org]

Hi Metze,

 > I have some code here:
 > http://gitweb.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-talloc-metze
 > 
 > I'll reply more verbose tomorrow or next week.

I'll be interested in the explanation, but this seems like a pretty
dangerous change in behaviour to me. Do you have any plans on ways to
find existing code that depends on the old behaviour?

The specific types of changes I am concerned about are:

 in module 1:
    allocate memory

 in module 2:
    take reference as child of x
    call talloc_free on referenced pointer
    talloc_free x

or for a more specific example:

static bool test_newref(void)
{
	struct somestruct {
		int *ptr;
	};
	struct somestruct *s1, *s2;
	void *root = talloc_new(NULL);

	s1 = talloc(root, struct somestruct);
	s1->ptr = talloc(s1, int);

	*s1->ptr = 42;

	s2 = talloc(root, struct somestruct);
	s2->ptr = talloc_reference(s2, s1->ptr);

	talloc_free(s2->ptr);
	talloc_free(s2);

	printf("%d\n", *s1->ptr);
	return true;
}

    

With the original behaviour of talloc you get '42' printed and no
errors. With the changes I put in you get this message printed:

  ERROR: talloc_free with references at testsuite.c:1126
         reference at testsuite.c:1124

but still get the same result printed and no valgrind errors.

With the code in your wip branch, I get a valgrind error, as the
printf of *s1->ptr references freed memory.

One of the things I'm trying to achieve with these talloc changes is:

  - when there is a change in behaviour that could affect code logic,
    make sure we produce a warning in the logs

  - err on the side of leaking memory if necessary (with a warning),
    rather than leaving code with what it thinks is valid pointers
    that it could then dereference

The 2nd point is particularly important, as otherwise we risk creating
a security hole. The above example shows how the hole could be
produced. We just don't have any sane way that I can think of to audit
all our code for a change like this. I don't think 'make test' is a
sufficient test to be sure we haven't created a security hole
somewhere by reversing the behaviour of talloc_free() in this sort of
example.

I also don't like this approach as it re-introduces a separation of
the 'original' parent as being special, which I think is what got us
in so much trouble in the first place. 

Cheers, Tridge