Bug 28526

Summary: sockname leaked in _xcb_get_auth_info
Product: XCB Reporter: Alan Coopersmith <alan.coopersmith>
Component: LibraryAssignee: xcb mailing list dummy <xcb>
Status: RESOLVED FIXED QA Contact: xcb mailing list dummy <xcb>
Severity: minor    
Priority: medium CC: lists
Version: unspecified   
Hardware: Other   
OS: Solaris   
Whiteboard:
i915 platform: i915 features:
Attachments: Fix memory leak in _xcb_get_auth_info.

Description Alan Coopersmith 2010-06-13 11:03:21 UTC 
Building from git commit 3f79628becbd3b0eff1aef804902eb739fac4403
our leak checker found a memory leak of sockname in _xcb_get_auth_info()

Stepping through in the debugger I can see this happening:

  302       if ((sockname = get_peer_sock_name(getpeername, fd)) == NULL)

returns a newly allocated pointer for sockname, but doesn't set gotsockname.
(gotsockname is only set in the == NULL case, which I don't understand.)

We then use that pointer to get the authptr:

(dbx) n
stopped in _xcb_get_auth_info at line 314 in file "xcb_auth.c"
  314       authptr = get_authptr(sockname, display);
(dbx) n
stopped in _xcb_get_auth_info at line 315 in file "xcb_auth.c"
  315       if (authptr == 0)
(dbx) n
stopped in _xcb_get_auth_info at line 321 in file "xcb_auth.c"
  321       info->namelen = memdup(&info->name, authptr->name, authptr->name_length);
(dbx) n
stopped in _xcb_get_auth_info at line 322 in file "xcb_auth.c"
  322       if (!info->namelen)
(dbx) print sockname
sockname = 0x806c618
(dbx) n
stopped in _xcb_get_auth_info at line 325 in file "xcb_auth.c"
  325       if (!gotsockname && (sockname = get_peer_sock_name(getsockname, fd)) == NULL)
(dbx) print gotsockname
gotsockname = 0
(dbx) n      
stopped in _xcb_get_auth_info at line 331 in file "xcb_auth.c"
  331       ret = compute_auth(info, authptr, sockname);
(dbx) print sockname
sockname = 0x806e878

So at 325 we overwrote sockname, leaking the old pointer.

I don't quite understand the code here, so I'm not sure if the correct fix
is setting gotsockname = 1 in a new else clause for the if clause at 302 or
simply free(sockname) before overwriting it at 325.

This isn't a big leak - just 124 bytes when opening a connection to the server,
which most programs only do once.
Comment 1 Martin Capitanio 2010-08-15 01:50:05 UTC 
It was introduced in
Get rid of PATH_MAX and MAXPATHLEN
a546d000 (Arnaud Fontaine 2010-04-30 14:47:16 +0200 259)     struct sockaddr *sockname = malloc(socknamelen), *new_sock

==27313== 124 bytes in 1 blocks are definitely lost in loss record 1 of 1
==27313==    at 0x4C284A8: malloc (vg_replace_malloc.c:236)
==27313==    by 0x405659: get_peer_sock_name (xcb_auth.c:259)
==27313==    by 0x405723: _xcb_get_auth_info (xcb_auth.c:302)
==27313==    by 0x4050FD: xcb_connect_to_display_with_auth_info (xcb_util.c:424)
==27313==    by 0x40185B: main (xcb-screent.c:43)
==27313== 
==27313== LEAK SUMMARY:
==27313==    definitely lost: 124 bytes in 1 blocks
Comment 2 jp.sittingduck 2010-09-23 04:53:25 UTC 
Created attachment 38898 [details] [review]
Fix memory leak in _xcb_get_auth_info.
Comment 3 jp.sittingduck 2010-09-23 04:54:46 UTC 
This is a problem in _xcb_get_auth_info: if the first get_peer_sock_name succeeds (returns non-NULL), gotsockname is not set to 1, so sockname is re-assigned later from another call to get_peer_sock_name.

Patch attached.
Comment 5 Alan Coopersmith 2012-01-30 10:06:34 UTC 
*** Bug 45390 has been marked as a duplicate of this bug. ***

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.