All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: Setenv/getenv are not thread-safe; whose bug is it?
       [not found] <HE1PR07MB095321C916B1298B997744F6F0200@HE1PR07MB0953.eurprd07.prod.outlook.com>
@ 2017-03-10 23:32 ` Mathieu Desnoyers
       [not found] ` <1656612379.3244.1489188755549.JavaMail.zimbra@efficios.com>
  1 sibling, 0 replies; 4+ messages in thread
From: Mathieu Desnoyers @ 2017-03-10 23:32 UTC (permalink / raw)
  To: Douglas Graham; +Cc: lttng-dev

----- On Mar 10, 2017, at 5:21 PM, Douglas Graham douglas.graham@ericsson.com wrote:

> Hi,
> 
> We have an application that uses lttng-ust for logging.  We are seeing a crash
> in getenv here:
> 
> #0  __GI_getenv (name=0xb6eb06de "TNG_UST_WITHOUT_BADDR_STATEDUMP") at
> getenv.c:85
> #1  0xb6e7b350 in do_baddr_statedump (owner=0xb6ecf300 <global_apps>) at
> lttng-ust-statedump.c:315
> #2  do_lttng_ust_statedump (owner=owner@entry=0xb6ecf300 <global_apps>)  at
> lttng-ust-statedump.c:341
> #3  0xb6e71ef4 in lttng_handle_pending_statedump (owner=owner@entry=0xb6ecf300
> <global_apps>)  at lttng-events.c:856
> #4  0xb6e690ac in handle_pending_statedump (sock_info=0xb6ecf300 <global_apps>)
> at lttng-ust-comm.c:581
> #5  handle_message (lum=0xb48fe66c, sock=<optimized out>, sock_info=<optimized
> out>)  at lttng-ust-comm.c:966
> #6  ust_listener_thread (arg=0xb6ecf300 <global_apps>)  at lttng-ust-comm.c:1490
> #7  0xb6e33f6c in start_thread (arg=0xb48ff220) at pthread_create.c:339
> 
> The core shows that this thread is one of three threads in the child process
> just after a fork().  After the fork(), the one application thread in the child
> calls setenv() to set up the environment, and then execs another program.  The
> problem is that setenv() is not thread-safe, especially if it requires the
> environment vector to be resized.  If the application thread calls setenv() to
> add a new environment  variable at the same time that getenv is called by this
> lttng listener thread, bad things can happen. The setenv can cause the
> environment vector to be resized at the same time it is being searched, which
> causes getenv go off into the weeds.
> 
> I assume that this listener thread is created because we have preloaded
> libttng-ust-fork, and I see no reason that this particular process really needs
> to preload that library, so one workaround is probably to just remove it.  The
> problem is that this process inherits LD_PRELOAD from a parent process (one
> similar to init) that launches many other daemons, some that might actually
> require liblttng-ust-fork, so removing this library from the process that is
> crashing is not entirely trivial.  And this crash also raises the question of
> whether we could encounter similar crashes in other processes that use
> liblttng-ust.  It's only after intensive testing for many hours that we see
> this crash.
> 
> Would it be safe to say that it is probably a bug for an lttng thread to make a
> call to a non thread-safe function like getenv()?  What's the best way to fix
> this?

Hi Douglas,

Thanks a lot for the very thorough but report!

I just prepared a patch for you sent in a separate thread:

"[PATCH lttng-ust] Fix: race between lttng-ust getenv() and application setenv()"

Can you give it a try and let me know if it fixes things on your side ?

Thanks,

Mathieu

> 
> Thanks,
> Doug
> 
> 
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Setenv/getenv are not thread-safe; whose bug is it?
       [not found] ` <1656612379.3244.1489188755549.JavaMail.zimbra@efficios.com>
@ 2017-03-13  0:21   ` Douglas Graham
       [not found]   ` <HE1PR07MB09535F02D666D99BB7221713F0250@HE1PR07MB0953.eurprd07.prod.outlook.com>
  1 sibling, 0 replies; 4+ messages in thread
From: Douglas Graham @ 2017-03-13  0:21 UTC (permalink / raw)
  To: Mathieu Desnoyers; +Cc: lttng-dev

> Thanks a lot for the very thorough but report!
> I just prepared a patch for you sent in a separate thread:
> "[PATCH lttng-ust] Fix: race between lttng-ust getenv() and application setenv()"
> Can you give it a try and let me know if it fixes things on your side ?

Thanks for the quick turnaround Mathieu!  

I don't know how we build lttng but I have passed this information along to the team that does, asking that they provide me a library with this patch applied for us to test.  I'll let you know how it looks when we get there.

Regards,
Doug
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Setenv/getenv are not thread-safe; whose bug is it?
       [not found]   ` <HE1PR07MB09535F02D666D99BB7221713F0250@HE1PR07MB0953.eurprd07.prod.outlook.com>
