[lttng-dev] [lttng-ust RFC PATCH v2 1/1] Add -ust to the name of UST threads of the application
Mathieu Desnoyers
mathieu.desnoyers at efficios.com
Fri Jun 3 15:55:27 UTC 2016
----- On Jun 3, 2016, at 5:54 PM, Mathieu Desnoyers mathieu.desnoyers at efficios.com wrote:
> ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert sboisvert at gydle.com wrote:
>
>> Hi Raphaël,
>>
>> I think you have a buffer overflow, see below.
>>
>> On 06/03/2016 11:17 AM, Raphaël Beamonte wrote:
>>> Add the required functions to change the thread name of the UST
>>> threads and add the -ust string at its end. This will help to
>>> identify LTTng-UST processes when analyzing the trace of a process.
>>>
>>> Signed-off-by: Raphaël Beamonte <raphael.beamonte at gmail.com>
>>> ---
>>> configure.ac | 5 +++++
>>> liblttng-ust/Makefile.am | 1 +
>>> liblttng-ust/compat.h | 48 ++++++++++++++++++++++++++++++++++++++++++-
>>> liblttng-ust/lttng-ust-comm.c | 6 ++++++
>>> 4 files changed, 59 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/configure.ac b/configure.ac
>>> index 5692553..de462ff 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -135,6 +135,11 @@ AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBDL], [test
>>> "x$have_libdl" = "xyes"])
>>> AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBC_DL], [test "x$have_libc_dl" = "xyes"])
>>>
>>> AC_CHECK_LIB([pthread], [pthread_create])
>>> +AC_CHECK_LIB([pthread], [pthread_setname_np],
>>> + AC_DEFINE([HAVE_PTHREAD_SETNAME_NP], [1], [Define to 1 if pthread_setname_np
>>> is available.]),
>>> + AC_CHECK_LIB([pthread], [pthread_set_name_np],
>>> + AC_DEFINE([HAVE_PTHREAD_SET_NAME_NP], [1], [Define to 1 if
>>> pthread_set_name_np is available.]),
>>> + AC_MSG_RESULT([pthread setname/set_name not found.])))
>>>
>>> # Check for dlfcn.h
>>> AC_CHECK_HEADER([dlfcn.h])
>>> diff --git a/liblttng-ust/Makefile.am b/liblttng-ust/Makefile.am
>>> index f1801cf..05929be 100644
>>> --- a/liblttng-ust/Makefile.am
>>> +++ b/liblttng-ust/Makefile.am
>>> @@ -14,6 +14,7 @@ liblttng_ust_tracepoint_la_SOURCES = \
>>> error.h
>>> liblttng_ust_tracepoint_la_LIBADD = \
>>> -lurcu-bp \
>>> + -lpthread \
>>> $(top_builddir)/snprintf/libustsnprintf.la
>>> liblttng_ust_tracepoint_la_LDFLAGS = -no-undefined -version-info
>>> $(LTTNG_UST_LIBRARY_VERSION)
>>> liblttng_ust_tracepoint_la_CFLAGS = -DUST_COMPONENT="liblttng_ust_tracepoint"
>>> -fno-strict-aliasing
>>> diff --git a/liblttng-ust/compat.h b/liblttng-ust/compat.h
>>> index 43b2223..4fee919 100644
>>> --- a/liblttng-ust/compat.h
>>> +++ b/liblttng-ust/compat.h
>>> @@ -3,6 +3,7 @@
>>>
>>> /*
>>> * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
>>> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte at gmail.com>
>>> *
>>> * This library is free software; you can redistribute it and/or
>>> * modify it under the terms of the GNU Lesser General Public
>>> @@ -23,7 +24,6 @@
>>> * lttng_ust_getprocname.
>>> */
>>> #ifdef __linux__
>>> -
>>> #include <sys/prctl.h>
>>>
>>> #define LTTNG_UST_PROCNAME_LEN 17
>>> @@ -34,6 +34,13 @@ void lttng_ust_getprocname(char *name)
>>> (void) prctl(PR_GET_NAME, (unsigned long) name, 0, 0, 0);
>>> }
>>>
>>> +/*
>>> + * if pthread_setname_np is available.
>>> + */
>>> +#ifdef HAVE_PTHREAD_SETNAME_NP
>>> +#define PTHREAD_SETNAME_NP pthread_setname_np
>>> +#endif
>>> +
>>> #elif defined(__FreeBSD__)
>>> #include <stdlib.h>
>>> #include <string.h>
>>> @@ -59,6 +66,45 @@ void lttng_ust_getprocname(char *name)
>>> strncpy(name, bsd_name, LTTNG_UST_PROCNAME_LEN - 1);
>>> }
>>>
>>> +/*
>>> + * if pthread_set_name_np is available.
>>> + */
>>> +#ifdef HAVE_PTHREAD_SET_NAME_NP
>>> +#define PTHREAD_SETNAME_NP pthread_set_name_np
>>> +#endif
>>> +
>>> +#endif
>>> +
>>> +/*
>>> + * if a pthread setname/set_name function is available, declare
>>> + * the setustprocname() function that will add '-ust' to the end
>>> + * of the current process name, while truncating it if needed.
>>> + */
>>> +#ifdef PTHREAD_SETNAME_NP
>>> +#define LTTNG_UST_SETUSTPROCNAME lttng_ust_setustprocname()
>>> +
>>> +#include <pthread.h>
>>> +
>>> +static inline
>>> +void lttng_ust_setustprocname()
>>> +{
>>> + char name[LTTNG_UST_PROCNAME_LEN];
>>
>> Let's say that LTTNG_UST_PROCNAME_LEN is 10.
>>
>>> + int limit = LTTNG_UST_PROCNAME_LEN - 4;
>>
>> Then, limit is 10 - 4 = 6.
>>
>>> + int len = 0;
>>
>> This can be moved on the strlen(name) line.
>>
>>> +
>>> + lttng_ust_getprocname(name);
>>
>> Let's suppose that after the call to lttng_ust_getprocname(), name contains
>> "012345678\0",
>>
>>> +
>>> + len = strlen(name);
>>
>> The variable "len" will contain the integer 9.
>>
>>> + if (len >= limit) {
>>> + len = limit;
>>> + }
>>> +
>>
>> After that, len is >= limit (9 >= 6), so len becomes 6.
>>
>>> + sprintf(name + len, "%s", "-ust");
>>
>> name is a pointer to "012345678\0" (9 bytes + '\0')
>>
>> name + len is name + 6, so it points to "678\0"
>>
>> sprintf will essentially copy "-ust\0" to "678\0".
>>
>> So, if my calculations are not wrong, you have a buffer overflow which will
>> corrupt the stack memory of the
>> current stack frame.
>>
>>
>>> +
>>> + PTHREAD_SETNAME_NP(pthread_self(), name);
>>> +}
>>> +#else
>>> +#define LTTNG_UST_SETUSTPROCNAME
>>> #endif
>>>
>>> #include <errno.h>
>>> diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c
>>> index e00a22c..4de68c7 100644
>>> --- a/liblttng-ust/lttng-ust-comm.c
>>> +++ b/liblttng-ust/lttng-ust-comm.c
>>> @@ -1295,6 +1295,12 @@ void *ust_listener_thread(void *arg)
>>> int sock, ret, prev_connect_failed = 0, has_waited = 0;
>>> long timeout;
>>>
>>> + /*
>>> + * If available, add '-ust' to the end of this thread's
>>> + * process name
>>> + */
>>> + LTTNG_UST_SETUSTPROCNAME;
>>
>> Is this common practice ? Just by looking at this line, it is unclear that it is
>> a function call.
>>
>> I would prefer
>>
>> LTTNG_UST_SETUSTPROCNAME()
>>
>> or
>>
>> lttng_ust_setustprocname()
>
> The latter is preferred by our coding style, indeed.
>
And there is no reason to make it a macro. It should be
a static inline whenever possible.
Thanks,
Mathieu
> Thanks,
>
> Mathieu
>
>>
>>
>>
>>> +
>>> /* Restart trying to connect to the session daemon */
>>> restart:
>>> if (prev_connect_failed) {
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
More information about the lttng-dev
mailing list