Re: [lttng-ust PATCH v3 1/1] Add -ust to the name of UST threads of the application

Re: [lttng-ust PATCH v3 1/1] Add -ust to the name of UST threads of the application

[Sebastien Boisvert]

+1

On 06/03/2016 12:45 PM, 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@gmail.com>
> ---
>  configure.ac                  |  5 +++++
>  liblttng-ust/Makefile.am      |  1 +
>  liblttng-ust/compat.h         | 51 ++++++++++++++++++++++++++++++++++++++++++-
>  liblttng-ust/lttng-ust-comm.c |  6 +++++
>  4 files changed, 62 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..68d4d50 100644
> --- a/liblttng-ust/compat.h
> +++ b/liblttng-ust/compat.h
> @@ -3,6 +3,7 @@
>  
>  /*
>   * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte@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,48 @@ 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_PROCNAME_SUFFIX "-ust"
> +
> +#include <pthread.h>
> +
> +static inline
> +void lttng_ust_setustprocname(void)
> +{
> +	char name[LTTNG_UST_PROCNAME_LEN];
> +	int limit = LTTNG_UST_PROCNAME_LEN - strlen(LTTNG_UST_PROCNAME_SUFFIX);
> +	int len;
> +
> +	lttng_ust_getprocname(name);
> +
> +	len = strlen(name);
> +	if (len > limit) {
> +		len = limit;
> +	}
> +
> +	sprintf(name + len, LTTNG_UST_PROCNAME_SUFFIX);
> +
> +	PTHREAD_SETNAME_NP(pthread_self(), name);
> +}
> +#else
> +static inline
> +void lttng_ust_setustprocname(void)
> +{
> +}
>  #endif
>  
>  #include <errno.h>
> diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c
> index e00a22c..c32e8ae 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) {
> 
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [lttng-ust PATCH v3 1/1] Add -ust to the name of UST threads of the application

[Mathieu Desnoyers]

----- On Jun 3, 2016, at 12:45 PM, Raphaël Beamonte raphael.beamonte@gmail.com 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@gmail.com>
> ---
> configure.ac                  |  5 +++++
> liblttng-ust/Makefile.am      |  1 +
> liblttng-ust/compat.h         | 51 ++++++++++++++++++++++++++++++++++++++++++-
> liblttng-ust/lttng-ust-comm.c |  6 +++++
> 4 files changed, 62 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..68d4d50 100644
> --- a/liblttng-ust/compat.h
> +++ b/liblttng-ust/compat.h
> @@ -3,6 +3,7 @@
> 
> /*
>  * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte@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

replace by:

static inline int lttng_pthread_setname_np(pthread_t thread, const char *name)
{
    return pthread_setname_np(thread, name);
}

> +#endif
> +
> #elif defined(__FreeBSD__)
> #include <stdlib.h>
> #include <string.h>
> @@ -59,6 +66,48 @@ 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

replace by:

static inline int lttng_pthread_setname_np(pthread_t thread, const char *name)
{
    return pthread_set_name_np(thread, name);
}

> +#endif
> +
> +#endif
> +
> +/*
> + * if a pthread setname/set_name function is available, declare

if -> If

> + * the setustprocname() function that will add '-ust' to the end
> + * of the current process name, while truncating it if needed.
> + */
> +#ifdef PTHREAD_SETNAME_NP

#if defined(HAVE_PTHREAD_SETNAME_NP) || defined(HAVE_PTHREAD_SETNAME_NP)

> +#define LTTNG_UST_PROCNAME_SUFFIX "-ust"
> +
> +#include <pthread.h>
> +
> +static inline
> +void lttng_ust_setustprocname(void)

should probably return int (error).

> +{
> +	char name[LTTNG_UST_PROCNAME_LEN];
> +	int limit = LTTNG_UST_PROCNAME_LEN - strlen(LTTNG_UST_PROCNAME_SUFFIX);
> +	int len;
> +
> +	lttng_ust_getprocname(name);
> +
> +	len = strlen(name);
> +	if (len > limit) {
> +		len = limit;
> +	}
> +
> +	sprintf(name + len, LTTNG_UST_PROCNAME_SUFFIX);

missing sprintf error handling.

> +
> +	PTHREAD_SETNAME_NP(pthread_self(), name);

replace by lttng_pthread_setname_np(), and add error handling.

> +}
> +#else
> +static inline
> +void lttng_ust_setustprocname(void)

should return int.

> +{
> +}
> #endif
> 
> #include <errno.h>
> diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c
> index e00a22c..c32e8ae 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();

Add error handling.

Thanks,

Mathieu

