Add -ust to the name of UST threads of the application

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

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 <[email protected]>:

Quoted 47 lines

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

Quoted 9 lines

I'll wait for your comments and ideas on that subject.

Thanks,
Raphaël

--
2.1.4

_______________________________________________
lttng-dev mailing list
[email protected]
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Hi Raphaël,

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

Regards,
Jérémie

Quoted 61 lines · 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 <[email protected]>:

Quoted 53 lines

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

Quoted 7 lines

Quoted 3 lines

--
2.1.4

_______________________________________________
lttng-dev mailing list
[email protected]
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com

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

Jérémie

Quoted 54 lines · 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

Quoted 44 lines · 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 <[email protected]>:

Quoted 36 lines

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

Quoted 27 lines

Quoted 17 lines

Quoted 17 lines

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

Quoted 7 lines

Quoted 3 lines

--
2.1.4

_______________________________________________
lttng-dev mailing list
[email protected]
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Quoted 4 lines

--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com

--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com

----- On Jun 3, 2016, at 8:11 AM, Jeremie Galarneau

Quoted 60 lines · [email protected] wrote:

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

Jérémie

Quoted 54 lines · 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

Quoted 44 lines · 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 <[email protected]>:

Quoted 36 lines

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

Quoted 34 lines

Quoted 27 lines

Quoted 17 lines

Quoted 17 lines

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

Quoted 7 lines

Quoted 3 lines

--
2.1.4

_______________________________________________
lttng-dev mailing list
[email protected]
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Quoted 4 lines

--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com

Quoted 4 lines

--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com

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

Do you know if the name of these listener threads can be set in their constructor, directly when they are brought into this world ?

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.

Quoted 2 lines · ----- On Jun 3, 2016, at 8:18 AM, Mathieu Desnoyers…

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

Quoted 4 lines

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.

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 <[email protected]>:

Quoted 3 lines

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.

Quoted 4 lines

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 <[email protected]>:

Quoted 6 lines

----- On Jun 3, 2016, at 8:18 AM, Mathieu Desnoyers

Quoted 2 lines · [email protected] 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?

Quoted 5 lines

Quoted 5 lines

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

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 <[email protected]>

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

configure.ac +5 -0

index 5692553..de462ff 100644
@@ -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])

liblttng-ust/Makefile.am +1 -0

index f1801cf..05929be 100644
@@ -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

liblttng-ust/compat.h +21 -1

index 43b2223..4fee919 100644
@@ -3,6 +3,7 @@
 
 /*
  * Copyright (C) 2011 Mathieu Desnoyers <[email protected]>
+ * Copyright (C) 2016 Raphaël Beamonte <[email protected]>
  *
  * 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.

liblttng-ust/compat.h +5 -0

+
+	PTHREAD_SETNAME_NP(pthread_self(), name);
+}
+#else
+#define LTTNG_UST_SETUSTPROCNAME
 #endif
 
 #include <errno.h>

liblttng-ust/lttng-ust-comm.c +0 -0

index e00a22c..4de68c7 100644

Hi Raphaël,

I think you have a buffer overflow, see below.

Quoted 96 lines · 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 <[email protected]>

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

configure.ac +5 -0

index 5692553..de462ff 100644
@@ -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])

liblttng-ust/Makefile.am +1 -0

index f1801cf..05929be 100644
@@ -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

liblttng-ust/compat.h +21 -1

index 43b2223..4fee919 100644
@@ -3,6 +3,7 @@
 
 /*
  * Copyright (C) 2011 Mathieu Desnoyers <[email protected]>
+ * Copyright (C) 2016 Raphaël Beamonte <[email protected]>
  *
  * 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.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 91

Quoted 10 lines

liblttng-ust/compat.h +10 -0

+ */
+#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.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 92

Quoted 1 line

liblttng-ust/compat.h +1 -0

+	int limit = LTTNG_UST_PROCNAME_LEN - 4;

Then, limit is 10 - 4 = 6.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 93

Quoted 1 line

liblttng-ust/compat.h +1 -0

+	int len = 0;

This can be moved on the strlen(name) line.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 95

Quoted 2 lines

liblttng-ust/compat.h +2 -0

+
+	lttng_ust_getprocname(name);

