[PATCH 1/5] dsdb: Fix talloc issues in dsdb_schema_copy_shallow

[Andrew Bartlett]

On Tue, 2013-01-15 at 18:11 +0100, Stefan (metze) Metzmacher wrote:
> Hi Andrew,
> 
> > From 6f8a4ebabe7228d5c9f2bf33066a00d6126dec78 Mon Sep 17 00:00:00 2001
> > From: Andrew Bartlett <abartlet at samba.org>
> > Date: Sat, 17 Nov 2012 11:49:25 +1100
> > Subject: [PATCH 1/5] dsdb: Fix talloc issues in dsdb_schema_copy_shallow
> > 
> > The problem was that we did not clear these structure members before
> > we regenerated these values in schema_fill_constructed().  The
> > talloc_realloc() in the schema_fill_possible_inferiors() and
> > schema_fill_system_possible_inferiors() calls would then use the old
> > talloc parent (cls, a child of schema) not cls_copy (a child of
> > schema_copy).
> > 
> > We also did not take a reference to the original class, to ensure the
> > other pointers remained valid.
> > 
> > As well as altering the original values in cls, if in the meantime the
> > original talloc parent (schema) would go away, we would use memory
> > after free().  (This is how I found the issue).
> > 
> > Andrew Bartlett
> > ---
> >  source4/dsdb/schema/schema_init.c | 32 ++++++++++++++++++++++++++++----
> >  1 file changed, 28 insertions(+), 4 deletions(-)
> > 
> > diff --git a/source4/dsdb/schema/schema_init.c b/source4/dsdb/schema/schema_init.c
> > index 752d4f5..0e8a3d5 100644
> > --- a/source4/dsdb/schema/schema_init.c
> > +++ b/source4/dsdb/schema/schema_init.c
> > @@ -75,21 +75,45 @@ struct dsdb_schema *dsdb_schema_copy_shallow(TALLOC_CTX *mem_ctx,
> >  
> >  	/* copy classes and attributes*/
> >  	for (cls = schema->classes; cls; cls = cls->next) {
> > -		struct dsdb_class *class_copy = talloc_memdup(schema_copy,
> > -							      cls, sizeof(*cls));
> > +		struct dsdb_class *class_copy = talloc(schema_copy, struct dsdb_class);
> >  		if (!class_copy) {
> >  			goto failed;
> >  		}
> > +		
> > +		/* 
> > +		 * We must take a reference here, otherwise the
> > +		 * pointers in class_copy might become invalid if the
> > +		 * original schema goes away first.
> > +		 */
> > +		if (!talloc_reference(class_copy, cls)) {
> > +			goto failed;
> > +		}
> 
> I think we should not add a talloc_reference() here,
> unless you can prove that it's 100% needed.

When I wrote this, I did, as far as I recall, using valgrind.  The only
reason i found this in the first place is because of valgrind finding
the use-after free, and then clearing that warning using this work.  It
seems cleanest to reference exactly the thing I'm copying, to keep the
reference to exactly what the child pointers will be on.

If we really can't progress here, the other option would seem to be to
put the schema into IDL, and pull/push it to a blob so as to get new,
unique and copies of all the pointers. 

In any case, while this patch was part of my testing, I'm most
interested in your views on the other patches, which seem to have made a
positive difference (even if we have to move the knownfail to the new
test made to fail, I think it's a positive change). 

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org