> +
> 	/* Restart trying to connect to the session daemon */
> restart:
> 	if (prev_connect_failed) {
> --
> 2.8.1
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Sebastien Boisvert]

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@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@efficios.com>
> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte@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) {
> 
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [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@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@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@efficios.com>
>> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte@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
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [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:54 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert sboisvert@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@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@efficios.com>
>>> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte@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

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Raphaël Beamonte]

2016-06-03 11:55 GMT-04:00 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>:
> ----- On Jun 3, 2016, at 5:54 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
>
>> ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert sboisvert@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@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@efficios.com>
>>>> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte@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
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [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 6:12 PM, Raphaël Beamonte raphael.beamonte@gmail.com wrote:

> 2016-06-03 11:55 GMT-04:00 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>:
>> ----- On Jun 3, 2016, at 5:54 PM, Mathieu Desnoyers
>> mathieu.desnoyers@efficios.com wrote:
>>
>>> ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert sboisvert@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@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@efficios.com>
>>>>> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte@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

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Sebastien Boisvert]

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@efficios.com>:
>> ----- On Jun 3, 2016, at 5:54 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
>>
>>> ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert sboisvert@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@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@efficios.com>
>>>>> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte@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
> 
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [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 6:24 PM, Sebastien Boisvert sboisvert@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@efficios.com>:
>>> ----- On Jun 3, 2016, at 5:54 PM, Mathieu Desnoyers
>>> mathieu.desnoyers@efficios.com wrote:
>>>
>>>> ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert sboisvert@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@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@efficios.com>
>>>>>> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte@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
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Raphaël Beamonte]

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@gmail.com>
---
 liblttng-ust/compat.h         | 22 ++++++++++++++++++++++
 liblttng-ust/lttng-ust-comm.c |  2 ++
 2 files changed, 24 insertions(+)

diff --git a/liblttng-ust/compat.h b/liblttng-ust/compat.h
index 43b2223..8d5fc77 100644
--- a/liblttng-ust/compat.h
+++ b/liblttng-ust/compat.h
@@ -34,6 +34,23 @@ void lttng_ust_getprocname(char *name)
 	(void) prctl(PR_GET_NAME, (unsigned long) name, 0, 0, 0);
 }
 
+static inline
+void lttng_ust_setustprocname()
+{
+	char name[LTTNG_UST_PROCNAME_LEN];
+	int limit = LTTNG_UST_PROCNAME_LEN - 4;
+
+	lttng_ust_getprocname(name);
+
+	if (strlen(name) >= limit) {
+		name[limit] = 0;
+	}
+
+	sprintf(name, "%s%s", name, "-ust");
+
+	(void) prctl(PR_SET_NAME, name, 0, 0, 0);
+}
+
 #elif defined(__FreeBSD__)
 #include <stdlib.h>
 #include <string.h>
@@ -59,6 +76,11 @@ void lttng_ust_getprocname(char *name)
 		strncpy(name, bsd_name, LTTNG_UST_PROCNAME_LEN - 1);
 }
 
+static inline
+void 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..a85cb21 100644
--- a/liblttng-ust/lttng-ust-comm.c
+++ b/liblttng-ust/lttng-ust-comm.c
@@ -1295,6 +1295,8 @@ void *ust_listener_thread(void *arg)
 	int sock, ret, prev_connect_failed = 0, has_waited = 0;
 	long timeout;
 
+	lttng_ust_setustprocname();
+
 	/* Restart trying to connect to the session daemon */
 restart:
 	if (prev_connect_failed) {
-- 
2.1.4

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

[RFC PATCH 2/2] Add -ust to the name of UST threads of the application

[Raphaël Beamonte]

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@gmail.com>
---
 liblttng-ust/Makefile.am      |  1 +
 liblttng-ust/compat.h         | 35 +++++++++++++++++++++++++++++++++++
 liblttng-ust/lttng-ust-comm.c |  2 ++
 3 files changed, 38 insertions(+)

diff --git a/liblttng-ust/Makefile.am b/liblttng-ust/Makefile.am
index 876e9b5..8d78d63 100644
--- a/liblttng-ust/Makefile.am
+++ b/liblttng-ust/Makefile.am
@@ -13,6 +13,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..161b957 100644
--- a/liblttng-ust/compat.h
+++ b/liblttng-ust/compat.h
@@ -24,6 +24,7 @@
  */
 #ifdef __linux__
 
+#include <pthread.h>
 #include <sys/prctl.h>
 
 #define LTTNG_UST_PROCNAME_LEN 17
@@ -34,6 +35,23 @@ void lttng_ust_getprocname(char *name)
 	(void) prctl(PR_GET_NAME, (unsigned long) name, 0, 0, 0);
 }
 
