[Xcb] bug report

[Uli Schlachter]

On 10.12.2011 12:22, Sidney Cadot wrote:
> Hi all,
> 
> Is this the appropriate place for bug reports on the xcb codebase? If
> so, see below for a bug in xcb_get_auth_info().
> 
> It is not entirely clear to me what the function is trying to
> accomplish (I feel the code is rather unclear, to be honest), so I
> cannot propose a patch. Nonetheless, I would appreciate it is this bug
> were fixed as it continuously shows up when valgrind'ing my programs.
> 
> Best regards, SIdney
> 
> 
> In src/xcb_auth.c the function "_xcb_get_auth_info" is defined as follows:
> 
>    289  int _xcb_get_auth_info(int fd, xcb_auth_info_t *info, int display)
>    290  {
>    291      /* code adapted from Xlib/ConnDis.c, xtrans/Xtranssocket.c,
>    292         xtrans/Xtransutils.c */
>    293      struct sockaddr *sockname = NULL;
>    294      int gotsockname = 0;
>    295      Xauth *authptr = 0;
>    296      int ret = 1;
>    297
>    298      /* Some systems like hpux or Hurd do not expose peer names
>    299       * for UNIX Domain Sockets, but this is irrelevant,
>    300       * since compute_auth() ignores the peer name in this
>    301       * case anyway.*/
>    302      if ((sockname = get_peer_sock_name(getpeername, fd)) == NULL)
>    303      {
>    304          if ((sockname = get_peer_sock_name(getsockname, fd)) == NULL)
>    305              return 0;   /* can only authenticate sockets */
>    306          if (sockname->sa_family != AF_UNIX)
>    307          {
>    308              free(sockname);
>    309              return 0;   /* except for AF_UNIX, sockets should
> have peernames */
>    310          }
>    311          gotsockname = 1;
>    312      }
>    313
>    314      authptr = get_authptr(sockname, display);
>    315      if (authptr == 0)
>    316      {
>    317          free(sockname);
>    318          return 0;   /* cannot find good auth data */
>    319      }
>    320
>    321      info->namelen = memdup(&info->name, authptr->name,
> authptr->name_length);
>    322      if (!info->namelen)
>    323          goto no_auth;   /* out of memory */
>    324
>    325      if (!gotsockname && (sockname =
> get_peer_sock_name(getsockname, fd)) == NULL)
>    326      {
>    327          free(info->name);
>    328          goto no_auth;   /* can only authenticate sockets */
>    329      }
>    330
>    331      ret = compute_auth(info, authptr, sockname);
>    332      if(!ret)
>    333      {
>    334          free(info->name);
>    335          goto no_auth;   /* cannot build auth record */
>    336      }
>    337
>    338      free(sockname);
>    339      sockname = NULL;
>    340
>    341      XauDisposeAuth(authptr);
>    342      return ret;
>    343
>    344   no_auth:
>    345      free(sockname);
>    346
>    347      info->name = 0;
>    348      info->namelen = 0;
>    349      XauDisposeAuth(authptr);
>    350      return 0;
>    351  }
> 
> This code has a memory leak:
> 
> (1) In line 302, get_peer_sock_name() is called. Supposing it returns
> a non-NULL value, it is a malloc'ed string that must be freed. Note
> that if this is the case, the variable 'gotsockname' is /not/ set on
> line 311. It remains at zero.
> 
> (2) Proceeding to line 325: since 'gotsockname' is now zero, another
> call to get_peer_sock_name() is executed. The value of 'sockname' is
> overwritten with a new malloced string. Since the previous value was
> not free()d at this point, we have a memory leak.

This was already fixed in git:

http://cgit.freedesktop.org/xcb/libxcb/commit/?id=5755582444ad0ba79e661ab3173cc38e9e588d83

So now we just need a new libxcb release...

Uli
-- 
"Why make things difficult, when it is possible to make them cryptic
and totally illogical, with just a little bit more effort?" -- A. P. J.