Crash in CLEAR_IF_FIRST handling in tdb

[Jeremy Allison]

On Tue, Oct 02, 2012 at 04:35:31PM +0200, Volker Lendecke wrote:
> Hi Rusty!
> 
> Under
> http://git.samba.org/?p=vl/samba.git/.git;a=shortlog;h=refs/heads/tdb
> find a patchset that for me fixes a crash in winbind in tdb.
> For the explanation, see the second patch from the top.
> 
> Comments?

Ok, as requested I'm looking into this code :-).

I have a question about the second patch in the
series, "tdb: Make tdb_expand robust against shrinking tdbs".

This adds an fstat into the tdb_expand_file()
call just before calling ftruncate, and updates
tdb->map_size and the passed in size, which
is actually the value tdb->map_size (so the
size variable is actually redundent, but
that's another matter).

tdb_expand_file() is called from tdb_expand(), in
the code that looks like this:

        if (tdb_lock(tdb, -1, F_WRLCK) == -1) {
                TDB_LOG((tdb, TDB_DEBUG_ERROR, "lock failed in tdb_expand\n"));
                return -1;
        }

        /* must know about any previous expansions by another process */
        tdb->methods->tdb_oob(tdb, tdb->map_size, 1, 1);

        size = tdb_expand_adjust(tdb->map_size, size, tdb->page_size);

        /* expand the file itself */
        if (!(tdb->flags & TDB_INTERNAL)) {
                if (tdb->methods->tdb_expand_file(tdb, tdb->map_size, size) != 0)
                        goto fail;
        }

However, inside tdb_oob() we already call
fstat(tdb->fd, &st), and then set:
tdb->map_size = st.st_size;
before returning from the tdb_oob() call.

So we've already done an fstat and updated
tdb->map_size under the F_WRLCK inside tdb_expand(),
so why is this particular patch needed ?

If there was a race condition between the

tdb->methods->tdb_oob()

call and the

tdb->methods->tdb_expand_file()

call this patch would shrink it, but not remove it.
So I'm wondering if this particular patch is really
needed ?

Or am I just hopelessly missing something (wouldn't
be the first time with tdb :-) ?

Thanks Volker,

Cheers,

	Jeremy.