[PATCH] Messaging improvements and fixes needed for auth logging

[Andrew Bartlett]

On Mon, 2017-03-20 at 21:52 +0100, Volker Lendecke via samba-technical
wrote:
> On Tue, Mar 21, 2017 at 09:02:10AM +1300, Andrew Bartlett wrote:
> > > Can you explain a bit more what is going on here?
> > 
> > Certainly.  Have you run the test I've added?  (make test
> > TESTS=messaging triggers it nicely). 
> 
> No, I haven't run the test. Can you explain in English words what the
> bug is please?

Due to a cast in server_id_db_lookup() strv_count() will walk off the
end of a zero-length talloc pointer in search of the terminating NULL.

> > >  I would like to
> > > avoid DATA_BLOB and/or TDB_DATA where it makes sense. Here we
> > > always
> > > return a talloc'ed object that carries its own length. I think
> > > that a
> > > talloc objects is just as expressive as a DATA_BLOB, you can
> > > always
> > > query its length with talloc_get_size.
> > > 
> > > I would like to understand the bug that this fixes that is not
> > > fixable
> > > with keeping just the uint8_t* return from tdb_fetch_talloc().
> > 
> > The callers otherwise assumed it was a NULL terminated string, and
> > wandered off the end of the string.  
> 
> I could understand that if talloc_get_size returned a char*. But it's
> a
> uint8_t* that is returned. There I would assume the caller to be
> smart
> and understand this is basically a blob.

I'm very sorry, I have to disagree.  The only caller of
tdb_fetch_talloc() (there are no tests) got it wrong, and the compiler
didn't notice either.  I think this shows that the API should be
clearer.  We have very well used abstractions for length-limited data,
and we should make the best use of them possible.

As context: because strv_delete() is based on talloc_realloc(),
talloc_get_size() returns 0 for the 'removed last entry' case (as it is
a NULL pointer), but strlen()+1 for all other cases.  This causes the
\0 terminator to be omitted when saved into the db via
talloc_tdb_data().

Thanks,

Andrew Bartlett