Let's suppose that after the call to lttng_ust_getprocname(), name contains "012345678\0",

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 97

Quoted 2 lines

liblttng-ust/compat.h +2 -0

+
+	len = strlen(name);

The variable "len" will contain the integer 9.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 101

Quoted 4 lines

liblttng-ust/compat.h +4 -0

+	if (len >= limit) {
+		len = limit;
+	}
+

After that, len is >= limit (9 >= 6), so len becomes 6.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 102

Quoted 1 line

liblttng-ust/compat.h +1 -0

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

Quoted 13 lines

liblttng-ust/compat.h +5 -0

+
+	PTHREAD_SETNAME_NP(pthread_self(), name);
+}
+#else
+#define LTTNG_UST_SETUSTPROCNAME
 #endif
 
 #include <errno.h>

liblttng-ust/lttng-ust-comm.c +0 -0

index e00a22c..4de68c7 100644

liblttng-ust/lttng-ust-comm.c void *ust_listener_thread(void *arg) · line 1302

Quoted 8 lines

liblttng-ust/lttng-ust-comm.c +5 -0

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

Quoted 7 lines

liblttng-ust/lttng-ust-comm.c +1 -0

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

Quoted 180 lines · ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert [email protected] wrote:

Hi Raphaël,

I think you have a buffer overflow, see below.

Quoted 96 lines · 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 <[email protected]>

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

configure.ac +5 -0

index 5692553..de462ff 100644
@@ -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])

liblttng-ust/Makefile.am +1 -0

index f1801cf..05929be 100644
@@ -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

liblttng-ust/compat.h +21 -1

index 43b2223..4fee919 100644
@@ -3,6 +3,7 @@
 
 /*
  * Copyright (C) 2011 Mathieu Desnoyers <[email protected]>
+ * Copyright (C) 2016 Raphaël Beamonte <[email protected]>
  *
  * 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.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 91

Quoted 10 lines

liblttng-ust/compat.h +10 -0

+ */
+#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.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 92

Quoted 1 line

liblttng-ust/compat.h +1 -0

+	int limit = LTTNG_UST_PROCNAME_LEN - 4;

Then, limit is 10 - 4 = 6.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 93

Quoted 1 line

liblttng-ust/compat.h +1 -0

+	int len = 0;

This can be moved on the strlen(name) line.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 95

Quoted 2 lines

liblttng-ust/compat.h +2 -0

+
+	lttng_ust_getprocname(name);

Let's suppose that after the call to lttng_ust_getprocname(), name contains "012345678\0",

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 97

Quoted 2 lines

liblttng-ust/compat.h +2 -0

+
+	len = strlen(name);

The variable "len" will contain the integer 9.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 101

Quoted 4 lines

liblttng-ust/compat.h +4 -0

+	if (len >= limit) {
+		len = limit;
+	}
+

After that, len is >= limit (9 >= 6), so len becomes 6.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 102

Quoted 1 line

liblttng-ust/compat.h +1 -0

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

Quoted 13 lines

liblttng-ust/compat.h +5 -0

+
+	PTHREAD_SETNAME_NP(pthread_self(), name);
+}
+#else
+#define LTTNG_UST_SETUSTPROCNAME
 #endif
 
 #include <errno.h>

liblttng-ust/lttng-ust-comm.c +0 -0

index e00a22c..4de68c7 100644

liblttng-ust/lttng-ust-comm.c void *ust_listener_thread(void *arg) · line 1302

Quoted 8 lines

liblttng-ust/lttng-ust-comm.c +5 -0

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

Quoted 5 lines

LTTNG_UST_SETUSTPROCNAME()

or

lttng_ust_setustprocname()

The latter is preferred by our coding style, indeed.

Thanks,

Mathieu

Quoted 7 lines

liblttng-ust/lttng-ust-comm.c void *ust_listener_thread(void *arg) · line 1306

Quoted 7 lines

liblttng-ust/lttng-ust-comm.c +1 -0

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

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

Quoted 146 lines · ----- On Jun 3, 2016, at 5:54 PM, Mathieu Desnoyers…

Quoted 144 lines · ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert [email protected] wrote:

Hi Raphaël,

I think you have a buffer overflow, see below.

Quoted 96 lines · 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 <[email protected]>

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

configure.ac +5 -0

