[lttng-dev] [lttng-ust RFC PATCH v2 1/1] Add -ust to the name of UST threads of the application
Sebastien Boisvert
sboisvert at gydle.com
Fri Jun 3 16:24:31 UTC 2016
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.
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
>
More information about the lttng-dev
mailing list