[PATCH] Messaging improvements and fixes needed for auth logging

[Volker Lendecke]

On Tue, Mar 21, 2017 at 12:22:42PM +1300, Andrew Bartlett wrote:
> 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.

That's a bug in server_id. It's great that you found it, thanks.
 
> 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().

Please don't change the API. Two reasons:

Better talloc integration: It's not as easy to make a DATA_BLOB a
talloc context as with just a uint8_t* that also carries its own
length, just like a DATA_BLOB does.

Easier re-use: With a DATA_BLOB in the API, it's just too easy to pull
in half of Samba. Even ctdb has not everything pulled in, and ctdb is
a very close partner of Samba.

Thanks,

Volker