index 5692553..de462ff 100644
@@ -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])

liblttng-ust/Makefile.am +1 -0

index f1801cf..05929be 100644
@@ -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

liblttng-ust/compat.h +21 -1

index 43b2223..4fee919 100644
@@ -3,6 +3,7 @@
 
 /*
  * Copyright (C) 2011 Mathieu Desnoyers <[email protected]>
+ * Copyright (C) 2016 Raphaël Beamonte <[email protected]>
  *
  * 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.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 91

Quoted 10 lines

liblttng-ust/compat.h +10 -0

+ */
+#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.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 92

Quoted 1 line

liblttng-ust/compat.h +1 -0

+   int limit = LTTNG_UST_PROCNAME_LEN - 4;

Then, limit is 10 - 4 = 6.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 93

Quoted 1 line

liblttng-ust/compat.h +1 -0

+   int len = 0;

This can be moved on the strlen(name) line.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 95

Quoted 2 lines

liblttng-ust/compat.h +2 -0

+
+   lttng_ust_getprocname(name);

Let's suppose that after the call to lttng_ust_getprocname(), name contains "012345678\0",

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 97

Quoted 2 lines

liblttng-ust/compat.h +2 -0

+
+   len = strlen(name);

The variable "len" will contain the integer 9.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 101

Quoted 4 lines

liblttng-ust/compat.h +4 -0

+   if (len >= limit) {
+           len = limit;
+   }
+

After that, len is >= limit (9 >= 6), so len becomes 6.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 102

Quoted 1 line

liblttng-ust/compat.h +1 -0

+   sprintf(name + len, "%s", "-ust");

name is a pointer to "012345678\0" (9 bytes + '\0')

Quoted 7 lines

Quoted 7 lines

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.

Quoted 33 lines

Quoted 33 lines

Quoted 15 lines

liblttng-ust/compat.h +5 -0

+
+   PTHREAD_SETNAME_NP(pthread_self(), name);
+}
+#else
+#define LTTNG_UST_SETUSTPROCNAME
 #endif
 
 #include <errno.h>

liblttng-ust/lttng-ust-comm.c +0 -0

index e00a22c..4de68c7 100644

liblttng-ust/lttng-ust-comm.c void *ust_listener_thread(void *arg) · line 1302

Quoted 8 lines

liblttng-ust/lttng-ust-comm.c +5 -0

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

Quoted 4 lines

Quoted 1 line

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.

2016-06-03 11:55 GMT-04:00 Mathieu Desnoyers <[email protected]>:

Quoted 156 lines

----- On Jun 3, 2016, at 5:54 PM, Mathieu Desnoyers

Quoted 153 lines · [email protected] wrote:

Quoted 151 lines · ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert [email protected] wrote:

Hi Raphaël,

I think you have a buffer overflow, see below.

Quoted 96 lines · 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 <[email protected]>

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

configure.ac +5 -0

index 5692553..de462ff 100644
@@ -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])

liblttng-ust/Makefile.am +1 -0

index f1801cf..05929be 100644
@@ -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

liblttng-ust/compat.h +21 -1

index 43b2223..4fee919 100644
@@ -3,6 +3,7 @@
 
 /*
  * Copyright (C) 2011 Mathieu Desnoyers <[email protected]>
+ * Copyright (C) 2016 Raphaël Beamonte <[email protected]>
  *
  * 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.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 91

Quoted 10 lines

liblttng-ust/compat.h +10 -0

+ */
+#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.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 92

Quoted 1 line

liblttng-ust/compat.h +1 -0

+   int limit = LTTNG_UST_PROCNAME_LEN - 4;

Then, limit is 10 - 4 = 6.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 93

Quoted 1 line

liblttng-ust/compat.h +1 -0

+   int len = 0;

This can be moved on the strlen(name) line.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 95

Quoted 2 lines

liblttng-ust/compat.h +2 -0

+
+   lttng_ust_getprocname(name);

Let's suppose that after the call to lttng_ust_getprocname(), name contains "012345678\0",

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 97

Quoted 2 lines

liblttng-ust/compat.h +2 -0

+
+   len = strlen(name);

The variable "len" will contain the integer 9.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 101

Quoted 4 lines

liblttng-ust/compat.h +4 -0