@ 2017-03-13  1:12     ` Mathieu Desnoyers
  0 siblings, 0 replies; 4+ messages in thread
From: Mathieu Desnoyers @ 2017-03-13  1:12 UTC (permalink / raw)
  To: Douglas Graham; +Cc: lttng-dev

----- On Mar 12, 2017, at 8:21 PM, Douglas Graham douglas.graham@ericsson.com wrote:

>> Thanks a lot for the very thorough but report!
>> I just prepared a patch for you sent in a separate thread:
>> "[PATCH lttng-ust] Fix: race between lttng-ust getenv() and application
>> setenv()"
>> Can you give it a try and let me know if it fixes things on your side ?
> 
> Thanks for the quick turnaround Mathieu!
> 
> I don't know how we build lttng but I have passed this information along to the
> team that does, asking that they provide me a library with this patch applied
> for us to test.  I'll let you know how it looks when we get there.

FYI, this patch is now in lttng-ust master branch, and the following
patch fixes a warning introduced by my initial patch:

commit 92ce256da4da1586465e18362e88b5be16752d59
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date:   Sat Mar 11 21:18:27 2017 -0500

    Fix: add missing getenv.h include to ustctl.c
    
    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thanks,

Mathieu


> 
> Regards,
> Doug

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Setenv/getenv are not thread-safe; whose bug is it?
@ 2017-03-10 22:21 Douglas Graham
  0 siblings, 0 replies; 4+ messages in thread
From: Douglas Graham @ 2017-03-10 22:21 UTC (permalink / raw)
  To: lttng-dev

Hi,

We have an application that uses lttng-ust for logging.  We are seeing a crash in getenv here:

#0  __GI_getenv (name=0xb6eb06de "TNG_UST_WITHOUT_BADDR_STATEDUMP") at getenv.c:85
#1  0xb6e7b350 in do_baddr_statedump (owner=0xb6ecf300 <global_apps>) at lttng-ust-statedump.c:315
#2  do_lttng_ust_statedump (owner=owner@entry=0xb6ecf300 <global_apps>)  at lttng-ust-statedump.c:341
#3  0xb6e71ef4 in lttng_handle_pending_statedump (owner=owner@entry=0xb6ecf300 <global_apps>)  at lttng-events.c:856
#4  0xb6e690ac in handle_pending_statedump (sock_info=0xb6ecf300 <global_apps>) at lttng-ust-comm.c:581
#5  handle_message (lum=0xb48fe66c, sock=<optimized out>, sock_info=<optimized out>)  at lttng-ust-comm.c:966
#6  ust_listener_thread (arg=0xb6ecf300 <global_apps>)  at lttng-ust-comm.c:1490
#7  0xb6e33f6c in start_thread (arg=0xb48ff220) at pthread_create.c:339

The core shows that this thread is one of three threads in the child process just after a fork().  After the fork(), the one application thread in the child calls setenv() to set up the environment, and then execs another program.  The problem is that setenv() is not thread-safe, especially if it requires the environment vector to be resized.  If the application thread calls setenv() to add a new environment  variable at the same time that getenv is called by this lttng listener thread, bad things can happen. The setenv can cause the environment vector to be resized at the same time it is being searched, which causes getenv go off into the weeds.

I assume that this listener thread is created because we have preloaded libttng-ust-fork, and I see no reason that this particular process really needs to preload that library, so one workaround is probably to just remove it.  The problem is that this process inherits LD_PRELOAD from a parent process (one similar to init) that launches many other daemons, some that might actually require liblttng-ust-fork, so removing this library from the process that is crashing is not entirely trivial.  And this crash also raises the question of whether we could encounter similar crashes in other processes that use liblttng-ust.  It's only after intensive testing for many hours that we see this crash.

Would it be safe to say that it is probably a bug for an lttng thread to make a call to a non thread-safe function like getenv()?  What's the best way to fix this?

Thanks,
Doug



_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2017-03-13  1:17 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <HE1PR07MB095321C916B1298B997744F6F0200@HE1PR07MB0953.eurprd07.prod.outlook.com>
2017-03-10 23:32 ` Setenv/getenv are not thread-safe; whose bug is it? Mathieu Desnoyers
     [not found] ` <1656612379.3244.1489188755549.JavaMail.zimbra@efficios.com>
2017-03-13  0:21   ` Douglas Graham
     [not found]   ` <HE1PR07MB09535F02D666D99BB7221713F0250@HE1PR07MB0953.eurprd07.prod.outlook.com>
2017-03-13  1:12     ` Mathieu Desnoyers
2017-03-10 22:21 Douglas Graham

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.