fencepost buglet in memcached-1.2.8 items.c

[Mat Hostetter]

In items.c in memcached-1.2.8, these arrays allow indices up to but
not including LARGEST_ID:

#define LARGEST_ID 255
...
static item *heads[LARGEST_ID];
static item *tails[LARGEST_ID];

So this comment (which appears twice) is confused:

/* always true, warns: assert(it->slabs_clsid <= LARGEST_ID); */

The assert should be checking for clsid < LARGEST_ID, not <=.
And then the warning would go away. Or was the intent always to have
256
entries in the arrays, not 255?

Similarly, this code in items.c should be checking >=, not >, as
heads[LARGEST_ID] would be out-of-bounds:

if (slabs_clsid > LARGEST_ID) return NULL;
it = heads[slabs_clsid];

IMHO the confusion stems from the fact that LARGEST_ID is not itself a
valid id, as the name seems to imply.

-Mat

[Dustin]

On Jun 2, 5:03 pm, Mat Hostetter <mjhostet...@gmail.com> wrote:
> In items.c in memcached-1.2.8, these arrays allow indices up to but
> not including LARGEST_ID:

Good calls. For bonus points, do you think you can contrive some
tests that trigger these? :)

[Mat Hostetter]

I think it's actually impossible to trigger this fencepost problem,
because the maximum possible slab class ID (POWER_LARGEST) is smaller
than LARGEST_ID. The code is "wrong" but it doesn't matter.

If I ran the zoo, I would clean this up by unifying POWER_LARGEST and
LARGEST_ID. I think it's reasonable for the slab allocator to export
the largest value slab_clsid() can possibly return,rather than making
the caller guess. This has the minor benefit that various items.c
arrays would not waste 54 entries for impossible slab class IDs.

-Mat