+   if (len >= limit) {
+           len = limit;
+   }
+

After that, len is >= limit (9 >= 6), so len becomes 6.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 102

Quoted 1 line

liblttng-ust/compat.h +1 -0

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

Quoted 33 lines

Quoted 33 lines

Quoted 33 lines

Quoted 15 lines

liblttng-ust/compat.h +5 -0

+
+   PTHREAD_SETNAME_NP(pthread_self(), name);
+}
+#else
+#define LTTNG_UST_SETUSTPROCNAME
 #endif
 
 #include <errno.h>

liblttng-ust/lttng-ust-comm.c +0 -0

index e00a22c..4de68c7 100644

liblttng-ust/lttng-ust-comm.c void *ust_listener_thread(void *arg) · line 1302

Quoted 8 lines

liblttng-ust/lttng-ust-comm.c +5 -0

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

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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

2016-06-03 11:55 GMT-04:00 Mathieu Desnoyers <[email protected]>:

Quoted 148 lines

Quoted 146 lines · ----- On Jun 3, 2016, at 5:54 PM, Mathieu Desnoyers…

Quoted 144 lines · ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert [email protected] wrote:

Hi Raphaël,

I think you have a buffer overflow, see below.

Quoted 96 lines · 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 <[email protected]>

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

configure.ac +5 -0

index 5692553..de462ff 100644
@@ -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])

liblttng-ust/Makefile.am +1 -0

index f1801cf..05929be 100644
@@ -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

liblttng-ust/compat.h +21 -1

index 43b2223..4fee919 100644
@@ -3,6 +3,7 @@
 
 /*
  * Copyright (C) 2011 Mathieu Desnoyers <[email protected]>
+ * Copyright (C) 2016 Raphaël Beamonte <[email protected]>
  *
  * 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.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 91

Quoted 10 lines

liblttng-ust/compat.h +10 -0

+ */
+#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.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 92

Quoted 1 line

liblttng-ust/compat.h +1 -0

+   int limit = LTTNG_UST_PROCNAME_LEN - 4;

Then, limit is 10 - 4 = 6.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 93

Quoted 1 line

liblttng-ust/compat.h +1 -0

+   int len = 0;

This can be moved on the strlen(name) line.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 95

Quoted 2 lines

liblttng-ust/compat.h +2 -0

+
+   lttng_ust_getprocname(name);

Let's suppose that after the call to lttng_ust_getprocname(), name contains "012345678\0",

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 97

Quoted 2 lines

liblttng-ust/compat.h +2 -0

+
+   len = strlen(name);

The variable "len" will contain the integer 9.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 101

Quoted 4 lines

liblttng-ust/compat.h +4 -0

+   if (len >= limit) {
+           len = limit;
+   }
+

After that, len is >= limit (9 >= 6), so len becomes 6.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 102

Quoted 1 line

liblttng-ust/compat.h +1 -0

+   sprintf(name + len, "%s", "-ust");

name is a pointer to "012345678\0" (9 bytes + '\0')

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

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

Quoted 33 lines

Quoted 33 lines

Quoted 33 lines

Quoted 15 lines

liblttng-ust/compat.h +5 -0

+
+   PTHREAD_SETNAME_NP(pthread_self(), name);
+}
+#else
+#define LTTNG_UST_SETUSTPROCNAME
 #endif
 
 #include <errno.h>

liblttng-ust/lttng-ust-comm.c +0 -0

index e00a22c..4de68c7 100644

liblttng-ust/lttng-ust-comm.c void *ust_listener_thread(void *arg) · line 1302

Quoted 8 lines

liblttng-ust/lttng-ust-comm.c +5 -0

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

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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

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.

Quoted 257 lines

2016-06-03 11:55 GMT-04:00 Mathieu Desnoyers <[email protected]>:

Quoted 156 lines

----- On Jun 3, 2016, at 5:54 PM, Mathieu Desnoyers

Quoted 153 lines · [email protected] wrote:

Quoted 151 lines · ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert [email protected] wrote:

Hi Raphaël,

I think you have a buffer overflow, see below.

Quoted 96 lines · 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 <[email protected]>

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

configure.ac +5 -0

index 5692553..de462ff 100644
@@ -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])

liblttng-ust/Makefile.am +1 -0

