[lttng-dev] [lttng-ust RFC PATCH v2 1/1] Add -ust to the name of UST threads of the application

[Mathieu Desnoyers]

----- 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.

Thanks,

Mathieu

> 
> 
> 
>> +
>>  	/* Restart trying to connect to the session daemon */
>>  restart:
>>  	if (prev_connect_failed) {

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com