[lttng-dev] [lttng-ust RFC PATCH v2 1/1] Add -ust to the name of UST threads of the application
Raphaël Beamonte
raphael.beamonte at gmail.com
Fri Jun 3 16:12:52 UTC 2016
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