index f1801cf..05929be 100644
@@ -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

liblttng-ust/compat.h +21 -1

index 43b2223..4fee919 100644
@@ -3,6 +3,7 @@
 
 /*
  * Copyright (C) 2011 Mathieu Desnoyers <[email protected]>
+ * Copyright (C) 2016 Raphaël Beamonte <[email protected]>
  *
  * 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.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 91

Quoted 10 lines

liblttng-ust/compat.h +10 -0

+ */
+#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.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 92

Quoted 1 line

liblttng-ust/compat.h +1 -0

+   int limit = LTTNG_UST_PROCNAME_LEN - 4;

Then, limit is 10 - 4 = 6.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 93

Quoted 1 line

liblttng-ust/compat.h +1 -0

+   int len = 0;

This can be moved on the strlen(name) line.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 95

Quoted 2 lines

liblttng-ust/compat.h +2 -0

+
+   lttng_ust_getprocname(name);

Let's suppose that after the call to lttng_ust_getprocname(), name contains "012345678\0",

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 97

Quoted 2 lines

liblttng-ust/compat.h +2 -0

+
+   len = strlen(name);

The variable "len" will contain the integer 9.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 101

Quoted 4 lines

liblttng-ust/compat.h +4 -0

+   if (len >= limit) {
+           len = limit;
+   }
+

After that, len is >= limit (9 >= 6), so len becomes 6.

liblttng-ust/compat.h void lttng_ust_getprocname(char *name) · line 102

Quoted 1 line

liblttng-ust/compat.h +1 -0

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

Quoted 33 lines

Quoted 33 lines

Quoted 33 lines

Quoted 15 lines

liblttng-ust/compat.h +5 -0

+
+   PTHREAD_SETNAME_NP(pthread_self(), name);
+}
+#else
+#define LTTNG_UST_SETUSTPROCNAME
 #endif
 
 #include <errno.h>

liblttng-ust/lttng-ust-comm.c +0 -0

index e00a22c..4de68c7 100644

liblttng-ust/lttng-ust-comm.c void *ust_listener_thread(void *arg) · line 1302

Quoted 8 lines

liblttng-ust/lttng-ust-comm.c +5 -0

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

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

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

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 <[email protected]>

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

configure.ac +5 -0

index 5692553..de462ff 100644
@@ -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])

liblttng-ust/Makefile.am +1 -0

index f1801cf..05929be 100644
@@ -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

liblttng-ust/compat.h +50 -1

index 43b2223..68d4d50 100644
@@ -3,6 +3,7 @@
 
 /*
  * Copyright (C) 2011 Mathieu Desnoyers <[email protected]>
+ * Copyright (C) 2016 Raphaël Beamonte <[email protected]>
  *
  * 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>

liblttng-ust/lttng-ust-comm.c +0 -0

index e00a22c..c32e8ae 100644
@@ -1295,6 +1295,12 @@ void *ust_listener_thread(void *arg)
 	int sock, ret, prev_connect_failed = 0, has_waited = 0;
 	long timeout;

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 <[email protected]>

---

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

configure.ac +5 -0

index 5692553..de462ff 100644
@@ -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])

liblttng-ust/Makefile.am +1 -0

index f1801cf..05929be 100644
@@ -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"

liblttng-ust/Makefile.am +0 -1

-fno-strict-aliasing

liblttng-ust/compat.h +1 -1

index 43b2223..68d4d50 100644
@@ -3,6 +3,7 @@
 
 /*
 * Copyright (C) 2011 Mathieu Desnoyers <[email protected]>
+ * Copyright (C) 2016 Raphaël Beamonte <[email protected]>
 *
 * 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

liblttng-ust/compat.h +2 -0

+#endif
+
 #elif defined(__FreeBSD__)
 #include <stdlib.h>
 #include <string.h>

liblttng-ust/compat.h +2 -0

+{
+}
 #endif
 
 #include <errno.h>

liblttng-ust/lttng-ust-comm.c +0 -0

index e00a22c..c32e8ae 100644

liblttng-ust/lttng-ust-comm.c +1 -0

+
 	/* Restart trying to connect to the session daemon */

restart:

	if (prev_connect_failed) {

--
2.8.1

_______________________________________________
lttng-dev mailing list
[email protected]
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev