[Xcb] [PATCH 1/2] xcb_disconnect(): Fix leak with error connections

[Uli Schlachter]

Hi,

On 31.12.2013 22:58, Josh Triplett wrote:
> On Tue, Dec 31, 2013 at 03:57:16PM +0100, Uli Schlachter wrote:
>> There are two kind of error connections in XCB. First, if something goes wrong
>> while the connection is being set up, _xcb_conn_ret_error() is used to return a
>> static connection in an error state. If something goes wrong later,
>> _xcb_conn_shutdown() is used to set c->has_error.
>>
>> This is important, because the static object that _xcb_conn_ret_error() returns
>> must not be freed, while the dynamically allocated objects that go through
>> _xcb_conn_shutdown() must obviously be properly deallocated.
>>
>> This used to work correctly, but in 769acff0da8, xcb_disconnect() was made to
>> ignore all connections in an error state completely. Fix this by only ignoring
>> the few static error connections that we have.
>>
>> This was tested with the following hack:
>>
>>     xcb_connection_t *c = xcb_connect(NULL, NULL);
>>     close(xcb_get_file_descriptor(c));
>>     xcb_discard_reply(c, xcb_get_input_focus(c).sequence);
>>     xcb_flush(c);
>>     xcb_disconnect(c);
>>
>> Valgrind confirms that xcb has a memory leak before this patch that this patch
>> indeed fixes.
>>
>> Signed-off-by: Uli Schlachter <psychon at znc.in>
>> ---
>>  src/xcb_conn.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/xcb_conn.c b/src/xcb_conn.c
>> index 46390e1..9da11b0 100644
>> --- a/src/xcb_conn.c
>> +++ b/src/xcb_conn.c
>> @@ -64,6 +64,7 @@ typedef struct {
>>      uint16_t length;
>>  } xcb_setup_generic_t;
>>  
>> +/* Keep this list in sync with xcb_disconnect()! */
>>  static const int xcb_con_error = XCB_CONN_ERROR;
>>  static const int xcb_con_closed_mem_er = XCB_CONN_CLOSED_MEM_INSUFFICIENT;
>>  static const int xcb_con_closed_parse_er = XCB_CONN_CLOSED_PARSE_ERR;
> 
> Rather than a list that has to be kept in sync in two non-adjacent
> places, how about writing a static inline is_static_error_conn function
> and keeping it right next to these?

We could just move xcb_disconnect() up to be right next to these. However, I
fear that this is still too far away and can easily be missed.

Instead, if we want to only have the list in one place, we could move the static
ints into an array and "simplify" _xcb_conn_ret_error() a bit:

static const int xcb_con_errors[] = {
  XCB_CONN_ERROR, XCB_CONN_CLOSED_MEM_INSUFFICIENT, ...
  /* To simplify _xcb_conn_ret_error(), I would mention all errors here */
};

void xcb_disconnect(xcb_connection_t *c)
{
  size_t i;
  /* We don't have an ARRAY_SIZE macro, is this for loop still clear enough? */
  for (i = 0; i < sizeof(xcb_con_errors) / sizeof(xcb_con_errors[0]); i++)
    if (c == (xcb_connection_t *c) &xcb_con_errors[i])
      return;
[...]

xcb_connection_t *_xcb_conn_ret_error(int err)
{
   assert(err > 0 && err <= sizeof(xcb_con_errors) / sizeof(xcb_con_errors[0]);
   assert(xcb_con_errors[err - 1] == err);
   return (xcb_connection_t *) &xcb_con_errors[err - 1];
}

Happy new year,
Uli
-- 
- Captain, I think I should tell you I've never
  actually landed a starship before.
- That's all right, Lieutenant, neither have I.