+static inline
+void lttng_ust_setustprocname()
+{
+	char name[LTTNG_UST_PROCNAME_LEN];
+	int limit = LTTNG_UST_PROCNAME_LEN - 4;
+
+	lttng_ust_getprocname(name);
+
+	if (strlen(name) >= limit) {
+		name[limit] = 0;
+	}
+
+	sprintf(name, "%s%s", name, "-ust");
+
+	pthread_setname_np(pthread_self(), name);
+}
+
 #elif defined(__FreeBSD__)
 #include <stdlib.h>
 #include <string.h>
@@ -59,6 +77,23 @@ void lttng_ust_getprocname(char *name)
 		strncpy(name, bsd_name, LTTNG_UST_PROCNAME_LEN - 1);
 }
 
+static inline
+void lttng_ust_setustprocname()
+{
+	char name[LTTNG_UST_PROCNAME_LEN];
+	int limit = LTTNG_UST_PROCNAME_LEN - 4;
+
+	lttng_ust_getprocname(name);
+
+	if (strlen(name) >= limit) {
+		name[limit] = 0;
+	}
+
+	sprintf(name, "%s%s", name, "-ust");
+
+	pthread_set_name_np(pthread_self(), name);
+}
+
 #endif
 
 #include <errno.h>
diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c
index e00a22c..a85cb21 100644
--- a/liblttng-ust/lttng-ust-comm.c
+++ b/liblttng-ust/lttng-ust-comm.c
@@ -1295,6 +1295,8 @@ void *ust_listener_thread(void *arg)
 	int sock, ret, prev_connect_failed = 0, has_waited = 0;
 	long timeout;
 
