diff mbox

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

Message ID 80ee3399800d43b5d3f51aa095ebca160d4f20b9.1464966729.git.raphael.beamonte@gmail.com
State Superseded, archived
Headers show

Commit Message

Raphaël Beamonte June 3, 2016, 3:17 p.m. UTC
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(-)

Comments

Sebastien Boisvert June 3, 2016, 3:47 p.m. UTC | #1
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()



> +
>  	/* Restart trying to connect to the session daemon */
>  restart:
>  	if (prev_connect_failed) {
>
Mathieu Desnoyers June 3, 2016, 3:54 p.m. UTC | #2
----- 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 June 3, 2016, 3:55 p.m. UTC | #3
----- 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
Raphaël Beamonte June 3, 2016, 4:12 p.m. UTC | #4
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 June 3, 2016, 4:19 p.m. UTC | #5
----- On Jun 3, 2016, at 6:12 PM, Raphaël Beamonte raphael.beamonte at gmail.com 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 ?

Hardcoded constants are always a bad idea.

#define MYSTRING "blah"

LTTNG_UST_PROCNAME_LEN - strlen(MYSTRING)

would be ok though.

> 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()
> {}

Change for

static inline
void lttng_ust_setustprocname(void)
{
}

> #endif
>

Don't forget that (void) in C differs from (). () is like
a variadic argument function.

The latter form is prefered by our coding style. No #ifdef within
functions unless it's really impossible to do otherwise.

Thanks,

Mathieu

 
> 
> 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
Sebastien Boisvert June 3, 2016, 4:24 p.m. UTC | #6
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
>
Mathieu Desnoyers June 3, 2016, 4:31 p.m. UTC | #7
----- 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
diff mbox

Patch

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];
+	int limit = LTTNG_UST_PROCNAME_LEN - 4;
+	int len = 0;
+
+	lttng_ust_getprocname(name);
+
+	len = strlen(name);
+	if (len >= limit) {
+		len = limit;
+	}
+
+	sprintf(name + len, "%s", "-ust");
+
+	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;
+
 	/* Restart trying to connect to the session daemon */
 restart:
 	if (prev_connect_failed) {