Message ID | 80ee3399800d43b5d3f51aa095ebca160d4f20b9.1464966729.git.raphael.beamonte@gmail.com |
---|---|
State | Superseded, archived |
Headers |
From: raphael.beamonte at gmail.com (=?UTF-8?q?Rapha=C3=ABl=20Beamonte?=) Date: Fri, 3 Jun 2016 11:17:00 -0400 Subject: [lttng-dev] [lttng-ust RFC PATCH v2 1/1] Add -ust to the name of UST threads of the application In-Reply-To: <cover.1464966729.git.raphael.beamonte@gmail.com> References: <cover.1464966729.git.raphael.beamonte@gmail.com> Message-ID: <80ee3399800d43b5d3f51aa095ebca160d4f20b9.1464966729.git.raphael.beamonte@gmail.com> |
Commit Message
Raphaël Beamonte
June 3, 2016, 3:17 p.m. UTC
Add the required functions to change the thread name of the UST
threads and add the -ust string at its end. This will help to
identify LTTng-UST processes when analyzing the trace of a process.
Signed-off-by: Raphaël Beamonte <raphael.beamonte at gmail.com>
---
configure.ac | 5 +++++
liblttng-ust/Makefile.am | 1 +
liblttng-ust/compat.h | 48 ++++++++++++++++++++++++++++++++++++++++++-
liblttng-ust/lttng-ust-comm.c | 6 ++++++
4 files changed, 59 insertions(+), 1 deletion(-)
Comments
Hi Raphaël, I think you have a buffer overflow, see below. On 06/03/2016 11:17 AM, Raphaël Beamonte wrote: > Add the required functions to change the thread name of the UST > threads and add the -ust string at its end. This will help to > identify LTTng-UST processes when analyzing the trace of a process. > > Signed-off-by: Raphaël Beamonte <raphael.beamonte at gmail.com> > --- > configure.ac | 5 +++++ > liblttng-ust/Makefile.am | 1 + > liblttng-ust/compat.h | 48 ++++++++++++++++++++++++++++++++++++++++++- > liblttng-ust/lttng-ust-comm.c | 6 ++++++ > 4 files changed, 59 insertions(+), 1 deletion(-) > > diff --git a/configure.ac b/configure.ac > index 5692553..de462ff 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -135,6 +135,11 @@ AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBDL], [test "x$have_libdl" = "xyes"]) > AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBC_DL], [test "x$have_libc_dl" = "xyes"]) > > AC_CHECK_LIB([pthread], [pthread_create]) > +AC_CHECK_LIB([pthread], [pthread_setname_np], > + AC_DEFINE([HAVE_PTHREAD_SETNAME_NP], [1], [Define to 1 if pthread_setname_np is available.]), > + AC_CHECK_LIB([pthread], [pthread_set_name_np], > + AC_DEFINE([HAVE_PTHREAD_SET_NAME_NP], [1], [Define to 1 if pthread_set_name_np is available.]), > + AC_MSG_RESULT([pthread setname/set_name not found.]))) > > # Check for dlfcn.h > AC_CHECK_HEADER([dlfcn.h]) > diff --git a/liblttng-ust/Makefile.am b/liblttng-ust/Makefile.am > index f1801cf..05929be 100644 > --- a/liblttng-ust/Makefile.am > +++ b/liblttng-ust/Makefile.am > @@ -14,6 +14,7 @@ liblttng_ust_tracepoint_la_SOURCES = \ > error.h > liblttng_ust_tracepoint_la_LIBADD = \ > -lurcu-bp \ > + -lpthread \ > $(top_builddir)/snprintf/libustsnprintf.la > liblttng_ust_tracepoint_la_LDFLAGS = -no-undefined -version-info $(LTTNG_UST_LIBRARY_VERSION) > liblttng_ust_tracepoint_la_CFLAGS = -DUST_COMPONENT="liblttng_ust_tracepoint" -fno-strict-aliasing > diff --git a/liblttng-ust/compat.h b/liblttng-ust/compat.h > index 43b2223..4fee919 100644 > --- a/liblttng-ust/compat.h > +++ b/liblttng-ust/compat.h > @@ -3,6 +3,7 @@ > > /* > * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers at efficios.com> > + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte at gmail.com> > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Lesser General Public > @@ -23,7 +24,6 @@ > * lttng_ust_getprocname. > */ > #ifdef __linux__ > - > #include <sys/prctl.h> > > #define LTTNG_UST_PROCNAME_LEN 17 > @@ -34,6 +34,13 @@ void lttng_ust_getprocname(char *name) > (void) prctl(PR_GET_NAME, (unsigned long) name, 0, 0, 0); > } > > +/* > + * if pthread_setname_np is available. > + */ > +#ifdef HAVE_PTHREAD_SETNAME_NP > +#define PTHREAD_SETNAME_NP pthread_setname_np > +#endif > + > #elif defined(__FreeBSD__) > #include <stdlib.h> > #include <string.h> > @@ -59,6 +66,45 @@ void lttng_ust_getprocname(char *name) > strncpy(name, bsd_name, LTTNG_UST_PROCNAME_LEN - 1); > } > > +/* > + * if pthread_set_name_np is available. > + */ > +#ifdef HAVE_PTHREAD_SET_NAME_NP > +#define PTHREAD_SETNAME_NP pthread_set_name_np > +#endif > + > +#endif > + > +/* > + * if a pthread setname/set_name function is available, declare > + * the setustprocname() function that will add '-ust' to the end > + * of the current process name, while truncating it if needed. > + */ > +#ifdef PTHREAD_SETNAME_NP > +#define LTTNG_UST_SETUSTPROCNAME lttng_ust_setustprocname() > + > +#include <pthread.h> > + > +static inline > +void lttng_ust_setustprocname() > +{ > + char name[LTTNG_UST_PROCNAME_LEN]; Let's say that LTTNG_UST_PROCNAME_LEN is 10. > + int limit = LTTNG_UST_PROCNAME_LEN - 4; Then, limit is 10 - 4 = 6. > + int len = 0; This can be moved on the strlen(name) line. > + > + lttng_ust_getprocname(name); Let's suppose that after the call to lttng_ust_getprocname(), name contains "012345678\0", > + > + len = strlen(name); The variable "len" will contain the integer 9. > + if (len >= limit) { > + len = limit; > + } > + After that, len is >= limit (9 >= 6), so len becomes 6. > + sprintf(name + len, "%s", "-ust"); name is a pointer to "012345678\0" (9 bytes + '\0') name + len is name + 6, so it points to "678\0" sprintf will essentially copy "-ust\0" to "678\0". So, if my calculations are not wrong, you have a buffer overflow which will corrupt the stack memory of the current stack frame. > + > + PTHREAD_SETNAME_NP(pthread_self(), name); > +} > +#else > +#define LTTNG_UST_SETUSTPROCNAME > #endif > > #include <errno.h> > diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c > index e00a22c..4de68c7 100644 > --- a/liblttng-ust/lttng-ust-comm.c > +++ b/liblttng-ust/lttng-ust-comm.c > @@ -1295,6 +1295,12 @@ void *ust_listener_thread(void *arg) > int sock, ret, prev_connect_failed = 0, has_waited = 0; > long timeout; > > + /* > + * If available, add '-ust' to the end of this thread's > + * process name > + */ > + LTTNG_UST_SETUSTPROCNAME; Is this common practice ? Just by looking at this line, it is unclear that it is a function call. I would prefer LTTNG_UST_SETUSTPROCNAME() or lttng_ust_setustprocname() > + > /* Restart trying to connect to the session daemon */ > restart: > if (prev_connect_failed) { >
----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert sboisvert at gydle.com wrote: > Hi Raphaël, > > I think you have a buffer overflow, see below. > > On 06/03/2016 11:17 AM, Raphaël Beamonte wrote: >> Add the required functions to change the thread name of the UST >> threads and add the -ust string at its end. This will help to >> identify LTTng-UST processes when analyzing the trace of a process. >> >> Signed-off-by: Raphaël Beamonte <raphael.beamonte at gmail.com> >> --- >> configure.ac | 5 +++++ >> liblttng-ust/Makefile.am | 1 + >> liblttng-ust/compat.h | 48 ++++++++++++++++++++++++++++++++++++++++++- >> liblttng-ust/lttng-ust-comm.c | 6 ++++++ >> 4 files changed, 59 insertions(+), 1 deletion(-) >> >> diff --git a/configure.ac b/configure.ac >> index 5692553..de462ff 100644 >> --- a/configure.ac >> +++ b/configure.ac >> @@ -135,6 +135,11 @@ AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBDL], [test >> "x$have_libdl" = "xyes"]) >> AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBC_DL], [test "x$have_libc_dl" = "xyes"]) >> >> AC_CHECK_LIB([pthread], [pthread_create]) >> +AC_CHECK_LIB([pthread], [pthread_setname_np], >> + AC_DEFINE([HAVE_PTHREAD_SETNAME_NP], [1], [Define to 1 if pthread_setname_np >> is available.]), >> + AC_CHECK_LIB([pthread], [pthread_set_name_np], >> + AC_DEFINE([HAVE_PTHREAD_SET_NAME_NP], [1], [Define to 1 if >> pthread_set_name_np is available.]), >> + AC_MSG_RESULT([pthread setname/set_name not found.]))) >> >> # Check for dlfcn.h >> AC_CHECK_HEADER([dlfcn.h]) >> diff --git a/liblttng-ust/Makefile.am b/liblttng-ust/Makefile.am >> index f1801cf..05929be 100644 >> --- a/liblttng-ust/Makefile.am >> +++ b/liblttng-ust/Makefile.am >> @@ -14,6 +14,7 @@ liblttng_ust_tracepoint_la_SOURCES = \ >> error.h >> liblttng_ust_tracepoint_la_LIBADD = \ >> -lurcu-bp \ >> + -lpthread \ >> $(top_builddir)/snprintf/libustsnprintf.la >> liblttng_ust_tracepoint_la_LDFLAGS = -no-undefined -version-info >> $(LTTNG_UST_LIBRARY_VERSION) >> liblttng_ust_tracepoint_la_CFLAGS = -DUST_COMPONENT="liblttng_ust_tracepoint" >> -fno-strict-aliasing >> diff --git a/liblttng-ust/compat.h b/liblttng-ust/compat.h >> index 43b2223..4fee919 100644 >> --- a/liblttng-ust/compat.h >> +++ b/liblttng-ust/compat.h >> @@ -3,6 +3,7 @@ >> >> /* >> * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers at efficios.com> >> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte at gmail.com> >> * >> * This library is free software; you can redistribute it and/or >> * modify it under the terms of the GNU Lesser General Public >> @@ -23,7 +24,6 @@ >> * lttng_ust_getprocname. >> */ >> #ifdef __linux__ >> - >> #include <sys/prctl.h> >> >> #define LTTNG_UST_PROCNAME_LEN 17 >> @@ -34,6 +34,13 @@ void lttng_ust_getprocname(char *name) >> (void) prctl(PR_GET_NAME, (unsigned long) name, 0, 0, 0); >> } >> >> +/* >> + * if pthread_setname_np is available. >> + */ >> +#ifdef HAVE_PTHREAD_SETNAME_NP >> +#define PTHREAD_SETNAME_NP pthread_setname_np >> +#endif >> + >> #elif defined(__FreeBSD__) >> #include <stdlib.h> >> #include <string.h> >> @@ -59,6 +66,45 @@ void lttng_ust_getprocname(char *name) >> strncpy(name, bsd_name, LTTNG_UST_PROCNAME_LEN - 1); >> } >> >> +/* >> + * if pthread_set_name_np is available. >> + */ >> +#ifdef HAVE_PTHREAD_SET_NAME_NP >> +#define PTHREAD_SETNAME_NP pthread_set_name_np >> +#endif >> + >> +#endif >> + >> +/* >> + * if a pthread setname/set_name function is available, declare >> + * the setustprocname() function that will add '-ust' to the end >> + * of the current process name, while truncating it if needed. >> + */ >> +#ifdef PTHREAD_SETNAME_NP >> +#define LTTNG_UST_SETUSTPROCNAME lttng_ust_setustprocname() >> + >> +#include <pthread.h> >> + >> +static inline >> +void lttng_ust_setustprocname() >> +{ >> + char name[LTTNG_UST_PROCNAME_LEN]; > > Let's say that LTTNG_UST_PROCNAME_LEN is 10. > >> + int limit = LTTNG_UST_PROCNAME_LEN - 4; > > Then, limit is 10 - 4 = 6. > >> + int len = 0; > > This can be moved on the strlen(name) line. > >> + >> + lttng_ust_getprocname(name); > > Let's suppose that after the call to lttng_ust_getprocname(), name contains > "012345678\0", > >> + >> + len = strlen(name); > > The variable "len" will contain the integer 9. > >> + if (len >= limit) { >> + len = limit; >> + } >> + > > After that, len is >= limit (9 >= 6), so len becomes 6. > >> + sprintf(name + len, "%s", "-ust"); > > name is a pointer to "012345678\0" (9 bytes + '\0') > > name + len is name + 6, so it points to "678\0" > > sprintf will essentially copy "-ust\0" to "678\0". > > So, if my calculations are not wrong, you have a buffer overflow which will > corrupt the stack memory of the > current stack frame. > > >> + >> + PTHREAD_SETNAME_NP(pthread_self(), name); >> +} >> +#else >> +#define LTTNG_UST_SETUSTPROCNAME >> #endif >> >> #include <errno.h> >> diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c >> index e00a22c..4de68c7 100644 >> --- a/liblttng-ust/lttng-ust-comm.c >> +++ b/liblttng-ust/lttng-ust-comm.c >> @@ -1295,6 +1295,12 @@ void *ust_listener_thread(void *arg) >> int sock, ret, prev_connect_failed = 0, has_waited = 0; >> long timeout; >> >> + /* >> + * If available, add '-ust' to the end of this thread's >> + * process name >> + */ >> + LTTNG_UST_SETUSTPROCNAME; > > Is this common practice ? Just by looking at this line, it is unclear that it is > a function call. > > I would prefer > > LTTNG_UST_SETUSTPROCNAME() > > or > > lttng_ust_setustprocname() The latter is preferred by our coding style, indeed. Thanks, Mathieu > > > >> + >> /* Restart trying to connect to the session daemon */ >> restart: >> if (prev_connect_failed) {
----- On Jun 3, 2016, at 5:54 PM, Mathieu Desnoyers mathieu.desnoyers at efficios.com wrote: > ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert sboisvert at gydle.com wrote: > >> Hi Raphaël, >> >> I think you have a buffer overflow, see below. >> >> On 06/03/2016 11:17 AM, Raphaël Beamonte wrote: >>> Add the required functions to change the thread name of the UST >>> threads and add the -ust string at its end. This will help to >>> identify LTTng-UST processes when analyzing the trace of a process. >>> >>> Signed-off-by: Raphaël Beamonte <raphael.beamonte at gmail.com> >>> --- >>> configure.ac | 5 +++++ >>> liblttng-ust/Makefile.am | 1 + >>> liblttng-ust/compat.h | 48 ++++++++++++++++++++++++++++++++++++++++++- >>> liblttng-ust/lttng-ust-comm.c | 6 ++++++ >>> 4 files changed, 59 insertions(+), 1 deletion(-) >>> >>> diff --git a/configure.ac b/configure.ac >>> index 5692553..de462ff 100644 >>> --- a/configure.ac >>> +++ b/configure.ac >>> @@ -135,6 +135,11 @@ AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBDL], [test >>> "x$have_libdl" = "xyes"]) >>> AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBC_DL], [test "x$have_libc_dl" = "xyes"]) >>> >>> AC_CHECK_LIB([pthread], [pthread_create]) >>> +AC_CHECK_LIB([pthread], [pthread_setname_np], >>> + AC_DEFINE([HAVE_PTHREAD_SETNAME_NP], [1], [Define to 1 if pthread_setname_np >>> is available.]), >>> + AC_CHECK_LIB([pthread], [pthread_set_name_np], >>> + AC_DEFINE([HAVE_PTHREAD_SET_NAME_NP], [1], [Define to 1 if >>> pthread_set_name_np is available.]), >>> + AC_MSG_RESULT([pthread setname/set_name not found.]))) >>> >>> # Check for dlfcn.h >>> AC_CHECK_HEADER([dlfcn.h]) >>> diff --git a/liblttng-ust/Makefile.am b/liblttng-ust/Makefile.am >>> index f1801cf..05929be 100644 >>> --- a/liblttng-ust/Makefile.am >>> +++ b/liblttng-ust/Makefile.am >>> @@ -14,6 +14,7 @@ liblttng_ust_tracepoint_la_SOURCES = \ >>> error.h >>> liblttng_ust_tracepoint_la_LIBADD = \ >>> -lurcu-bp \ >>> + -lpthread \ >>> $(top_builddir)/snprintf/libustsnprintf.la >>> liblttng_ust_tracepoint_la_LDFLAGS = -no-undefined -version-info >>> $(LTTNG_UST_LIBRARY_VERSION) >>> liblttng_ust_tracepoint_la_CFLAGS = -DUST_COMPONENT="liblttng_ust_tracepoint" >>> -fno-strict-aliasing >>> diff --git a/liblttng-ust/compat.h b/liblttng-ust/compat.h >>> index 43b2223..4fee919 100644 >>> --- a/liblttng-ust/compat.h >>> +++ b/liblttng-ust/compat.h >>> @@ -3,6 +3,7 @@ >>> >>> /* >>> * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers at efficios.com> >>> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte at gmail.com> >>> * >>> * This library is free software; you can redistribute it and/or >>> * modify it under the terms of the GNU Lesser General Public >>> @@ -23,7 +24,6 @@ >>> * lttng_ust_getprocname. >>> */ >>> #ifdef __linux__ >>> - >>> #include <sys/prctl.h> >>> >>> #define LTTNG_UST_PROCNAME_LEN 17 >>> @@ -34,6 +34,13 @@ void lttng_ust_getprocname(char *name) >>> (void) prctl(PR_GET_NAME, (unsigned long) name, 0, 0, 0); >>> } >>> >>> +/* >>> + * if pthread_setname_np is available. >>> + */ >>> +#ifdef HAVE_PTHREAD_SETNAME_NP >>> +#define PTHREAD_SETNAME_NP pthread_setname_np >>> +#endif >>> + >>> #elif defined(__FreeBSD__) >>> #include <stdlib.h> >>> #include <string.h> >>> @@ -59,6 +66,45 @@ void lttng_ust_getprocname(char *name) >>> strncpy(name, bsd_name, LTTNG_UST_PROCNAME_LEN - 1); >>> } >>> >>> +/* >>> + * if pthread_set_name_np is available. >>> + */ >>> +#ifdef HAVE_PTHREAD_SET_NAME_NP >>> +#define PTHREAD_SETNAME_NP pthread_set_name_np >>> +#endif >>> + >>> +#endif >>> + >>> +/* >>> + * if a pthread setname/set_name function is available, declare >>> + * the setustprocname() function that will add '-ust' to the end >>> + * of the current process name, while truncating it if needed. >>> + */ >>> +#ifdef PTHREAD_SETNAME_NP >>> +#define LTTNG_UST_SETUSTPROCNAME lttng_ust_setustprocname() >>> + >>> +#include <pthread.h> >>> + >>> +static inline >>> +void lttng_ust_setustprocname() >>> +{ >>> + char name[LTTNG_UST_PROCNAME_LEN]; >> >> Let's say that LTTNG_UST_PROCNAME_LEN is 10. >> >>> + int limit = LTTNG_UST_PROCNAME_LEN - 4; >> >> Then, limit is 10 - 4 = 6. >> >>> + int len = 0; >> >> This can be moved on the strlen(name) line. >> >>> + >>> + lttng_ust_getprocname(name); >> >> Let's suppose that after the call to lttng_ust_getprocname(), name contains >> "012345678\0", >> >>> + >>> + len = strlen(name); >> >> The variable "len" will contain the integer 9. >> >>> + if (len >= limit) { >>> + len = limit; >>> + } >>> + >> >> After that, len is >= limit (9 >= 6), so len becomes 6. >> >>> + sprintf(name + len, "%s", "-ust"); >> >> name is a pointer to "012345678\0" (9 bytes + '\0') >> >> name + len is name + 6, so it points to "678\0" >> >> sprintf will essentially copy "-ust\0" to "678\0". >> >> So, if my calculations are not wrong, you have a buffer overflow which will >> corrupt the stack memory of the >> current stack frame. >> >> >>> + >>> + PTHREAD_SETNAME_NP(pthread_self(), name); >>> +} >>> +#else >>> +#define LTTNG_UST_SETUSTPROCNAME >>> #endif >>> >>> #include <errno.h> >>> diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c >>> index e00a22c..4de68c7 100644 >>> --- a/liblttng-ust/lttng-ust-comm.c >>> +++ b/liblttng-ust/lttng-ust-comm.c >>> @@ -1295,6 +1295,12 @@ void *ust_listener_thread(void *arg) >>> int sock, ret, prev_connect_failed = 0, has_waited = 0; >>> long timeout; >>> >>> + /* >>> + * If available, add '-ust' to the end of this thread's >>> + * process name >>> + */ >>> + LTTNG_UST_SETUSTPROCNAME; >> >> Is this common practice ? Just by looking at this line, it is unclear that it is >> a function call. >> >> I would prefer >> >> LTTNG_UST_SETUSTPROCNAME() >> >> or >> >> lttng_ust_setustprocname() > > The latter is preferred by our coding style, indeed. > And there is no reason to make it a macro. It should be a static inline whenever possible. Thanks, Mathieu > Thanks, > > Mathieu > >> >> >> >>> + >>> /* Restart trying to connect to the session daemon */ >>> restart: >>> if (prev_connect_failed) { > > -- > Mathieu Desnoyers > EfficiOS Inc. > http://www.efficios.com
2016-06-03 11:55 GMT-04:00 Mathieu Desnoyers <mathieu.desnoyers at efficios.com>: > ----- On Jun 3, 2016, at 5:54 PM, Mathieu Desnoyers mathieu.desnoyers at efficios.com wrote: > >> ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert sboisvert at gydle.com wrote: >> >>> Hi Raphaël, >>> >>> I think you have a buffer overflow, see below. >>> >>> On 06/03/2016 11:17 AM, Raphaël Beamonte wrote: >>>> Add the required functions to change the thread name of the UST >>>> threads and add the -ust string at its end. This will help to >>>> identify LTTng-UST processes when analyzing the trace of a process. >>>> >>>> Signed-off-by: Raphaël Beamonte <raphael.beamonte at gmail.com> >>>> --- >>>> configure.ac | 5 +++++ >>>> liblttng-ust/Makefile.am | 1 + >>>> liblttng-ust/compat.h | 48 ++++++++++++++++++++++++++++++++++++++++++- >>>> liblttng-ust/lttng-ust-comm.c | 6 ++++++ >>>> 4 files changed, 59 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/configure.ac b/configure.ac >>>> index 5692553..de462ff 100644 >>>> --- a/configure.ac >>>> +++ b/configure.ac >>>> @@ -135,6 +135,11 @@ AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBDL], [test >>>> "x$have_libdl" = "xyes"]) >>>> AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBC_DL], [test "x$have_libc_dl" = "xyes"]) >>>> >>>> AC_CHECK_LIB([pthread], [pthread_create]) >>>> +AC_CHECK_LIB([pthread], [pthread_setname_np], >>>> + AC_DEFINE([HAVE_PTHREAD_SETNAME_NP], [1], [Define to 1 if pthread_setname_np >>>> is available.]), >>>> + AC_CHECK_LIB([pthread], [pthread_set_name_np], >>>> + AC_DEFINE([HAVE_PTHREAD_SET_NAME_NP], [1], [Define to 1 if >>>> pthread_set_name_np is available.]), >>>> + AC_MSG_RESULT([pthread setname/set_name not found.]))) >>>> >>>> # Check for dlfcn.h >>>> AC_CHECK_HEADER([dlfcn.h]) >>>> diff --git a/liblttng-ust/Makefile.am b/liblttng-ust/Makefile.am >>>> index f1801cf..05929be 100644 >>>> --- a/liblttng-ust/Makefile.am >>>> +++ b/liblttng-ust/Makefile.am >>>> @@ -14,6 +14,7 @@ liblttng_ust_tracepoint_la_SOURCES = \ >>>> error.h >>>> liblttng_ust_tracepoint_la_LIBADD = \ >>>> -lurcu-bp \ >>>> + -lpthread \ >>>> $(top_builddir)/snprintf/libustsnprintf.la >>>> liblttng_ust_tracepoint_la_LDFLAGS = -no-undefined -version-info >>>> $(LTTNG_UST_LIBRARY_VERSION) >>>> liblttng_ust_tracepoint_la_CFLAGS = -DUST_COMPONENT="liblttng_ust_tracepoint" >>>> -fno-strict-aliasing >>>> diff --git a/liblttng-ust/compat.h b/liblttng-ust/compat.h >>>> index 43b2223..4fee919 100644 >>>> --- a/liblttng-ust/compat.h >>>> +++ b/liblttng-ust/compat.h >>>> @@ -3,6 +3,7 @@ >>>> >>>> /* >>>> * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers at efficios.com> >>>> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte at gmail.com> >>>> * >>>> * This library is free software; you can redistribute it and/or >>>> * modify it under the terms of the GNU Lesser General Public >>>> @@ -23,7 +24,6 @@ >>>> * lttng_ust_getprocname. >>>> */ >>>> #ifdef __linux__ >>>> - >>>> #include <sys/prctl.h> >>>> >>>> #define LTTNG_UST_PROCNAME_LEN 17 >>>> @@ -34,6 +34,13 @@ void lttng_ust_getprocname(char *name) >>>> (void) prctl(PR_GET_NAME, (unsigned long) name, 0, 0, 0); >>>> } >>>> >>>> +/* >>>> + * if pthread_setname_np is available. >>>> + */ >>>> +#ifdef HAVE_PTHREAD_SETNAME_NP >>>> +#define PTHREAD_SETNAME_NP pthread_setname_np >>>> +#endif >>>> + >>>> #elif defined(__FreeBSD__) >>>> #include <stdlib.h> >>>> #include <string.h> >>>> @@ -59,6 +66,45 @@ void lttng_ust_getprocname(char *name) >>>> strncpy(name, bsd_name, LTTNG_UST_PROCNAME_LEN - 1); >>>> } >>>> >>>> +/* >>>> + * if pthread_set_name_np is available. >>>> + */ >>>> +#ifdef HAVE_PTHREAD_SET_NAME_NP >>>> +#define PTHREAD_SETNAME_NP pthread_set_name_np >>>> +#endif >>>> + >>>> +#endif >>>> + >>>> +/* >>>> + * if a pthread setname/set_name function is available, declare >>>> + * the setustprocname() function that will add '-ust' to the end >>>> + * of the current process name, while truncating it if needed. >>>> + */ >>>> +#ifdef PTHREAD_SETNAME_NP >>>> +#define LTTNG_UST_SETUSTPROCNAME lttng_ust_setustprocname() >>>> + >>>> +#include <pthread.h> >>>> + >>>> +static inline >>>> +void lttng_ust_setustprocname() >>>> +{ >>>> + char name[LTTNG_UST_PROCNAME_LEN]; >>> >>> Let's say that LTTNG_UST_PROCNAME_LEN is 10. >>> >>>> + int limit = LTTNG_UST_PROCNAME_LEN - 4; >>> >>> Then, limit is 10 - 4 = 6. >>> >>>> + int len = 0; >>> >>> This can be moved on the strlen(name) line. >>> >>>> + >>>> + lttng_ust_getprocname(name); >>> >>> Let's suppose that after the call to lttng_ust_getprocname(), name contains >>> "012345678\0", >>> >>>> + >>>> + len = strlen(name); >>> >>> The variable "len" will contain the integer 9. >>> >>>> + if (len >= limit) { >>>> + len = limit; >>>> + } >>>> + >>> >>> After that, len is >= limit (9 >= 6), so len becomes 6. >>> >>>> + sprintf(name + len, "%s", "-ust"); >>> >>> name is a pointer to "012345678\0" (9 bytes + '\0') >>> >>> name + len is name + 6, so it points to "678\0" >>> >>> sprintf will essentially copy "-ust\0" to "678\0". >>> >>> So, if my calculations are not wrong, you have a buffer overflow which will >>> corrupt the stack memory of the >>> current stack frame. You're right, will fix that. I'm also questionning the interest of limit being a variable here. Should I replace both occurrences directly by "LTTNG_UST_PROCNAME_LEN - 5" or create a "#define LTTNG_UST_PROCNAME_LIMIT (LTTNG_UST_PROCNAME_LEN - 5)" macro ? Mathieu, what do you think would be the best to do here ? The condition will also be replaced from ">=" to ">", as the "=" part is not useful anymore in that version of the patch (it was in v1, because of the operations that were done). >>> >>> >>>> + >>>> + PTHREAD_SETNAME_NP(pthread_self(), name); >>>> +} >>>> +#else >>>> +#define LTTNG_UST_SETUSTPROCNAME >>>> #endif >>>> >>>> #include <errno.h> >>>> diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c >>>> index e00a22c..4de68c7 100644 >>>> --- a/liblttng-ust/lttng-ust-comm.c >>>> +++ b/liblttng-ust/lttng-ust-comm.c >>>> @@ -1295,6 +1295,12 @@ void *ust_listener_thread(void *arg) >>>> int sock, ret, prev_connect_failed = 0, has_waited = 0; >>>> long timeout; >>>> >>>> + /* >>>> + * If available, add '-ust' to the end of this thread's >>>> + * process name >>>> + */ >>>> + LTTNG_UST_SETUSTPROCNAME; >>> >>> Is this common practice ? Just by looking at this line, it is unclear that it is >>> a function call. >>> >>> I would prefer >>> >>> LTTNG_UST_SETUSTPROCNAME() >>> >>> or >>> >>> lttng_ust_setustprocname() >> >> The latter is preferred by our coding style, indeed. >> > > And there is no reason to make it a macro. It should be > a static inline whenever possible. I made it a macro as I was not sure about the best path to take, and the macro seemed to be the cleaner approach! However, would it be better like that: #ifdef PTHREAD_SETNAME_NP #include <pthread.h> #endif static inline void lttng_ust_setustprocname() { #ifdef PTHREAD_SETNAME_NP ... PTHREAD_SETNAME_NP(pthread_self(), name); #endif } Or like that ? #ifdef PTHREAD_SETNAME_NP #include <pthread.h> static inline void lttng_ust_setustprocname() { ... PTHREAD_SETNAME_NP(pthread_self(), name); } #else static inline void lttng_ust_setustprocname() {} #endif In both those cases, the macro disappears, and is replaced by a call to lttng_ust_setustprocname(). I'm not sure however which is preferable according to the coding style (In one case, two #ifdef are required, in the other, we declare the function in each branch of the ifdef/else). Thoughts ? I'll send a v3 of the patch according to your preference in both matters. Thanks a lot! Raphaël
----- On Jun 3, 2016, at 6:12 PM, Raphaël Beamonte raphael.beamonte at gmail.com wrote: > 2016-06-03 11:55 GMT-04:00 Mathieu Desnoyers <mathieu.desnoyers at efficios.com>: >> ----- On Jun 3, 2016, at 5:54 PM, Mathieu Desnoyers >> mathieu.desnoyers at efficios.com wrote: >> >>> ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert sboisvert at gydle.com wrote: >>> >>>> Hi Raphaël, >>>> >>>> I think you have a buffer overflow, see below. >>>> >>>> On 06/03/2016 11:17 AM, Raphaël Beamonte wrote: >>>>> Add the required functions to change the thread name of the UST >>>>> threads and add the -ust string at its end. This will help to >>>>> identify LTTng-UST processes when analyzing the trace of a process. >>>>> >>>>> Signed-off-by: Raphaël Beamonte <raphael.beamonte at gmail.com> >>>>> --- >>>>> configure.ac | 5 +++++ >>>>> liblttng-ust/Makefile.am | 1 + >>>>> liblttng-ust/compat.h | 48 ++++++++++++++++++++++++++++++++++++++++++- >>>>> liblttng-ust/lttng-ust-comm.c | 6 ++++++ >>>>> 4 files changed, 59 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/configure.ac b/configure.ac >>>>> index 5692553..de462ff 100644 >>>>> --- a/configure.ac >>>>> +++ b/configure.ac >>>>> @@ -135,6 +135,11 @@ AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBDL], [test >>>>> "x$have_libdl" = "xyes"]) >>>>> AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBC_DL], [test "x$have_libc_dl" = "xyes"]) >>>>> >>>>> AC_CHECK_LIB([pthread], [pthread_create]) >>>>> +AC_CHECK_LIB([pthread], [pthread_setname_np], >>>>> + AC_DEFINE([HAVE_PTHREAD_SETNAME_NP], [1], [Define to 1 if pthread_setname_np >>>>> is available.]), >>>>> + AC_CHECK_LIB([pthread], [pthread_set_name_np], >>>>> + AC_DEFINE([HAVE_PTHREAD_SET_NAME_NP], [1], [Define to 1 if >>>>> pthread_set_name_np is available.]), >>>>> + AC_MSG_RESULT([pthread setname/set_name not found.]))) >>>>> >>>>> # Check for dlfcn.h >>>>> AC_CHECK_HEADER([dlfcn.h]) >>>>> diff --git a/liblttng-ust/Makefile.am b/liblttng-ust/Makefile.am >>>>> index f1801cf..05929be 100644 >>>>> --- a/liblttng-ust/Makefile.am >>>>> +++ b/liblttng-ust/Makefile.am >>>>> @@ -14,6 +14,7 @@ liblttng_ust_tracepoint_la_SOURCES = \ >>>>> error.h >>>>> liblttng_ust_tracepoint_la_LIBADD = \ >>>>> -lurcu-bp \ >>>>> + -lpthread \ >>>>> $(top_builddir)/snprintf/libustsnprintf.la >>>>> liblttng_ust_tracepoint_la_LDFLAGS = -no-undefined -version-info >>>>> $(LTTNG_UST_LIBRARY_VERSION) >>>>> liblttng_ust_tracepoint_la_CFLAGS = -DUST_COMPONENT="liblttng_ust_tracepoint" >>>>> -fno-strict-aliasing >>>>> diff --git a/liblttng-ust/compat.h b/liblttng-ust/compat.h >>>>> index 43b2223..4fee919 100644 >>>>> --- a/liblttng-ust/compat.h >>>>> +++ b/liblttng-ust/compat.h >>>>> @@ -3,6 +3,7 @@ >>>>> >>>>> /* >>>>> * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers at efficios.com> >>>>> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte at gmail.com> >>>>> * >>>>> * This library is free software; you can redistribute it and/or >>>>> * modify it under the terms of the GNU Lesser General Public >>>>> @@ -23,7 +24,6 @@ >>>>> * lttng_ust_getprocname. >>>>> */ >>>>> #ifdef __linux__ >>>>> - >>>>> #include <sys/prctl.h> >>>>> >>>>> #define LTTNG_UST_PROCNAME_LEN 17 >>>>> @@ -34,6 +34,13 @@ void lttng_ust_getprocname(char *name) >>>>> (void) prctl(PR_GET_NAME, (unsigned long) name, 0, 0, 0); >>>>> } >>>>> >>>>> +/* >>>>> + * if pthread_setname_np is available. >>>>> + */ >>>>> +#ifdef HAVE_PTHREAD_SETNAME_NP >>>>> +#define PTHREAD_SETNAME_NP pthread_setname_np >>>>> +#endif >>>>> + >>>>> #elif defined(__FreeBSD__) >>>>> #include <stdlib.h> >>>>> #include <string.h> >>>>> @@ -59,6 +66,45 @@ void lttng_ust_getprocname(char *name) >>>>> strncpy(name, bsd_name, LTTNG_UST_PROCNAME_LEN - 1); >>>>> } >>>>> >>>>> +/* >>>>> + * if pthread_set_name_np is available. >>>>> + */ >>>>> +#ifdef HAVE_PTHREAD_SET_NAME_NP >>>>> +#define PTHREAD_SETNAME_NP pthread_set_name_np >>>>> +#endif >>>>> + >>>>> +#endif >>>>> + >>>>> +/* >>>>> + * if a pthread setname/set_name function is available, declare >>>>> + * the setustprocname() function that will add '-ust' to the end >>>>> + * of the current process name, while truncating it if needed. >>>>> + */ >>>>> +#ifdef PTHREAD_SETNAME_NP >>>>> +#define LTTNG_UST_SETUSTPROCNAME lttng_ust_setustprocname() >>>>> + >>>>> +#include <pthread.h> >>>>> + >>>>> +static inline >>>>> +void lttng_ust_setustprocname() >>>>> +{ >>>>> + char name[LTTNG_UST_PROCNAME_LEN]; >>>> >>>> Let's say that LTTNG_UST_PROCNAME_LEN is 10. >>>> >>>>> + int limit = LTTNG_UST_PROCNAME_LEN - 4; >>>> >>>> Then, limit is 10 - 4 = 6. >>>> >>>>> + int len = 0; >>>> >>>> This can be moved on the strlen(name) line. >>>> >>>>> + >>>>> + lttng_ust_getprocname(name); >>>> >>>> Let's suppose that after the call to lttng_ust_getprocname(), name contains >>>> "012345678\0", >>>> >>>>> + >>>>> + len = strlen(name); >>>> >>>> The variable "len" will contain the integer 9. >>>> >>>>> + if (len >= limit) { >>>>> + len = limit; >>>>> + } >>>>> + >>>> >>>> After that, len is >= limit (9 >= 6), so len becomes 6. >>>> >>>>> + sprintf(name + len, "%s", "-ust"); >>>> >>>> name is a pointer to "012345678\0" (9 bytes + '\0') >>>> >>>> name + len is name + 6, so it points to "678\0" >>>> >>>> sprintf will essentially copy "-ust\0" to "678\0". >>>> >>>> So, if my calculations are not wrong, you have a buffer overflow which will >>>> corrupt the stack memory of the >>>> current stack frame. > > You're right, will fix that. > I'm also questionning the interest of limit being a variable here. > Should I replace both occurrences directly by "LTTNG_UST_PROCNAME_LEN > - 5" or create a "#define LTTNG_UST_PROCNAME_LIMIT > (LTTNG_UST_PROCNAME_LEN - 5)" macro ? Mathieu, what do you think would > be the best to do here ? Hardcoded constants are always a bad idea. #define MYSTRING "blah" LTTNG_UST_PROCNAME_LEN - strlen(MYSTRING) would be ok though. > The condition will also be replaced from ">=" to ">", as the "=" part > is not useful anymore in that version of the patch (it was in v1, > because of the operations that were done). > >>>> >>>> >>>>> + >>>>> + PTHREAD_SETNAME_NP(pthread_self(), name); >>>>> +} >>>>> +#else >>>>> +#define LTTNG_UST_SETUSTPROCNAME >>>>> #endif >>>>> >>>>> #include <errno.h> >>>>> diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c >>>>> index e00a22c..4de68c7 100644 >>>>> --- a/liblttng-ust/lttng-ust-comm.c >>>>> +++ b/liblttng-ust/lttng-ust-comm.c >>>>> @@ -1295,6 +1295,12 @@ void *ust_listener_thread(void *arg) >>>>> int sock, ret, prev_connect_failed = 0, has_waited = 0; >>>>> long timeout; >>>>> >>>>> + /* >>>>> + * If available, add '-ust' to the end of this thread's >>>>> + * process name >>>>> + */ >>>>> + LTTNG_UST_SETUSTPROCNAME; >>>> >>>> Is this common practice ? Just by looking at this line, it is unclear that it is >>>> a function call. >>>> >>>> I would prefer >>>> >>>> LTTNG_UST_SETUSTPROCNAME() >>>> >>>> or >>>> >>>> lttng_ust_setustprocname() >>> >>> The latter is preferred by our coding style, indeed. >>> >> >> And there is no reason to make it a macro. It should be >> a static inline whenever possible. > > I made it a macro as I was not sure about the best path to take, and > the macro seemed to be the cleaner approach! > However, would it be better like that: > > #ifdef PTHREAD_SETNAME_NP > #include <pthread.h> > #endif > > static inline > void lttng_ust_setustprocname() > { > #ifdef PTHREAD_SETNAME_NP > ... > > PTHREAD_SETNAME_NP(pthread_self(), name); > #endif > } > > Or like that ? > > #ifdef PTHREAD_SETNAME_NP > #include <pthread.h> > > static inline > void lttng_ust_setustprocname() > { > ... > > PTHREAD_SETNAME_NP(pthread_self(), name); > } > #else > static inline > void lttng_ust_setustprocname() > {} Change for static inline void lttng_ust_setustprocname(void) { } > #endif > Don't forget that (void) in C differs from (). () is like a variadic argument function. The latter form is prefered by our coding style. No #ifdef within functions unless it's really impossible to do otherwise. Thanks, Mathieu > > In both those cases, the macro disappears, and is replaced by a call > to lttng_ust_setustprocname(). > I'm not sure however which is preferable according to the coding style > (In one case, two #ifdef are required, in the other, we declare the > function in each branch of the ifdef/else). > > Thoughts ? I'll send a v3 of the patch according to your preference in > both matters. > > Thanks a lot! > Raphaël
Hi Raphaël, I had another thought. If the process name is "my-super-process", I think it only makes sense to append the "-ust" suffix if there is sufficient space. "my-super-process-ust" is fine, but I feel that "my-super-pro-ust" is meaningless. On 06/03/2016 12:12 PM, Raphaël Beamonte wrote: > 2016-06-03 11:55 GMT-04:00 Mathieu Desnoyers <mathieu.desnoyers at efficios.com>: >> ----- On Jun 3, 2016, at 5:54 PM, Mathieu Desnoyers mathieu.desnoyers at efficios.com wrote: >> >>> ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert sboisvert at gydle.com wrote: >>> >>>> Hi Raphaël, >>>> >>>> I think you have a buffer overflow, see below. >>>> >>>> On 06/03/2016 11:17 AM, Raphaël Beamonte wrote: >>>>> Add the required functions to change the thread name of the UST >>>>> threads and add the -ust string at its end. This will help to >>>>> identify LTTng-UST processes when analyzing the trace of a process. >>>>> >>>>> Signed-off-by: Raphaël Beamonte <raphael.beamonte at gmail.com> >>>>> --- >>>>> configure.ac | 5 +++++ >>>>> liblttng-ust/Makefile.am | 1 + >>>>> liblttng-ust/compat.h | 48 ++++++++++++++++++++++++++++++++++++++++++- >>>>> liblttng-ust/lttng-ust-comm.c | 6 ++++++ >>>>> 4 files changed, 59 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/configure.ac b/configure.ac >>>>> index 5692553..de462ff 100644 >>>>> --- a/configure.ac >>>>> +++ b/configure.ac >>>>> @@ -135,6 +135,11 @@ AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBDL], [test >>>>> "x$have_libdl" = "xyes"]) >>>>> AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBC_DL], [test "x$have_libc_dl" = "xyes"]) >>>>> >>>>> AC_CHECK_LIB([pthread], [pthread_create]) >>>>> +AC_CHECK_LIB([pthread], [pthread_setname_np], >>>>> + AC_DEFINE([HAVE_PTHREAD_SETNAME_NP], [1], [Define to 1 if pthread_setname_np >>>>> is available.]), >>>>> + AC_CHECK_LIB([pthread], [pthread_set_name_np], >>>>> + AC_DEFINE([HAVE_PTHREAD_SET_NAME_NP], [1], [Define to 1 if >>>>> pthread_set_name_np is available.]), >>>>> + AC_MSG_RESULT([pthread setname/set_name not found.]))) >>>>> >>>>> # Check for dlfcn.h >>>>> AC_CHECK_HEADER([dlfcn.h]) >>>>> diff --git a/liblttng-ust/Makefile.am b/liblttng-ust/Makefile.am >>>>> index f1801cf..05929be 100644 >>>>> --- a/liblttng-ust/Makefile.am >>>>> +++ b/liblttng-ust/Makefile.am >>>>> @@ -14,6 +14,7 @@ liblttng_ust_tracepoint_la_SOURCES = \ >>>>> error.h >>>>> liblttng_ust_tracepoint_la_LIBADD = \ >>>>> -lurcu-bp \ >>>>> + -lpthread \ >>>>> $(top_builddir)/snprintf/libustsnprintf.la >>>>> liblttng_ust_tracepoint_la_LDFLAGS = -no-undefined -version-info >>>>> $(LTTNG_UST_LIBRARY_VERSION) >>>>> liblttng_ust_tracepoint_la_CFLAGS = -DUST_COMPONENT="liblttng_ust_tracepoint" >>>>> -fno-strict-aliasing >>>>> diff --git a/liblttng-ust/compat.h b/liblttng-ust/compat.h >>>>> index 43b2223..4fee919 100644 >>>>> --- a/liblttng-ust/compat.h >>>>> +++ b/liblttng-ust/compat.h >>>>> @@ -3,6 +3,7 @@ >>>>> >>>>> /* >>>>> * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers at efficios.com> >>>>> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte at gmail.com> >>>>> * >>>>> * This library is free software; you can redistribute it and/or >>>>> * modify it under the terms of the GNU Lesser General Public >>>>> @@ -23,7 +24,6 @@ >>>>> * lttng_ust_getprocname. >>>>> */ >>>>> #ifdef __linux__ >>>>> - >>>>> #include <sys/prctl.h> >>>>> >>>>> #define LTTNG_UST_PROCNAME_LEN 17 >>>>> @@ -34,6 +34,13 @@ void lttng_ust_getprocname(char *name) >>>>> (void) prctl(PR_GET_NAME, (unsigned long) name, 0, 0, 0); >>>>> } >>>>> >>>>> +/* >>>>> + * if pthread_setname_np is available. >>>>> + */ >>>>> +#ifdef HAVE_PTHREAD_SETNAME_NP >>>>> +#define PTHREAD_SETNAME_NP pthread_setname_np >>>>> +#endif >>>>> + >>>>> #elif defined(__FreeBSD__) >>>>> #include <stdlib.h> >>>>> #include <string.h> >>>>> @@ -59,6 +66,45 @@ void lttng_ust_getprocname(char *name) >>>>> strncpy(name, bsd_name, LTTNG_UST_PROCNAME_LEN - 1); >>>>> } >>>>> >>>>> +/* >>>>> + * if pthread_set_name_np is available. >>>>> + */ >>>>> +#ifdef HAVE_PTHREAD_SET_NAME_NP >>>>> +#define PTHREAD_SETNAME_NP pthread_set_name_np >>>>> +#endif >>>>> + >>>>> +#endif >>>>> + >>>>> +/* >>>>> + * if a pthread setname/set_name function is available, declare >>>>> + * the setustprocname() function that will add '-ust' to the end >>>>> + * of the current process name, while truncating it if needed. >>>>> + */ >>>>> +#ifdef PTHREAD_SETNAME_NP >>>>> +#define LTTNG_UST_SETUSTPROCNAME lttng_ust_setustprocname() >>>>> + >>>>> +#include <pthread.h> >>>>> + >>>>> +static inline >>>>> +void lttng_ust_setustprocname() >>>>> +{ >>>>> + char name[LTTNG_UST_PROCNAME_LEN]; >>>> >>>> Let's say that LTTNG_UST_PROCNAME_LEN is 10. >>>> >>>>> + int limit = LTTNG_UST_PROCNAME_LEN - 4; >>>> >>>> Then, limit is 10 - 4 = 6. >>>> >>>>> + int len = 0; >>>> >>>> This can be moved on the strlen(name) line. >>>> >>>>> + >>>>> + lttng_ust_getprocname(name); >>>> >>>> Let's suppose that after the call to lttng_ust_getprocname(), name contains >>>> "012345678\0", >>>> >>>>> + >>>>> + len = strlen(name); >>>> >>>> The variable "len" will contain the integer 9. >>>> >>>>> + if (len >= limit) { >>>>> + len = limit; >>>>> + } >>>>> + >>>> >>>> After that, len is >= limit (9 >= 6), so len becomes 6. >>>> >>>>> + sprintf(name + len, "%s", "-ust"); >>>> >>>> name is a pointer to "012345678\0" (9 bytes + '\0') >>>> >>>> name + len is name + 6, so it points to "678\0" >>>> >>>> sprintf will essentially copy "-ust\0" to "678\0". >>>> >>>> So, if my calculations are not wrong, you have a buffer overflow which will >>>> corrupt the stack memory of the >>>> current stack frame. > > You're right, will fix that. > I'm also questionning the interest of limit being a variable here. > Should I replace both occurrences directly by "LTTNG_UST_PROCNAME_LEN > - 5" or create a "#define LTTNG_UST_PROCNAME_LIMIT > (LTTNG_UST_PROCNAME_LEN - 5)" macro ? Mathieu, what do you think would > be the best to do here ? > The condition will also be replaced from ">=" to ">", as the "=" part > is not useful anymore in that version of the patch (it was in v1, > because of the operations that were done). > >>>> >>>> >>>>> + >>>>> + PTHREAD_SETNAME_NP(pthread_self(), name); >>>>> +} >>>>> +#else >>>>> +#define LTTNG_UST_SETUSTPROCNAME >>>>> #endif >>>>> >>>>> #include <errno.h> >>>>> diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c >>>>> index e00a22c..4de68c7 100644 >>>>> --- a/liblttng-ust/lttng-ust-comm.c >>>>> +++ b/liblttng-ust/lttng-ust-comm.c >>>>> @@ -1295,6 +1295,12 @@ void *ust_listener_thread(void *arg) >>>>> int sock, ret, prev_connect_failed = 0, has_waited = 0; >>>>> long timeout; >>>>> >>>>> + /* >>>>> + * If available, add '-ust' to the end of this thread's >>>>> + * process name >>>>> + */ >>>>> + LTTNG_UST_SETUSTPROCNAME; >>>> >>>> Is this common practice ? Just by looking at this line, it is unclear that it is >>>> a function call. >>>> >>>> I would prefer >>>> >>>> LTTNG_UST_SETUSTPROCNAME() >>>> >>>> or >>>> >>>> lttng_ust_setustprocname() >>> >>> The latter is preferred by our coding style, indeed. >>> >> >> And there is no reason to make it a macro. It should be >> a static inline whenever possible. > > I made it a macro as I was not sure about the best path to take, and > the macro seemed to be the cleaner approach! > However, would it be better like that: > > #ifdef PTHREAD_SETNAME_NP > #include <pthread.h> > #endif > > static inline > void lttng_ust_setustprocname() > { > #ifdef PTHREAD_SETNAME_NP > ... > > PTHREAD_SETNAME_NP(pthread_self(), name); > #endif > } > > Or like that ? > > #ifdef PTHREAD_SETNAME_NP > #include <pthread.h> > > static inline > void lttng_ust_setustprocname() > { > ... > > PTHREAD_SETNAME_NP(pthread_self(), name); > } > #else > static inline > void lttng_ust_setustprocname() > {} > #endif > > > In both those cases, the macro disappears, and is replaced by a call > to lttng_ust_setustprocname(). > I'm not sure however which is preferable according to the coding style > (In one case, two #ifdef are required, in the other, we declare the > function in each branch of the ifdef/else). > > Thoughts ? I'll send a v3 of the patch according to your preference in > both matters. > > Thanks a lot! > Raphaël >
----- On Jun 3, 2016, at 6:24 PM, Sebastien Boisvert sboisvert at gydle.com wrote: > Hi Raphaël, > > I had another thought. > > If the process name is "my-super-process", I think it only makes sense to append > the > "-ust" suffix if there is sufficient space. "my-super-process-ust" is fine, but > I feel > that "my-super-pro-ust" is meaningless. I see having the -ust overwriting the end of the process name as a feature that would be useful for users to distinguish between ust threads and non-ust threads in cases where the binary name is long. Thanks, Mathieu > > > On 06/03/2016 12:12 PM, Raphaël Beamonte wrote: >> 2016-06-03 11:55 GMT-04:00 Mathieu Desnoyers <mathieu.desnoyers at efficios.com>: >>> ----- On Jun 3, 2016, at 5:54 PM, Mathieu Desnoyers >>> mathieu.desnoyers at efficios.com wrote: >>> >>>> ----- On Jun 3, 2016, at 5:47 PM, Sebastien Boisvert sboisvert at gydle.com wrote: >>>> >>>>> Hi Raphaël, >>>>> >>>>> I think you have a buffer overflow, see below. >>>>> >>>>> On 06/03/2016 11:17 AM, Raphaël Beamonte wrote: >>>>>> Add the required functions to change the thread name of the UST >>>>>> threads and add the -ust string at its end. This will help to >>>>>> identify LTTng-UST processes when analyzing the trace of a process. >>>>>> >>>>>> Signed-off-by: Raphaël Beamonte <raphael.beamonte at gmail.com> >>>>>> --- >>>>>> configure.ac | 5 +++++ >>>>>> liblttng-ust/Makefile.am | 1 + >>>>>> liblttng-ust/compat.h | 48 ++++++++++++++++++++++++++++++++++++++++++- >>>>>> liblttng-ust/lttng-ust-comm.c | 6 ++++++ >>>>>> 4 files changed, 59 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/configure.ac b/configure.ac >>>>>> index 5692553..de462ff 100644 >>>>>> --- a/configure.ac >>>>>> +++ b/configure.ac >>>>>> @@ -135,6 +135,11 @@ AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBDL], [test >>>>>> "x$have_libdl" = "xyes"]) >>>>>> AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBC_DL], [test "x$have_libc_dl" = "xyes"]) >>>>>> >>>>>> AC_CHECK_LIB([pthread], [pthread_create]) >>>>>> +AC_CHECK_LIB([pthread], [pthread_setname_np], >>>>>> + AC_DEFINE([HAVE_PTHREAD_SETNAME_NP], [1], [Define to 1 if pthread_setname_np >>>>>> is available.]), >>>>>> + AC_CHECK_LIB([pthread], [pthread_set_name_np], >>>>>> + AC_DEFINE([HAVE_PTHREAD_SET_NAME_NP], [1], [Define to 1 if >>>>>> pthread_set_name_np is available.]), >>>>>> + AC_MSG_RESULT([pthread setname/set_name not found.]))) >>>>>> >>>>>> # Check for dlfcn.h >>>>>> AC_CHECK_HEADER([dlfcn.h]) >>>>>> diff --git a/liblttng-ust/Makefile.am b/liblttng-ust/Makefile.am >>>>>> index f1801cf..05929be 100644 >>>>>> --- a/liblttng-ust/Makefile.am >>>>>> +++ b/liblttng-ust/Makefile.am >>>>>> @@ -14,6 +14,7 @@ liblttng_ust_tracepoint_la_SOURCES = \ >>>>>> error.h >>>>>> liblttng_ust_tracepoint_la_LIBADD = \ >>>>>> -lurcu-bp \ >>>>>> + -lpthread \ >>>>>> $(top_builddir)/snprintf/libustsnprintf.la >>>>>> liblttng_ust_tracepoint_la_LDFLAGS = -no-undefined -version-info >>>>>> $(LTTNG_UST_LIBRARY_VERSION) >>>>>> liblttng_ust_tracepoint_la_CFLAGS = -DUST_COMPONENT="liblttng_ust_tracepoint" >>>>>> -fno-strict-aliasing >>>>>> diff --git a/liblttng-ust/compat.h b/liblttng-ust/compat.h >>>>>> index 43b2223..4fee919 100644 >>>>>> --- a/liblttng-ust/compat.h >>>>>> +++ b/liblttng-ust/compat.h >>>>>> @@ -3,6 +3,7 @@ >>>>>> >>>>>> /* >>>>>> * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers at efficios.com> >>>>>> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte at gmail.com> >>>>>> * >>>>>> * This library is free software; you can redistribute it and/or >>>>>> * modify it under the terms of the GNU Lesser General Public >>>>>> @@ -23,7 +24,6 @@ >>>>>> * lttng_ust_getprocname. >>>>>> */ >>>>>> #ifdef __linux__ >>>>>> - >>>>>> #include <sys/prctl.h> >>>>>> >>>>>> #define LTTNG_UST_PROCNAME_LEN 17 >>>>>> @@ -34,6 +34,13 @@ void lttng_ust_getprocname(char *name) >>>>>> (void) prctl(PR_GET_NAME, (unsigned long) name, 0, 0, 0); >>>>>> } >>>>>> >>>>>> +/* >>>>>> + * if pthread_setname_np is available. >>>>>> + */ >>>>>> +#ifdef HAVE_PTHREAD_SETNAME_NP >>>>>> +#define PTHREAD_SETNAME_NP pthread_setname_np >>>>>> +#endif >>>>>> + >>>>>> #elif defined(__FreeBSD__) >>>>>> #include <stdlib.h> >>>>>> #include <string.h> >>>>>> @@ -59,6 +66,45 @@ void lttng_ust_getprocname(char *name) >>>>>> strncpy(name, bsd_name, LTTNG_UST_PROCNAME_LEN - 1); >>>>>> } >>>>>> >>>>>> +/* >>>>>> + * if pthread_set_name_np is available. >>>>>> + */ >>>>>> +#ifdef HAVE_PTHREAD_SET_NAME_NP >>>>>> +#define PTHREAD_SETNAME_NP pthread_set_name_np >>>>>> +#endif >>>>>> + >>>>>> +#endif >>>>>> + >>>>>> +/* >>>>>> + * if a pthread setname/set_name function is available, declare >>>>>> + * the setustprocname() function that will add '-ust' to the end >>>>>> + * of the current process name, while truncating it if needed. >>>>>> + */ >>>>>> +#ifdef PTHREAD_SETNAME_NP >>>>>> +#define LTTNG_UST_SETUSTPROCNAME lttng_ust_setustprocname() >>>>>> + >>>>>> +#include <pthread.h> >>>>>> + >>>>>> +static inline >>>>>> +void lttng_ust_setustprocname() >>>>>> +{ >>>>>> + char name[LTTNG_UST_PROCNAME_LEN]; >>>>> >>>>> Let's say that LTTNG_UST_PROCNAME_LEN is 10. >>>>> >>>>>> + int limit = LTTNG_UST_PROCNAME_LEN - 4; >>>>> >>>>> Then, limit is 10 - 4 = 6. >>>>> >>>>>> + int len = 0; >>>>> >>>>> This can be moved on the strlen(name) line. >>>>> >>>>>> + >>>>>> + lttng_ust_getprocname(name); >>>>> >>>>> Let's suppose that after the call to lttng_ust_getprocname(), name contains >>>>> "012345678\0", >>>>> >>>>>> + >>>>>> + len = strlen(name); >>>>> >>>>> The variable "len" will contain the integer 9. >>>>> >>>>>> + if (len >= limit) { >>>>>> + len = limit; >>>>>> + } >>>>>> + >>>>> >>>>> After that, len is >= limit (9 >= 6), so len becomes 6. >>>>> >>>>>> + sprintf(name + len, "%s", "-ust"); >>>>> >>>>> name is a pointer to "012345678\0" (9 bytes + '\0') >>>>> >>>>> name + len is name + 6, so it points to "678\0" >>>>> >>>>> sprintf will essentially copy "-ust\0" to "678\0". >>>>> >>>>> So, if my calculations are not wrong, you have a buffer overflow which will >>>>> corrupt the stack memory of the >>>>> current stack frame. >> >> You're right, will fix that. >> I'm also questionning the interest of limit being a variable here. >> Should I replace both occurrences directly by "LTTNG_UST_PROCNAME_LEN >> - 5" or create a "#define LTTNG_UST_PROCNAME_LIMIT >> (LTTNG_UST_PROCNAME_LEN - 5)" macro ? Mathieu, what do you think would >> be the best to do here ? >> The condition will also be replaced from ">=" to ">", as the "=" part >> is not useful anymore in that version of the patch (it was in v1, >> because of the operations that were done). >> >>>>> >>>>> >>>>>> + >>>>>> + PTHREAD_SETNAME_NP(pthread_self(), name); >>>>>> +} >>>>>> +#else >>>>>> +#define LTTNG_UST_SETUSTPROCNAME >>>>>> #endif >>>>>> >>>>>> #include <errno.h> >>>>>> diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c >>>>>> index e00a22c..4de68c7 100644 >>>>>> --- a/liblttng-ust/lttng-ust-comm.c >>>>>> +++ b/liblttng-ust/lttng-ust-comm.c >>>>>> @@ -1295,6 +1295,12 @@ void *ust_listener_thread(void *arg) >>>>>> int sock, ret, prev_connect_failed = 0, has_waited = 0; >>>>>> long timeout; >>>>>> >>>>>> + /* >>>>>> + * If available, add '-ust' to the end of this thread's >>>>>> + * process name >>>>>> + */ >>>>>> + LTTNG_UST_SETUSTPROCNAME; >>>>> >>>>> Is this common practice ? Just by looking at this line, it is unclear that it is >>>>> a function call. >>>>> >>>>> I would prefer >>>>> >>>>> LTTNG_UST_SETUSTPROCNAME() >>>>> >>>>> or >>>>> >>>>> lttng_ust_setustprocname() >>>> >>>> The latter is preferred by our coding style, indeed. >>>> >>> >>> And there is no reason to make it a macro. It should be >>> a static inline whenever possible. >> >> I made it a macro as I was not sure about the best path to take, and >> the macro seemed to be the cleaner approach! >> However, would it be better like that: >> >> #ifdef PTHREAD_SETNAME_NP >> #include <pthread.h> >> #endif >> >> static inline >> void lttng_ust_setustprocname() >> { >> #ifdef PTHREAD_SETNAME_NP >> ... >> >> PTHREAD_SETNAME_NP(pthread_self(), name); >> #endif >> } >> >> Or like that ? >> >> #ifdef PTHREAD_SETNAME_NP >> #include <pthread.h> >> >> static inline >> void lttng_ust_setustprocname() >> { >> ... >> >> PTHREAD_SETNAME_NP(pthread_self(), name); >> } >> #else >> static inline >> void lttng_ust_setustprocname() >> {} >> #endif >> >> >> In both those cases, the macro disappears, and is replaced by a call >> to lttng_ust_setustprocname(). >> I'm not sure however which is preferable according to the coding style >> (In one case, two #ifdef are required, in the other, we declare the >> function in each branch of the ifdef/else). >> >> Thoughts ? I'll send a v3 of the patch according to your preference in >> both matters. >> >> Thanks a lot! >> Raphaël
diff --git a/configure.ac b/configure.ac index 5692553..de462ff 100644 --- a/configure.ac +++ b/configure.ac @@ -135,6 +135,11 @@ AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBDL], [test "x$have_libdl" = "xyes"]) AM_CONDITIONAL([LTTNG_UST_BUILD_WITH_LIBC_DL], [test "x$have_libc_dl" = "xyes"]) AC_CHECK_LIB([pthread], [pthread_create]) +AC_CHECK_LIB([pthread], [pthread_setname_np], + AC_DEFINE([HAVE_PTHREAD_SETNAME_NP], [1], [Define to 1 if pthread_setname_np is available.]), + AC_CHECK_LIB([pthread], [pthread_set_name_np], + AC_DEFINE([HAVE_PTHREAD_SET_NAME_NP], [1], [Define to 1 if pthread_set_name_np is available.]), + AC_MSG_RESULT([pthread setname/set_name not found.]))) # Check for dlfcn.h AC_CHECK_HEADER([dlfcn.h]) diff --git a/liblttng-ust/Makefile.am b/liblttng-ust/Makefile.am index f1801cf..05929be 100644 --- a/liblttng-ust/Makefile.am +++ b/liblttng-ust/Makefile.am @@ -14,6 +14,7 @@ liblttng_ust_tracepoint_la_SOURCES = \ error.h liblttng_ust_tracepoint_la_LIBADD = \ -lurcu-bp \ + -lpthread \ $(top_builddir)/snprintf/libustsnprintf.la liblttng_ust_tracepoint_la_LDFLAGS = -no-undefined -version-info $(LTTNG_UST_LIBRARY_VERSION) liblttng_ust_tracepoint_la_CFLAGS = -DUST_COMPONENT="liblttng_ust_tracepoint" -fno-strict-aliasing diff --git a/liblttng-ust/compat.h b/liblttng-ust/compat.h index 43b2223..4fee919 100644 --- a/liblttng-ust/compat.h +++ b/liblttng-ust/compat.h @@ -3,6 +3,7 @@ /* * Copyright (C) 2011 Mathieu Desnoyers <mathieu.desnoyers at efficios.com> + * Copyright (C) 2016 Raphaël Beamonte <raphael.beamonte at gmail.com> * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -23,7 +24,6 @@ * lttng_ust_getprocname. */ #ifdef __linux__ - #include <sys/prctl.h> #define LTTNG_UST_PROCNAME_LEN 17 @@ -34,6 +34,13 @@ void lttng_ust_getprocname(char *name) (void) prctl(PR_GET_NAME, (unsigned long) name, 0, 0, 0); } +/* + * if pthread_setname_np is available. + */ +#ifdef HAVE_PTHREAD_SETNAME_NP +#define PTHREAD_SETNAME_NP pthread_setname_np +#endif + #elif defined(__FreeBSD__) #include <stdlib.h> #include <string.h> @@ -59,6 +66,45 @@ void lttng_ust_getprocname(char *name) strncpy(name, bsd_name, LTTNG_UST_PROCNAME_LEN - 1); } +/* + * if pthread_set_name_np is available. + */ +#ifdef HAVE_PTHREAD_SET_NAME_NP +#define PTHREAD_SETNAME_NP pthread_set_name_np +#endif + +#endif + +/* + * if a pthread setname/set_name function is available, declare + * the setustprocname() function that will add '-ust' to the end + * of the current process name, while truncating it if needed. + */ +#ifdef PTHREAD_SETNAME_NP +#define LTTNG_UST_SETUSTPROCNAME lttng_ust_setustprocname() + +#include <pthread.h> + +static inline +void lttng_ust_setustprocname() +{ + char name[LTTNG_UST_PROCNAME_LEN]; + int limit = LTTNG_UST_PROCNAME_LEN - 4; + int len = 0; + + lttng_ust_getprocname(name); + + len = strlen(name); + if (len >= limit) { + len = limit; + } + + sprintf(name + len, "%s", "-ust"); + + PTHREAD_SETNAME_NP(pthread_self(), name); +} +#else +#define LTTNG_UST_SETUSTPROCNAME #endif #include <errno.h> diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c index e00a22c..4de68c7 100644 --- a/liblttng-ust/lttng-ust-comm.c +++ b/liblttng-ust/lttng-ust-comm.c @@ -1295,6 +1295,12 @@ void *ust_listener_thread(void *arg) int sock, ret, prev_connect_failed = 0, has_waited = 0; long timeout; + /* + * If available, add '-ust' to the end of this thread's + * process name + */ + LTTNG_UST_SETUSTPROCNAME; + /* Restart trying to connect to the session daemon */ restart: if (prev_connect_failed) {