Bug 658571

Created attachment 463796 [details]
simpler reproduction program

(In reply to comment #2)
> Created attachment 463796 [details]
> simpler reproduction program

$ gcc -o foo -Wall foo.c -pthread -lselinux
$ valgrind --quiet --leak-check=full ./foo 2
==5658== 54 bytes in 2 blocks are definitely lost in loss record 1 of 2
==5658==    at 0x4A05E46: malloc (vg_replace_malloc.c:195)
==5658==    by 0x37E3082A91: strdup (strdup.c:43)
==5658==    by 0x3864E111D3: selinux_raw_to_trans_context (setrans_client.c:314)
==5658==    by 0x3864E102B9: getfilecon (getfilecon.c:64)
==5658==    by 0x400780: leaker (foo.c:10)
==5658==    by 0x37E3806D5A: start_thread (pthread_create.c:301)
==5658==    by 0x37E30E4AAC: clone (clone.S:115)
==5658== 
==5658== 54 bytes in 2 blocks are definitely lost in loss record 2 of 2
==5658==    at 0x4A05E46: malloc (vg_replace_malloc.c:195)
==5658==    by 0x37E3082A91: strdup (strdup.c:43)
==5658==    by 0x3864E111FA: selinux_raw_to_trans_context (setrans_client.c:317)
==5658==    by 0x3864E102B9: getfilecon (getfilecon.c:64)
==5658==    by 0x400780: leaker (foo.c:10)
==5658==    by 0x37E3806D5A: start_thread (pthread_create.c:301)
==5658==    by 0x37E30E4AAC: clone (clone.S:115)
==5658== 

A non-leaky solution should be silent with 0 exit status.

I think the best solution to this is to move to using selabel_open directly rather then matchpathcon within libvirt.

Changing libvirt to use selabel_open would only fix libvirt's leak, but not any other clients.  I still think that libselinux should be fixed to be thread-friendly.  The memory leak is in libselinux, whether or not libvirt is fixed to avoid libselinux.

Reassigning this bug back to libselinux, and cloning a new bug for libvirt to consider using selabel_open instead of matchpathcon

Looking at selinux.git, for at least libselinux/src/setrans_client.c, I can see that this has been contentious in the past:

commit a842c9dae863 (Jun 2009) converted __thread to pthread_key_t handling to resolve some crashes, and as a side effect, became multi-thread safe

commit 1ac1ff638250 (Jul 2009) reverted that patch, and instead avoided the crash by:

commit 8c372f665db4 (Jul 2009) instead ensuring that the __thread variables were properly initialized, but ended up reintroducing the leak for multi-thread processes

I think the pthread_key_t patch was reverted because we don't want libselinux to link to libpthread unconditionally.

The solution will need to use weak symbols for pthread_key_create, pthread_getspecific, and pthread_set_specific, with macros for the non-pthread case.  This is already done with pthread_once (see selinux_internal.h)

Will try to put something together tomorrow.

Make(In reply to comment #8)
> I think the pthread_key_t patch was reverted because we don't want libselinux
> to link to libpthread unconditionally.

Makes sense to me.

> 
> The solution will need to use weak symbols for pthread_key_create,
> pthread_getspecific, and pthread_set_specific, with macros for the non-pthread
> case.  This is already done with pthread_once (see selinux_internal.h)

I wasn't too familiar with the libselinux code base, so thanks for stepping in and taking over.

> 
> Will try to put something together tomorrow.

I'm happy to review whatever you come up with.

Also, I thought of something else you can use to minimize code churn.  It should be a much smaller patch to just introduce one additional variable (per file that uses __thread), guarantee that it gets set to a non-0 thread-local value any time any of the other __thread variables are touched, and that all the __thread variables get cleaned as a side effect of the destructor for the one pthread_key_t variable (that is, you only need a single destructor for a new variable, rather than one destructor for each instance of a __thread variable, and the bulk of your code can benefit from the ease of use of __thread rather than the verbosity of pthread_{get,set}specific).

Created attachment 464338 [details]
Proposed fix

Please test patch.

(In reply to comment #11)
> Please test patch.

Tested.  It solves the leak on getfilecon when leaked with the simple program, and looks like it does the right thing for matchpathcon as well.  Thanks!

Kind of a scary package to update,  I think we should run it for a couple of days internally and in rawhide before we ship.

Fix is upstream along with 2 other patches related to thread-local storage.  libselinux 2.0.97.

Fixed in libselinux-2.0.94-3.el6

Eamon this is causing other bugs.

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

Created attachment 490044 [details]
Miroslav lets add this patch from the other bug.

Created attachment 492476 [details]
test case demonstrationg the destruction of innocent pthread_key.

(In reply to comment #19)
> Eamon this is causing other bugs.

I am also running into a side effect of this change on RHEL 6.1 beta.  I can't access the other bug, so can't verify if it is the same issue.  My problem happens when I dlopen() and then later dlclose() libselinux.  After dlclose(), the TSD key I created using pthread_key_create() is no longer accessible.  I attached gdb, and discovered that libselinux is destroying it in its destructor.  This is because libselinux's destructor_key is a static variable that can sometimes be left uninitialized.  Yet, the library destructor unconditionally calls pthread_key_delete() on this uninitialized value (zero, since it's static).  I will attach a test program that demonstrates this.

comment 25 has a patch that appears to remove all phtread_key-related functionality.  It should certainly fix my problem.  Are you planning to fix this in 6.1 beta, for GA or at some later point?

A patch from Dan has gone upstream to fix this.

    Technical note added. If any revisions are required, please edit the "Technical Notes" field
    accordingly. All revisions will be proofread by the Engineering Content Services team.
    
    New Contents:
libselinux used __thread variables to store malloc() data in order to minimize computation. Destructors cannot be associated with __thread variables, so malloc() data stored in a __thread void* variable could potentially cause memory leaks upon thread exit. For example, repeatedly starting and stopping domains with libvirt could trigger out-of-memory exceptions, since libvirt starts one thread per domain, and each thread uses libselinux calls such as fgetfilecon. libselinux has been updated to be thread-safe, preventing these potential memory leaks.

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2011-0751.html