* Libxenstore memory leak on static compile
@ 2012-09-12 13:44 Andres Lagar-Cavilla
2012-09-12 14:56 ` Ian Campbell
0 siblings, 1 reply; 6+ messages in thread
From: Andres Lagar-Cavilla @ 2012-09-12 13:44 UTC (permalink / raw)
To: xen-devel, Ian Campbell, Ian Jackson
When statically compiling libxenstore.a, the USE_PTHREAD define is not applied. This results in cleanup_{pus/pop} being no-ops. cleanup_p* are used throughout xs.c:read_message to free malloc'ed objects.
In short, libxenstore.a will leak memory when reading xenstore messages. OOM killer awaits.
This could be solved by either turning on USE_PTHREAD for .a compilation (which, N.B. will not actually link libpthread but instead produce an object archive that needs to be eventually linked to libpthread.so), or by replacing cleanup_p* by proper free calls.
Thoughts?
Andres
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Libxenstore memory leak on static compile
2012-09-12 13:44 Libxenstore memory leak on static compile Andres Lagar-Cavilla
@ 2012-09-12 14:56 ` Ian Campbell
2012-09-12 15:03 ` Roger Pau Monne
0 siblings, 1 reply; 6+ messages in thread
From: Ian Campbell @ 2012-09-12 14:56 UTC (permalink / raw)
To: Andres Lagar-Cavilla; +Cc: Roger Pau Monne, Ian Jackson, xen-devel
On Wed, 2012-09-12 at 14:44 +0100, Andres Lagar-Cavilla wrote:
> When statically compiling libxenstore.a, the USE_PTHREAD define is not
> applied. This results in cleanup_{pus/pop} being no-ops. cleanup_p*
> are used throughout xs.c:read_message to free malloc'ed objects.
>
> In short, libxenstore.a will leak memory when reading xenstore
> messages. OOM killer awaits.
>
> This could be solved by either turning on USE_PTHREAD for .a
> compilation (which, N.B. will not actually link libpthread but instead
> produce an object archive that needs to be eventually linked to
> libpthread.so), or by replacing cleanup_p* by proper free calls.
The reason for the non-pthreads static library was so that you could
build tiny statically linked xenstore clients against tiny libcs (like
uclibc) to have things small enough to fit in e.g. your installer initrd
or in your "guest tools" package.
It used to be that uclibc didn't have a pthreads library. Maybe this
has changed though (Roger, CCd, would know).
We don't seem to use pthread_cleanup_push/pop very extensively, so I
think using proper free calls is probably the way to go?
Using that pthread facility as a cheap and nasty GC seems a bit wrong to
me anyhow.
Ian.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Libxenstore memory leak on static compile
2012-09-12 14:56 ` Ian Campbell
@ 2012-09-12 15:03 ` Roger Pau Monne
2012-09-12 15:09 ` Andres Lagar-Cavilla
0 siblings, 1 reply; 6+ messages in thread
From: Roger Pau Monne @ 2012-09-12 15:03 UTC (permalink / raw)
To: Ian Campbell; +Cc: Andres Lagar-Cavilla, Ian Jackson, xen-devel
Ian Campbell wrote:
> On Wed, 2012-09-12 at 14:44 +0100, Andres Lagar-Cavilla wrote:
>> When statically compiling libxenstore.a, the USE_PTHREAD define is not
>> applied. This results in cleanup_{pus/pop} being no-ops. cleanup_p*
>> are used throughout xs.c:read_message to free malloc'ed objects.
>>
>> In short, libxenstore.a will leak memory when reading xenstore
>> messages. OOM killer awaits.
>>
>> This could be solved by either turning on USE_PTHREAD for .a
>> compilation (which, N.B. will not actually link libpthread but instead
>> produce an object archive that needs to be eventually linked to
>> libpthread.so), or by replacing cleanup_p* by proper free calls.
>
> The reason for the non-pthreads static library was so that you could
> build tiny statically linked xenstore clients against tiny libcs (like
> uclibc) to have things small enough to fit in e.g. your installer initrd
> or in your "guest tools" package.
>
> It used to be that uclibc didn't have a pthreads library. Maybe this
> has changed though (Roger, CCd, would know).
Yes, uclibc added a pthread library back in 2002:
http://mailman.uclinux.org/pipermail/uclinux-dev/2002-March/007613.html
> We don't seem to use pthread_cleanup_push/pop very extensively, so I
> think using proper free calls is probably the way to go?
>
> Using that pthread facility as a cheap and nasty GC seems a bit wrong to
> me anyhow.
>
> Ian.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Libxenstore memory leak on static compile
2012-09-12 15:03 ` Roger Pau Monne
@ 2012-09-12 15:09 ` Andres Lagar-Cavilla
2012-09-12 15:56 ` Ian Campbell
0 siblings, 1 reply; 6+ messages in thread
From: Andres Lagar-Cavilla @ 2012-09-12 15:09 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Andres Lagar-Cavilla, Ian Jackson, Ian Campbell, xen-devel
On Sep 12, 2012, at 11:03 AM, Roger Pau Monne wrote:
> Ian Campbell wrote:
>> On Wed, 2012-09-12 at 14:44 +0100, Andres Lagar-Cavilla wrote:
>>> When statically compiling libxenstore.a, the USE_PTHREAD define is not
>>> applied. This results in cleanup_{pus/pop} being no-ops. cleanup_p*
>>> are used throughout xs.c:read_message to free malloc'ed objects.
>>>
>>> In short, libxenstore.a will leak memory when reading xenstore
>>> messages. OOM killer awaits.
>>>
>>> This could be solved by either turning on USE_PTHREAD for .a
>>> compilation (which, N.B. will not actually link libpthread but instead
>>> produce an object archive that needs to be eventually linked to
>>> libpthread.so), or by replacing cleanup_p* by proper free calls.
>>
>> The reason for the non-pthreads static library was so that you could
>> build tiny statically linked xenstore clients against tiny libcs (like
>> uclibc) to have things small enough to fit in e.g. your installer initrd
>> or in your "guest tools" package.
>>
>> It used to be that uclibc didn't have a pthreads library. Maybe this
>> has changed though (Roger, CCd, would know).
>
> Yes, uclibc added a pthread library back in 2002:
>
> http://mailman.uclinux.org/pipermail/uclinux-dev/2002-March/007613.html
I have a simple fix to compile with pthread.h (below). It's really your call as to whether you want to maintain pthread-free versions of libraries for the next embedded femto-libc (and whether that is consistently applied throughout the tree, etc). Getting rid of pthread_push/pop as alternative is not bad at all.
Andres
# HG changeset patch
# Parent b3a8ef4c556cc9a2d310cd8dd1e2f625c92c8515
Compile xs.o with pthread support.
Otherwise messages are not freed in the statically linked libxenstore.a.
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
diff -r b3a8ef4c556c tools/xenstore/Makefile
--- a/tools/xenstore/Makefile
+++ b/tools/xenstore/Makefile
@@ -81,6 +81,7 @@ libxenstore.so.$(MAJOR): libxenstore.so.
ln -sf $< $@
xs.opic: CFLAGS += -DUSE_PTHREAD
+xs.o: CFLAGS += -DUSE_PTHREAD
libxenstore.so.$(MAJOR).$(MINOR): xs.opic xs_lib.opic
$(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxenstore.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $^ $(SOCKET_LIBS) -lpthread $(APPEND_LDFLAGS)
>
>> We don't seem to use pthread_cleanup_push/pop very extensively, so I
>> think using proper free calls is probably the way to go?
>>
>> Using that pthread facility as a cheap and nasty GC seems a bit wrong to
>> me anyhow.
>>
>> Ian.
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Libxenstore memory leak on static compile
2012-09-12 15:09 ` Andres Lagar-Cavilla
@ 2012-09-12 15:56 ` Ian Campbell
2012-09-13 14:33 ` Ian Jackson
0 siblings, 1 reply; 6+ messages in thread
From: Ian Campbell @ 2012-09-12 15:56 UTC (permalink / raw)
To: Andres Lagar-Cavilla; +Cc: xen-devel, Ian Jackson, Roger Pau Monne
On Wed, 2012-09-12 at 16:09 +0100, Andres Lagar-Cavilla wrote:
> On Sep 12, 2012, at 11:03 AM, Roger Pau Monne wrote:
>
> > Ian Campbell wrote:
> >> On Wed, 2012-09-12 at 14:44 +0100, Andres Lagar-Cavilla wrote:
> >>> When statically compiling libxenstore.a, the USE_PTHREAD define is not
> >>> applied. This results in cleanup_{pus/pop} being no-ops. cleanup_p*
> >>> are used throughout xs.c:read_message to free malloc'ed objects.
> >>>
> >>> In short, libxenstore.a will leak memory when reading xenstore
> >>> messages. OOM killer awaits.
> >>>
> >>> This could be solved by either turning on USE_PTHREAD for .a
> >>> compilation (which, N.B. will not actually link libpthread but instead
> >>> produce an object archive that needs to be eventually linked to
> >>> libpthread.so), or by replacing cleanup_p* by proper free calls.
> >>
> >> The reason for the non-pthreads static library was so that you could
> >> build tiny statically linked xenstore clients against tiny libcs (like
> >> uclibc) to have things small enough to fit in e.g. your installer initrd
> >> or in your "guest tools" package.
> >>
> >> It used to be that uclibc didn't have a pthreads library. Maybe this
> >> has changed though (Roger, CCd, would know).
> >
> > Yes, uclibc added a pthread library back in 2002:
> >
> > http://mailman.uclinux.org/pipermail/uclinux-dev/2002-March/007613.html
> I have a simple fix to compile with pthread.h (below). It's really your call as to whether you want to maintain pthread-free versions of libraries for the next embedded femto-libc (and whether that is consistently applied throughout the tree, etc). Getting rid of pthread_push/pop as alternative is not bad at all.
>
> Andres
>
> # HG changeset patch
> # Parent b3a8ef4c556cc9a2d310cd8dd1e2f625c92c8515
> Compile xs.o with pthread support.
>
> Otherwise messages are not freed in the statically linked libxenstore.a.
If you do this then you may as well nuke the !USE_PTHREAD case
altogether, since this makes it pointless.
I'd prefer not to do that though.
>
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>
> diff -r b3a8ef4c556c tools/xenstore/Makefile
> --- a/tools/xenstore/Makefile
> +++ b/tools/xenstore/Makefile
> @@ -81,6 +81,7 @@ libxenstore.so.$(MAJOR): libxenstore.so.
> ln -sf $< $@
>
> xs.opic: CFLAGS += -DUSE_PTHREAD
> +xs.o: CFLAGS += -DUSE_PTHREAD
>
> libxenstore.so.$(MAJOR).$(MINOR): xs.opic xs_lib.opic
> $(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxenstore.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $^ $(SOCKET_LIBS) -lpthread $(APPEND_LDFLAGS)
>
> >
> >> We don't seem to use pthread_cleanup_push/pop very extensively, so I
> >> think using proper free calls is probably the way to go?
> >>
> >> Using that pthread facility as a cheap and nasty GC seems a bit wrong to
> >> me anyhow.
> >>
> >> Ian.
> >>
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Libxenstore memory leak on static compile
2012-09-12 15:56 ` Ian Campbell
@ 2012-09-13 14:33 ` Ian Jackson
0 siblings, 0 replies; 6+ messages in thread
From: Ian Jackson @ 2012-09-13 14:33 UTC (permalink / raw)
To: Ian Campbell; +Cc: Andres Lagar-Cavilla, Roger Pau Monne, xen-devel
Ian Campbell writes ("Re: [Xen-devel] Libxenstore memory leak on static compile"):
> If you do this then you may as well nuke the !USE_PTHREAD case
> altogether, since this makes it pointless.
>
> I'd prefer not to do that though.
I agree.
I think it would be better to just fix the leak. This should go into
unstable and after that into 4.2.1.
Also I think we should rename (in unstable only) the .a to make it
clear that it's not just a .a version of the .so.
Ian.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-13 14:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-12 13:44 Libxenstore memory leak on static compile Andres Lagar-Cavilla
2012-09-12 14:56 ` Ian Campbell
2012-09-12 15:03 ` Roger Pau Monne
2012-09-12 15:09 ` Andres Lagar-Cavilla
2012-09-12 15:56 ` Ian Campbell
2012-09-13 14:33 ` Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.