[Xcb] Invalid XIDs

[Nathaniel Smith]

On Sat, Jan 24, 2009 at 5:41 PM, Barton C Massey <bart at cs.pdx.edu> wrote:
> Note that once xid.last becomes greater than xid.max
> xcb_generate_id() will never again request a new xid block
> but will happily keep allocating bogus xids, which is
> apparently what has happened to you.

Right.

> Aside from the obvious way this could happen (something
> elsewhere setting xid.last deliberately or accidentally),
> there's at least one code path that could do it here, namely
> if range->count comes back as 1 (or 0, but presumably that
> would be illegal).

Checking X.org, it looks like the exact rules are:
  if there is at least one XID available:
    range->start_id is the first available xid
    range->start_id + count is the first *un*available xid
  if there are no XIDs available:
    range->start_id is 0
    range->count is 1 (??)
This latter part is arguably a bug in the X server or something, but I
don't see how it would ever matter, and anyway, yes, count is always
at least 1.

> In that case, xid.last == xid.max and
> then we increment xid.last.  I think the - 1 in this line
>
>  c->xid.max = range->start_id + (range->count - 1) * c->xid.inc;
>
> is just a bug as the code stands.  The invariant the code
> maintains is that outside of the function xid.last (bad
> name) contains the next xid to be returned.  Thus, in
> addition to being potentially buggy the code in question
> "wastes" an xid out of every block.

Good call -- that seems plausible.

> Why don't you try this code instead?  I changed the
> invariant so that outside of the function xid.last really is
> the xid allocated on the last call and xid.max really is the
> maximum xid to be allocated in this block.  I also changed
> the block allocation test and added a couple of assertions
> so that if this code fails it will fail early.

That definitely looks more correct (whether it fixed my bug or not
:-)).  I think the actual invariants are perhaps clearer with the
asserts and tests arranged like this?:

     assert(c->xid.last <= c->xid.max);
     if(c->xid.last == c->xid.max)
     {
         xcb_xc_misc_get_xid_range_reply_t *range;
         range = xcb_xc_misc_get_xid_range_reply(c,
                   xcb_xc_misc_get_xid_range(c), 0);
         if(!range)
         {
             pthread_mutex_unlock(&c->xid.lock);
             return -1;
         }
         assert(range->count > 0);
         c->xid.last = range->start_id;
         c->xid.max = range->start_id + (range->count - 1) * c->xid.inc;
         free(range);
     } else {
         c->xid.last += c->xid.inc;
     }
     assert(c->xid.last <= c->xid.max);
     ret = c->xid.last | c->xid.base;
     pthread_mutex_unlock(&c->xid.lock);
     return ret;

Anyway, since the crash is so rare (occurs maybe once a week,
unpredictably), it's very hard to tell whether a patch works or not by
trying it.  ("It's been a week without a crash... so did we fix it or
just get lucky?").  So I'm going to instrument the range resetting
code instead, and the next time the crash happens we'll know for sure
whether it was proceeded by a count==1 reply.

Thanks!

-- Nathaniel