[PATCH] minor tdb fixes

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] minor tdb fixes

Samba - samba-technical mailing list
Hi!

Attached is a small patchset that fixes the BUCKET() macro in tdb and the offset
calculations.

I stumpled upon this when looking at the `tdbtool DB list` output: the freelist
was showing the entries from another chain.

Turns out that the BUCKET(-1) macro returns wrong results because of sign
conversion, eg -1 is sign converted to an unsigned int 4294967295 and 4294967295
% 10 = 5. The expected result is -1, not 5.

This doesn't cause more severe issues because we consistenly lock the wrong
chain when trying to lock the freelist. It doesn't deadlock because in tdb_store
we lock the freelist before the hashchain. And finally in freelist.c we use the
FREELIST_TOP define directly, not the TDB_HASH_TOP macro that uses BUCKET.

Thoughts?

Cheerio!
-slow
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] minor tdb fixes

Samba - samba-technical mailing list
Hi Ralph,

> Attached is a small patchset that fixes the BUCKET() macro in tdb and the offset
> calculations.
>
> I stumpled upon this when looking at the `tdbtool DB list` output: the freelist
> was showing the entries from another chain.
>
> Turns out that the BUCKET(-1) macro returns wrong results because of sign
> conversion, eg -1 is sign converted to an unsigned int 4294967295 and 4294967295
> % 10 = 5. The expected result is -1, not 5.
>
> This doesn't cause more severe issues because we consistenly lock the wrong
> chain when trying to lock the freelist. It doesn't deadlock because in tdb_store
> we lock the freelist before the hashchain. And finally in freelist.c we use the
> FREELIST_TOP define directly, not the TDB_HASH_TOP macro that uses BUCKET.
>
> Thoughts?
Sadly it's now a feature as existing tdb versions use this logic.
So we can't just fix this as it's an incompatible change.

metze



signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] minor tdb fixes

Samba - samba-technical mailing list
On Mon, Jul 03, 2017 at 10:06:28AM +0200, Stefan Metzmacher wrote:

> > Attached is a small patchset that fixes the BUCKET() macro in tdb and the offset
> > calculations.
> >
> > I stumpled upon this when looking at the `tdbtool DB list` output: the freelist
> > was showing the entries from another chain.
> >
> > Turns out that the BUCKET(-1) macro returns wrong results because of sign
> > conversion, eg -1 is sign converted to an unsigned int 4294967295 and 4294967295
> > % 10 = 5. The expected result is -1, not 5.
> >
> > This doesn't cause more severe issues because we consistenly lock the wrong
> > chain when trying to lock the freelist. It doesn't deadlock because in tdb_store
> > we lock the freelist before the hashchain. And finally in freelist.c we use the
> > FREELIST_TOP define directly, not the TDB_HASH_TOP macro that uses BUCKET.
> >
> > Thoughts?
>
> Sadly it's now a feature as existing tdb versions use this logic.
> So we can't just fix this as it's an incompatible change.

oh, that sucks, but you're right.

I'll prepare patches that fix `tdbtool DB list` differently and that document
the broken BUCKET macro.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] minor tdb fixes

Samba - samba-technical mailing list
On Mon, Jul 03, 2017 at 10:25:19AM +0200, Ralph Böhme wrote:

> On Mon, Jul 03, 2017 at 10:06:28AM +0200, Stefan Metzmacher wrote:
> > > Attached is a small patchset that fixes the BUCKET() macro in tdb and the offset
> > > calculations.
> > >
> > > I stumpled upon this when looking at the `tdbtool DB list` output: the freelist
> > > was showing the entries from another chain.
> > >
> > > Turns out that the BUCKET(-1) macro returns wrong results because of sign
> > > conversion, eg -1 is sign converted to an unsigned int 4294967295 and 4294967295
> > > % 10 = 5. The expected result is -1, not 5.
> > >
> > > This doesn't cause more severe issues because we consistenly lock the wrong
> > > chain when trying to lock the freelist. It doesn't deadlock because in tdb_store
> > > we lock the freelist before the hashchain. And finally in freelist.c we use the
> > > FREELIST_TOP define directly, not the TDB_HASH_TOP macro that uses BUCKET.
> > >
> > > Thoughts?
> >
> > Sadly it's now a feature as existing tdb versions use this logic.
> > So we can't just fix this as it's an incompatible change.
>
> oh, that sucks, but you're right.
>
> I'll prepare patches that fix `tdbtool DB list` differently and that document
> the broken BUCKET macro.
attached.

Please review & push if ok. Thanks!

Cheerio!
-slow
Loading...