[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 16:31:55 UTC 2016
----- On Jun 3, 2016, at 6:24 PM, Sebastien Boisvert sboisvert at gydle.com wrote:
> Hi Raphaël,
>
> I had another thought.
>
> If the process name is "my-super-process", I think it only makes sense to append
> the
> "-ust" suffix if there is sufficient space. "my-super-process-ust" is fine, but
> I feel
> that "my-super-pro-ust" is meaningless.
I see having the -ust overwriting the end of the process name as a feature
that would be useful for users to distinguish between ust threads and non-ust
threads in cases where the binary name is long.
Thanks,
Mathieu
>
>
> On 06/03/2016 12:12 PM, Raphaël Beamonte wrote:
>> 2016-06-03 11:55 GMT-04:00 Mathieu Desnoyers <mathieu.desnoyers at efficios.com>:
>>> ----- 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.
>>
>> You're right, will fix that.
>> I'm also questionning the interest of limit being a variable here.
>> Should I replace both occurrences directly by "LTTNG_UST_PROCNAME_LEN
>> - 5" or create a "#define LTTNG_UST_PROCNAME_LIMIT
>> (LTTNG_UST_PROCNAME_LEN - 5)" macro ? Mathieu, what do you think would
>> be the best to do here ?
>> The condition will also be replaced from ">=" to ">", as the "=" part
>> is not useful anymore in that version of the patch (it was in v1,
>> because of the operations that were done).
>>
>>>>>
>>>>>
>>>>>> +
>>>>>> + 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.
>>
>> I made it a macro as I was not sure about the best path to take, and
>> the macro seemed to be the cleaner approach!
>> However, would it be better like that:
>>
>> #ifdef PTHREAD_SETNAME_NP
>> #include <pthread.h>
>> #endif
>>
>> static inline
>> void lttng_ust_setustprocname()
>> {
>> #ifdef PTHREAD_SETNAME_NP
>> ...
>>
>> PTHREAD_SETNAME_NP(pthread_self(), name);
>> #endif
>> }
>>
>> Or like that ?
>>
>> #ifdef PTHREAD_SETNAME_NP
>> #include <pthread.h>
>>
>> static inline
>> void lttng_ust_setustprocname()
>> {
>> ...
>>
>> PTHREAD_SETNAME_NP(pthread_self(), name);
>> }
>> #else
>> static inline
>> void lttng_ust_setustprocname()
>> {}
>> #endif
>>
>>
>> In both those cases, the macro disappears, and is replaced by a call
>> to lttng_ust_setustprocname().
>> I'm not sure however which is preferable according to the coding style
>> (In one case, two #ifdef are required, in the other, we declare the
>> function in each branch of the ifdef/else).
>>
>> Thoughts ? I'll send a v3 of the patch according to your preference in
>> both matters.
>>
>> Thanks a lot!
>> Raphaël
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
More information about the lttng-dev
mailing list