+	lttng_ust_setustprocname();
+
 	/* Restart trying to connect to the session daemon */
 restart:
 	if (prev_connect_failed) {
-- 
2.1.4

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [RFC PATCH 0/2] Identify UST threads by changing thread names

[Raphaël Beamonte]

Hi,

I did not get any feedback about this, any thoughts ?

Thanks,
Raphaël

2016-04-21 16:50 GMT-04:00 Raphaël Beamonte <raphael.beamonte@gmail.com>:
> Hi,
>
> This is an RFC patch for comments and ideas about how this should be
> done. In recent work I've been doing, I found out that some UST-
> instrumented userspace process was preempting itself. Looking further,
> it was not the same TID, but the same process name. These processes
> were LTTng-UST listener threads, but were not identified as so.
> Adding a simple symbol, such as "-ust" or "-lttng" at the end of the
> process name would help to identify such process directly in the
> generated trace.
>
> You'll find below two different implementations with their pros
> and cons, for which the "-ust" symbol was choosen:
>
>
> 1/ An implementation using prctl
>         It works properly on Linux, but I didn't find a way to make
>         it work under FreeBSD, hence the empty function in FreeBSD
>         case (keeping the situation as it is now).
>
> Raphaël Beamonte (1):
>   Add -ust to the name of UST threads of the application
>
>  liblttng-ust/compat.h         | 22 ++++++++++++++++++++++
>  liblttng-ust/lttng-ust-comm.c |  2 ++
>  2 files changed, 24 insertions(+)
>
>
>
> 2/ An implementation using pthread
>         It works on both Linux and FreeBSD, but requires to add a
>         link to the pthread library in liblttng-ust Makefile.am.
>         Also, this approach relies on the presence of the
>         pthread_setname_np function, which was included in glibc
>         2.12, meaning it will not work with older versions (but
>         this should not be a problem?)
>
> Raphaël Beamonte (1):
>   Add -ust to the name of UST threads of the application
>
>  liblttng-ust/Makefile.am      |  1 +
>  liblttng-ust/compat.h         | 35 +++++++++++++++++++++++++++++++++++
>  liblttng-ust/lttng-ust-comm.c |  2 ++
>  3 files changed, 38 insertions(+)
>
>
>
> I'll wait for your comments and ideas on that subject.
>
> Thanks,
> Raphaël
>
>
> --
> 2.1.4
>
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [RFC PATCH 0/2] Identify UST threads by changing thread names

[Jérémie Galarneau]

Hi Raphaël,

Adding Mathieu in CC. Please make sure you CC the maintainers when
submitting a patch.

Regards,
Jérémie

On Wed, Jun 1, 2016 at 11:04 AM, Raphaël Beamonte
<raphael.beamonte@gmail.com> wrote:
> Hi,
>
> I did not get any feedback about this, any thoughts ?
>
> Thanks,
> Raphaël
>
> 2016-04-21 16:50 GMT-04:00 Raphaël Beamonte <raphael.beamonte@gmail.com>:
>> Hi,
>>
>> This is an RFC patch for comments and ideas about how this should be
>> done. In recent work I've been doing, I found out that some UST-
>> instrumented userspace process was preempting itself. Looking further,
>> it was not the same TID, but the same process name. These processes
>> were LTTng-UST listener threads, but were not identified as so.
>> Adding a simple symbol, such as "-ust" or "-lttng" at the end of the
>> process name would help to identify such process directly in the
>> generated trace.
>>
>> You'll find below two different implementations with their pros
>> and cons, for which the "-ust" symbol was choosen:
>>
>>
>> 1/ An implementation using prctl
>>         It works properly on Linux, but I didn't find a way to make
>>         it work under FreeBSD, hence the empty function in FreeBSD
>>         case (keeping the situation as it is now).
>>
>> Raphaël Beamonte (1):
>>   Add -ust to the name of UST threads of the application
>>
>>  liblttng-ust/compat.h         | 22 ++++++++++++++++++++++
>>  liblttng-ust/lttng-ust-comm.c |  2 ++
>>  2 files changed, 24 insertions(+)
>>
>>
>>
>> 2/ An implementation using pthread
>>         It works on both Linux and FreeBSD, but requires to add a
>>         link to the pthread library in liblttng-ust Makefile.am.
>>         Also, this approach relies on the presence of the
>>         pthread_setname_np function, which was included in glibc
>>         2.12, meaning it will not work with older versions (but
>>         this should not be a problem?)
>>
>> Raphaël Beamonte (1):
>>   Add -ust to the name of UST threads of the application
>>
>>  liblttng-ust/Makefile.am      |  1 +
>>  liblttng-ust/compat.h         | 35 +++++++++++++++++++++++++++++++++++
>>  liblttng-ust/lttng-ust-comm.c |  2 ++
>>  3 files changed, 38 insertions(+)
>>
>>
>>
>> I'll wait for your comments and ideas on that subject.
>>
>> Thanks,
>> Raphaël
>>
>>
>> --
>> 2.1.4
>>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [RFC PATCH 0/2] Identify UST threads by changing thread names

[Jérémie Galarneau]

And, of course, I forget to add Mathieu in CC... Coffee time!

Jérémie

On Fri, Jun 3, 2016 at 2:10 AM, Jérémie Galarneau
<jeremie.galarneau@efficios.com> wrote:
> Hi Raphaël,
>
> Adding Mathieu in CC. Please make sure you CC the maintainers when
> submitting a patch.
>
> Regards,
> Jérémie
>
> On Wed, Jun 1, 2016 at 11:04 AM, Raphaël Beamonte
> <raphael.beamonte@gmail.com> wrote:
>> Hi,
>>
>> I did not get any feedback about this, any thoughts ?
>>
>> Thanks,
>> Raphaël
>>
>> 2016-04-21 16:50 GMT-04:00 Raphaël Beamonte <raphael.beamonte@gmail.com>:
>>> Hi,
>>>
>>> This is an RFC patch for comments and ideas about how this should be
>>> done. In recent work I've been doing, I found out that some UST-
>>> instrumented userspace process was preempting itself. Looking further,
>>> it was not the same TID, but the same process name. These processes
>>> were LTTng-UST listener threads, but were not identified as so.
>>> Adding a simple symbol, such as "-ust" or "-lttng" at the end of the
>>> process name would help to identify such process directly in the
>>> generated trace.
>>>
>>> You'll find below two different implementations with their pros
>>> and cons, for which the "-ust" symbol was choosen:
>>>
>>>
>>> 1/ An implementation using prctl
>>>         It works properly on Linux, but I didn't find a way to make
>>>         it work under FreeBSD, hence the empty function in FreeBSD
>>>         case (keeping the situation as it is now).
>>>
>>> Raphaël Beamonte (1):
>>>   Add -ust to the name of UST threads of the application
>>>
>>>  liblttng-ust/compat.h         | 22 ++++++++++++++++++++++
>>>  liblttng-ust/lttng-ust-comm.c |  2 ++
>>>  2 files changed, 24 insertions(+)
>>>
>>>
>>>
>>> 2/ An implementation using pthread
>>>         It works on both Linux and FreeBSD, but requires to add a
>>>         link to the pthread library in liblttng-ust Makefile.am.
>>>         Also, this approach relies on the presence of the
>>>         pthread_setname_np function, which was included in glibc
>>>         2.12, meaning it will not work with older versions (but
>>>         this should not be a problem?)
>>>
>>> Raphaël Beamonte (1):
>>>   Add -ust to the name of UST threads of the application
>>>
>>>  liblttng-ust/Makefile.am      |  1 +
>>>  liblttng-ust/compat.h         | 35 +++++++++++++++++++++++++++++++++++
>>>  liblttng-ust/lttng-ust-comm.c |  2 ++
>>>  3 files changed, 38 insertions(+)
>>>
>>>
>>>
>>> I'll wait for your comments and ideas on that subject.
>>>
>>> Thanks,
>>> Raphaël
>>>
>>>
>>> --
>>> 2.1.4
>>>
>> _______________________________________________
>> lttng-dev mailing list
>> lttng-dev@lists.lttng.org
>> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>
>
>
> --
> Jérémie Galarneau
> EfficiOS Inc.
> http://www.efficios.com



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [RFC PATCH 0/2] Identify UST threads by changing thread names

[Mathieu Desnoyers]

----- On Jun 3, 2016, at 8:11 AM, Jeremie Galarneau jeremie.galarneau@efficios.com wrote:

> And, of course, I forget to add Mathieu in CC... Coffee time!
> 
> Jérémie
> 
> On Fri, Jun 3, 2016 at 2:10 AM, Jérémie Galarneau
> <jeremie.galarneau@efficios.com> wrote:
>> Hi Raphaël,
>>
>> Adding Mathieu in CC. Please make sure you CC the maintainers when
>> submitting a patch.
>>
>> Regards,
>> Jérémie
>>
>> On Wed, Jun 1, 2016 at 11:04 AM, Raphaël Beamonte
>> <raphael.beamonte@gmail.com> wrote:
>>> Hi,
>>>
>>> I did not get any feedback about this, any thoughts ?
>>>
>>> Thanks,
>>> Raphaël
>>>
>>> 2016-04-21 16:50 GMT-04:00 Raphaël Beamonte <raphael.beamonte@gmail.com>:
>>>> Hi,
>>>>
>>>> This is an RFC patch for comments and ideas about how this should be
>>>> done. In recent work I've been doing, I found out that some UST-
>>>> instrumented userspace process was preempting itself. Looking further,
>>>> it was not the same TID, but the same process name. These processes
>>>> were LTTng-UST listener threads, but were not identified as so.
>>>> Adding a simple symbol, such as "-ust" or "-lttng" at the end of the
>>>> process name would help to identify such process directly in the
>>>> generated trace.
>>>>
>>>> You'll find below two different implementations with their pros
>>>> and cons, for which the "-ust" symbol was choosen:
>>>>
>>>>
>>>> 1/ An implementation using prctl
>>>>         It works properly on Linux, but I didn't find a way to make
>>>>         it work under FreeBSD, hence the empty function in FreeBSD
>>>>         case (keeping the situation as it is now).
>>>>
>>>> Raphaël Beamonte (1):
>>>>   Add -ust to the name of UST threads of the application
>>>>
>>>>  liblttng-ust/compat.h         | 22 ++++++++++++++++++++++
>>>>  liblttng-ust/lttng-ust-comm.c |  2 ++
>>>>  2 files changed, 24 insertions(+)
>>>>
>>>>
>>>>
>>>> 2/ An implementation using pthread
>>>>         It works on both Linux and FreeBSD, but requires to add a
>>>>         link to the pthread library in liblttng-ust Makefile.am.
>>>>         Also, this approach relies on the presence of the
>>>>         pthread_setname_np function, which was included in glibc
>>>>         2.12, meaning it will not work with older versions (but
>>>>         this should not be a problem?)

lttng-ust already uses libc for mutexes and other. So I don't see why
you have to _add_ any explicit dependency there.

About the glibc version, we should use autotools to detect the
features supported by the system libc, and pass a config define
to the source code, so we can use the glibc pthread_setname_np
if available, else we keep the current behavior.

Thoughts ?

Thanks,

Mathieu

>>>>
>>>> Raphaël Beamonte (1):
>>>>   Add -ust to the name of UST threads of the application
>>>>
>>>>  liblttng-ust/Makefile.am      |  1 +
>>>>  liblttng-ust/compat.h         | 35 +++++++++++++++++++++++++++++++++++
>>>>  liblttng-ust/lttng-ust-comm.c |  2 ++
>>>>  3 files changed, 38 insertions(+)
>>>>
>>>>
>>>>
>>>> I'll wait for your comments and ideas on that subject.
>>>>
>>>> Thanks,
>>>> Raphaël
>>>>
>>>>
>>>> --
>>>> 2.1.4
>>>>
>>> _______________________________________________
>>> lttng-dev mailing list
>>> lttng-dev@lists.lttng.org
>>> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>>
>>
>>
>> --
>> Jérémie Galarneau
>> EfficiOS Inc.
>> http://www.efficios.com
> 
> 
> 
> --
> Jérémie Galarneau
> EfficiOS Inc.
> http://www.efficios.com

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [RFC PATCH 0/2] Identify UST threads by changing thread names

[Mathieu Desnoyers]

----- On Jun 3, 2016, at 8:18 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> ----- On Jun 3, 2016, at 8:11 AM, Jeremie Galarneau
> jeremie.galarneau@efficios.com wrote:
> 
>> And, of course, I forget to add Mathieu in CC... Coffee time!
>> 
>> Jérémie
>> 
>> On Fri, Jun 3, 2016 at 2:10 AM, Jérémie Galarneau
>> <jeremie.galarneau@efficios.com> wrote:
>>> Hi Raphaël,
>>>
>>> Adding Mathieu in CC. Please make sure you CC the maintainers when
>>> submitting a patch.
>>>
>>> Regards,
>>> Jérémie
>>>
>>> On Wed, Jun 1, 2016 at 11:04 AM, Raphaël Beamonte
>>> <raphael.beamonte@gmail.com> wrote:
>>>> Hi,
>>>>
>>>> I did not get any feedback about this, any thoughts ?
>>>>
>>>> Thanks,
>>>> Raphaël
>>>>
>>>> 2016-04-21 16:50 GMT-04:00 Raphaël Beamonte <raphael.beamonte@gmail.com>:
>>>>> Hi,
>>>>>
>>>>> This is an RFC patch for comments and ideas about how this should be
>>>>> done. In recent work I've been doing, I found out that some UST-
>>>>> instrumented userspace process was preempting itself. Looking further,
>>>>> it was not the same TID, but the same process name. These processes
>>>>> were LTTng-UST listener threads, but were not identified as so.
>>>>> Adding a simple symbol, such as "-ust" or "-lttng" at the end of the
>>>>> process name would help to identify such process directly in the
>>>>> generated trace.
>>>>>
>>>>> You'll find below two different implementations with their pros
>>>>> and cons, for which the "-ust" symbol was choosen:
>>>>>
>>>>>
>>>>> 1/ An implementation using prctl
>>>>>         It works properly on Linux, but I didn't find a way to make
>>>>>         it work under FreeBSD, hence the empty function in FreeBSD
>>>>>         case (keeping the situation as it is now).
>>>>>
>>>>> Raphaël Beamonte (1):
>>>>>   Add -ust to the name of UST threads of the application
>>>>>
>>>>>  liblttng-ust/compat.h         | 22 ++++++++++++++++++++++
>>>>>  liblttng-ust/lttng-ust-comm.c |  2 ++
>>>>>  2 files changed, 24 insertions(+)
>>>>>
>>>>>
>>>>>
>>>>> 2/ An implementation using pthread
>>>>>         It works on both Linux and FreeBSD, but requires to add a
>>>>>         link to the pthread library in liblttng-ust Makefile.am.
>>>>>         Also, this approach relies on the presence of the
>>>>>         pthread_setname_np function, which was included in glibc
>>>>>         2.12, meaning it will not work with older versions (but
>>>>>         this should not be a problem?)
> 
> lttng-ust already uses libc for mutexes and other. So I don't see why
> you have to _add_ any explicit dependency there.

and we already explicitly use pthread (pthread_create).

Thanks,

Mathieu

> 
> About the glibc version, we should use autotools to detect the
> features supported by the system libc, and pass a config define
> to the source code, so we can use the glibc pthread_setname_np
> if available, else we keep the current behavior.
> 
> Thoughts ?
> 
> Thanks,
> 
> Mathieu
> 
>>>>>
>>>>> Raphaël Beamonte (1):
>>>>>   Add -ust to the name of UST threads of the application
>>>>>
>>>>>  liblttng-ust/Makefile.am      |  1 +
>>>>>  liblttng-ust/compat.h         | 35 +++++++++++++++++++++++++++++++++++
>>>>>  liblttng-ust/lttng-ust-comm.c |  2 ++
>>>>>  3 files changed, 38 insertions(+)
>>>>>
>>>>>
>>>>>
>>>>> I'll wait for your comments and ideas on that subject.
>>>>>
>>>>> Thanks,
>>>>> Raphaël
>>>>>
>>>>>
>>>>> --
>>>>> 2.1.4
>>>>>
>>>> _______________________________________________
>>>> lttng-dev mailing list
>>>> lttng-dev@lists.lttng.org
>>>> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>>>
>>>
>>>
>>> --
>>> Jérémie Galarneau
>>> EfficiOS Inc.
>>> http://www.efficios.com
>> 
>> 
>> 
>> --
>> Jérémie Galarneau
>> EfficiOS Inc.
>> http://www.efficios.com
> 
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [RFC PATCH 0/2] Identify UST threads by changing thread names

[Raphaël Beamonte]

Hi!

Thanks for all of your comments, I appreciate it!
Thanks Jeremie for adding Mathieu in Cc, forgot to do it... Will
remember next time! :)

