[libvirt] [PATCH 5/n] security: plug memory leak

[Eric Blake]

On 11/24/2010 02:57 PM, Eric Blake wrote:
> security_context_t happens to be a typedef for char*, and happens to
> begin with a string usable as a raw context string.  But in reality,
> it is an opaque type that may or may not have additional information
> after the first NUL byte, where that additional information can
> include pointers that can only be freed via freecon().
> 
> Proof is from this valgrind run of daemon/libvirtd:
> 
> ==6028== 839,169 (40 direct, 839,129 indirect) bytes in 1 blocks are definitely lost in loss record 274 of 274
> ==6028==    at 0x4A0515D: malloc (vg_replace_malloc.c:195)
> ==6028==    by 0x3022E0D48C: selabel_open (label.c:165)
> ==6028==    by 0x3022E11646: matchpathcon_init_prefix (matchpathcon.c:296)
> ==6028==    by 0x3022E1190D: matchpathcon (matchpathcon.c:317)
> ==6028==    by 0x4F9D842: SELinuxRestoreSecurityFileLabel (security_selinux.c:382)
> 
> 800k is a lot of memory to be leaking.
> 
> * src/security/security_selinux.c
> (SELinuxReserveSecurityLabel, SELinuxGetSecurityProcessLabel)
> (SELinuxRestoreSecurityFileLabel): Use correct function to free
> security_context_t.
> ---
>  src/security/security_selinux.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)

Actually, it turns out that even with this patch, libvirtd is still
suffering from the same severe 800k leak.  libselinux has a bug where it
uses __thread variables to store malloc'd memory, which is a no-no
(since __thread has no destructor, you can't ever release the memory on
pthread_exit(), in contrast to the heavier-weight
pthread_key_create/pthread_getspecific interface).

https://bugzilla.redhat.com/show_bug.cgi?id=658571

I'm wondering if adding a matchpathcon_fini() call would help reduce the
leak [to pair up with the implicit matchpathcon_init(NULL) during our
first use of matchpathcon()], although it won't eliminate it.

Also, I peeked at the libselinux source; for now, security_context_t is
just a NUL-terminated string grabbed from fgetxattr(XATTR_NAME_SELINUX),
and freecon() is a thin wrapper around free(); but there's no guarantee
that this won't change in a future version of the libselinux library.
Therefore, using VIR_FREE() interchangeably with freecon() is not a bug
today, but is not future-proof, so I'm still happy this patch went in.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101130/ee771dfc/attachment-0001.sig>