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
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.
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. >
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.htmlI 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. >> >
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. > >> > > >
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.