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

[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

[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

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

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

[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

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: [RFC PATCH 0/2] Identify UST threads by changing thread names

[Sebastien Boisvert]

Hi Raphaël,

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

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

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

I prefer this one. It is less complex, in my opinion.

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

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.

> 
> 
> I'll wait for your comments and ideas on that subject.
> 
> Thanks,
> Raphaël
> 
> 
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Raphaël Beamonte]

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