2016-06-02 9:22 GMT-04:00 Sebastien Boisvert <sboisvert@gydle.com>:
> Do you know if the name of these listener threads can be set in their constructor, directly
> when they are brought into this world ?

pthread_create does not allow to specify a name when creating the new
thread, as far as I know! That's why the 'lttng_ust_setustprocname()'
function is called first thing in the ust_listener_thread function.

> Perhaps the availability of pthread_setname_np and/or pthread_set_name_np (you have both names
> in your patch 2) should be ascertained in another venue, such as the configure.ac -- where it belongs.

True!

2016-06-03 2:24 GMT-04:00 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>:
> ----- On Jun 3, 2016, at 8:18 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
>> lttng-ust already uses libc for mutexes and other. So I don't see why
>> you have to _add_ any explicit dependency there.
>
> and we already explicitly use pthread (pthread_create).

You're right, pthread is already used. However, when compiling the
code without adding the pthread dependency, I get the following
warning:

In file included from lttng-tracer.h:30:0,
                 from lttng-ust-abi.c:50:
compat.h: In function ‘lttng_ust_setustprocname’:
compat.h:52:2: warning: implicit declaration of function
‘pthread_setname_np’ [-Wimplicit-function-declaration]
  pthread_setname_np(pthread_self(), name);
  ^
  CC       lttng-context-procname.lo
In file included from lttng-context-procname.c:28:0:
compat.h: In function ‘lttng_ust_setustprocname’:
compat.h:52:2: warning: implicit declaration of function
‘pthread_setname_np’ [-Wimplicit-function-declaration]
  pthread_setname_np(pthread_self(), name);
  ^

It seems to be because pthread is not specified as dependency for
liblttng-ust-tracepoint in liblttng-ust/Makefile.am. However, perhaps
that pthread dependency should be added elsewhere?

>> About the glibc version, we should use autotools to detect the
>> features supported by the system libc, and pass a config define
>> to the source code, so we can use the glibc pthread_setname_np
>> if available, else we keep the current behavior.

That's a good idea, will do!

I'll update and rebase the patch following your comments.
If you have any other insight about the pthread dependency, I'm all ears!

Thanks,
Raphaël
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [RFC PATCH 0/2] Identify UST threads by changing thread names

[Mathieu Desnoyers]

----- On Jun 3, 2016, at 4:12 PM, Raphaël Beamonte raphael.beamonte@gmail.com wrote:

> Hi!
> 
> Thanks for all of your comments, I appreciate it!
> Thanks Jeremie for adding Mathieu in Cc, forgot to do it... Will
> remember next time! :)
> 
> 2016-06-02 9:22 GMT-04:00 Sebastien Boisvert <sboisvert@gydle.com>:
>> Do you know if the name of these listener threads can be set in their
>> constructor, directly
>> when they are brought into this world ?
> 
> pthread_create does not allow to specify a name when creating the new
> thread, as far as I know! That's why the 'lttng_ust_setustprocname()'
> function is called first thing in the ust_listener_thread function.
> 
>> Perhaps the availability of pthread_setname_np and/or pthread_set_name_np (you
>> have both names
>> in your patch 2) should be ascertained in another venue, such as the
>> configure.ac -- where it belongs.
> 
> True!
> 
> 2016-06-03 2:24 GMT-04:00 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>:
>> ----- On Jun 3, 2016, at 8:18 AM, Mathieu Desnoyers
>> mathieu.desnoyers@efficios.com wrote:
>>> lttng-ust already uses libc for mutexes and other. So I don't see why
>>> you have to _add_ any explicit dependency there.
>>
>> and we already explicitly use pthread (pthread_create).
> 
> You're right, pthread is already used. However, when compiling the
> code without adding the pthread dependency, I get the following
> warning:
> 
> In file included from lttng-tracer.h:30:0,
>                 from lttng-ust-abi.c:50:
> compat.h: In function ‘lttng_ust_setustprocname’:
> compat.h:52:2: warning: implicit declaration of function
> ‘pthread_setname_np’ [-Wimplicit-function-declaration]
>  pthread_setname_np(pthread_self(), name);
>  ^
>  CC       lttng-context-procname.lo
> In file included from lttng-context-procname.c:28:0:
> compat.h: In function ‘lttng_ust_setustprocname’:
> compat.h:52:2: warning: implicit declaration of function
> ‘pthread_setname_np’ [-Wimplicit-function-declaration]
>  pthread_setname_np(pthread_self(), name);
>  ^
> 
> It seems to be because pthread is not specified as dependency for
> liblttng-ust-tracepoint in liblttng-ust/Makefile.am. However, perhaps
> that pthread dependency should be added elsewhere?

Seems fair.

Thanks,

Mathieu

> 
>>> About the glibc version, we should use autotools to detect the
>>> features supported by the system libc, and pass a config define
>>> to the source code, so we can use the glibc pthread_setname_np
>>> if available, else we keep the current behavior.
> 
> That's a good idea, will do!
> 
> I'll update and rebase the patch following your comments.
> If you have any other insight about the pthread dependency, I'm all ears!
> 
> Thanks,
> Raphaël

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

[lttng-ust RFC PATCH v2 0/1] Identify UST threads by changing thread names

[Raphaël Beamonte]

Hi,

This is the update of the RFC patch following your comments.
This patch updates the pthread_setname_np/pthread_set_name_np version of
the RFC patch, and includes the following changes:
- Check in configure.ac whether the pthread_setname_np or
  pthread_set_name_np functions exists. If it's not the case,
  current behavior is preserved.
- Update compat.h to define a macro for the pthread_set*name_np
  function to use in order to reduce repeated code.
- Update compat.h and lttng-ust-comm.c to use a macro to call the
  lttng_ust_setustprocname function in order for that macro to be
  empty when the required pthread_setn*ame_np is not available.
- Update compat.h to fix sprintf usage with the same variable to
  read from and write to.
- The patch has been rebased on current lttng-ust's master branch.

This patch still contains the link to the pthread library in the
liblttng-ust/Makefile.am file, which still seems required to avoid
compilation warnings about implicit function declaration.

Thoughts and comments?

Thanks!
Raphaël


Raphaël Beamonte (1):
  Add -ust to the name of UST threads of the application

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

-- 
2.8.1

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Raphaël Beamonte]

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@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@efficios.com>
+ * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte@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) {
-- 
2.8.1

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

[lttng-ust PATCH v3 0/1] Identify UST threads by changing thread names

[Raphaël Beamonte]

Hi,

This is the update of the RFC patch following your comments.
This patch updates the pthread_setname_np/pthread_set_name_np version of
the RFC patch, and includes the following changes:
- Add a LTTNG_UST_PROCNAME_SUFFIX macro in compat.h that contains the
  string to be added at the end of the process name
- Fix the limit value (was 4 instead of 5, now uses directly strlen on
  the LTTNG_UST_PROCNAME_SUFFIX macro value)
- Removes the macro used to call the lttng_ust_setustprocname function,
  an empty lttng_ust_setustprocname is called instead when the set*name
  pthread functions are not available
- Replaced 'lttng_ust_setustprocname()' per 'lttng_ust_setustprocname(void)'
  in the function declaration.

This should address all the comments you had!
Any other thoughts or comments?

Thanks a lot!
Raphaël


Raphaël Beamonte (1):
  Add -ust to the name of UST threads of the application

 configure.ac                  |  5 +++++
 liblttng-ust/Makefile.am      |  1 +
 liblttng-ust/compat.h         | 51 ++++++++++++++++++++++++++++++++++++++++++-
 liblttng-ust/lttng-ust-comm.c |  6 +++++
 4 files changed, 62 insertions(+), 1 deletion(-)

-- 
2.8.1

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Raphaël Beamonte]

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@gmail.com>
---
 configure.ac                  |  5 +++++
 liblttng-ust/Makefile.am      |  1 +
 liblttng-ust/compat.h         | 51 ++++++++++++++++++++++++++++++++++++++++++-
 liblttng-ust/lttng-ust-comm.c |  6 +++++
 4 files changed, 62 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..68d4d50 100644
--- a/liblttng-ust/compat.h
+++ b/liblttng-ust/compat.h
@@ -3,6 +3,7 @@
 
 /*
  * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
+ * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte@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,48 @@ 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_PROCNAME_SUFFIX "-ust"
+
+#include <pthread.h>
+
+static inline
+void lttng_ust_setustprocname(void)
+{
+	char name[LTTNG_UST_PROCNAME_LEN];
+	int limit = LTTNG_UST_PROCNAME_LEN - strlen(LTTNG_UST_PROCNAME_SUFFIX);
+	int len;
+
+	lttng_ust_getprocname(name);
+
+	len = strlen(name);
+	if (len > limit) {
+		len = limit;
+	}
+
+	sprintf(name + len, LTTNG_UST_PROCNAME_SUFFIX);
+
+	PTHREAD_SETNAME_NP(pthread_self(), name);
+}
+#else
+static inline
+void lttng_ust_setustprocname(void)
+{
+}
 #endif
 
 #include <errno.h>
diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c
index e00a22c..c32e8ae 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) {
-- 
2.8.1

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev