Brad Bishop | 1932369 | 2019-04-05 15:28:33 -0400 | [diff] [blame] | 1 | From 10e8001ad876d8cb3b5a17c7492e713bbc047975 Mon Sep 17 00:00:00 2001 |
| 2 | From: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com> |
| 3 | Date: Thu, 28 Mar 2019 18:31:29 -0400 |
| 4 | Subject: [PATCH] Fix: getgrnam is not MT-Safe, use getgrnam_r |
| 5 | |
| 6 | Running the test suite under a Yocto musl build resulted in musl |
| 7 | coredump due to double freeing. |
| 8 | |
| 9 | We get the following backtraces: |
| 10 | |
| 11 | 0 a_crash () at ./arch/x86_64/atomic_arch.h:108 |
| 12 | 1 unmap_chunk (self=<optimized out>) at src/malloc/malloc.c:515 |
| 13 | 2 free (p=<optimized out>) at src/malloc/malloc.c:526 |
| 14 | 3 0x00007f46d9dc3849 in __getgrent_a (f=f@entry=0x7f46d9d1f7e0, gr=gr@entry=0x7f46d9e24460 <gr>, line=line@entry=0x7f46d9e26058 <line>, size=size@entry=0x7f46d92db550, mem=mem@entry=0x7f46d9e26050 <mem>, nmem=nmem@entry=0x7f46d92db558, res=0x7f46d92db548) at src/passwd/getgrent_a.c:45 |
| 15 | 4 0x00007f46d9dc2e6b in __getgr_a (name=0x487242 "tracing", gid=gid@entry=0, gr=gr@entry=0x7f46d9e24460 <gr>, buf=buf@entry=0x7f46d9e26058 <line>, size=size@entry=0x7f46d92db550, mem=mem@entry=0x7f46d9e26050 <mem>, nmem=0x7f46d92db558, res=0x7f46d92db548) at src/passwd/getgr_a.c:30 |
| 16 | 5 0x00007f46d9dc3733 in getgrnam (name=<optimized out>) at src/passwd/getgrent.c:37 |
| 17 | 6 0x0000000000460b29 in utils_get_group_id (name=<optimized out>) at ../../../lttng-tools-2.10.6/src/common/utils.c:1241 |
| 18 | 7 0x000000000044ee69 in thread_manage_health (data=<optimized out>) at ../../../../lttng-tools-2.10.6/src/bin/lttng-sessiond/main.c:4115 |
| 19 | 8 0x00007f46d9de1541 in start (p=<optimized out>) at src/thread/pthread_create.c:195 |
| 20 | 9 0x00007f46d9dee661 in __clone () at src/thread/x86_64/clone.s:22 |
| 21 | |
| 22 | From another run: |
| 23 | |
| 24 | 0 a_crash () at ./arch/x86_64/atomic_arch.h:108 |
| 25 | 1 unmap_chunk (self=<optimized out>) at src/malloc/malloc.c:515 |
| 26 | 2 free (p=<optimized out>) at src/malloc/malloc.c:526 |
| 27 | 3 0x00007f5abc210849 in __getgrent_a (f=f@entry=0x7f5abc2733e0, gr=gr@entry=0x7f5abc271460 <gr>, line=line@entry=0x7f5abc273058 <line>, size=size@entry=0x7f5abaef5510, mem=mem@entry=0x7f5abc273050 <mem>, nmem=nmem@entry=0x7f5abaef5518, res=0x7f5abaef5508) at src/passwd/getgrent_a.c:45 |
| 28 | 4 0x00007f5abc20fe6b in __getgr_a (name=0x487242 "tracing", gid=gid@entry=0, gr=gr@entry=0x7f5abc271460 <gr>, buf=buf@entry=0x7f5abc273058 <line>, size=size@entry=0x7f5abaef5510, mem=mem@entry=0x7f5abc273050 <mem>, nmem=0x7f5abaef5518, res=0x7f5abaef5508) at src/passwd/getgr_a.c:30 |
| 29 | 5 0x00007f5abc210733 in getgrnam (name=<optimized out>) at src/passwd/getgrent.c:37 |
| 30 | 6 0x0000000000460b29 in utils_get_group_id (name=<optimized out>) at ../../../lttng-tools-2.10.6/src/common/utils.c:1241 |
| 31 | 7 0x000000000042dee4 in notification_channel_socket_create () at ../../../../lttng-tools-2.10.6/src/bin/lttng-sessiond/notification-thread.c:238 |
| 32 | 8 init_thread_state (state=0x7f5abaef5560, handle=0x7f5abbf9be40) at ../../../../lttng-tools-2.10.6/src/bin/lttng-sessiond/notification-thread.c:375 |
| 33 | 9 thread_notification (data=0x7f5abbf9be40) at ../../../../lttng-tools-2.10.6/src/bin/lttng-sessiond/notification-thread.c:495 |
| 34 | 10 0x00007f5abc22e541 in start (p=<optimized out>) at src/thread/pthread_create.c:195 |
| 35 | 11 0x00007f5abc23b661 in __clone () at src/thread/x86_64/clone.s:22 |
| 36 | |
| 37 | The problem was easily reproducible (~6 crash on ~300 runs). A prototype fix |
| 38 | using mutex around the getgrnam yielded no crash in over 1000 runs. This |
| 39 | patch yielded the same results as the prototype fix. |
| 40 | |
| 41 | Unfortunately we cannot rely on a mutex in liblttng-ctl since we cannot |
| 42 | enforce the locking for the application using the lib. |
| 43 | |
| 44 | Use getgrnam_r instead. |
| 45 | |
| 46 | The previous implementation of utils_get_group_id returned the gid of |
| 47 | the root group (0) on error/not found. lttng_check_tracing_group needs |
| 48 | to know if an error/not found occured, returning the root group is not |
| 49 | enough. We now return the gid via the passed parameter. The caller is |
| 50 | responsible for either defaulting to the root group or propagating the |
| 51 | error. |
| 52 | |
| 53 | We also do not want to warn when used in liblttng-ctl context. We might |
| 54 | want to move the warning elsewhere in the future. For now, pass a bool |
| 55 | if we need to warn or not. |
| 56 | |
| 57 | Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com> |
Brad Bishop | c342db3 | 2019-05-15 21:57:59 -0400 | [diff] [blame] | 58 | |
| 59 | Upstream-Status: Submitted [https://patchwork.lttng.org/patch/2314703] |
Brad Bishop | 1932369 | 2019-04-05 15:28:33 -0400 | [diff] [blame] | 60 | --- |
| 61 | src/bin/lttng-consumerd/health-consumerd.c | 10 ++- |
| 62 | src/bin/lttng-relayd/health-relayd.c | 20 ++++-- |
| 63 | src/bin/lttng-sessiond/main.c | 24 +++++-- |
| 64 | src/bin/lttng-sessiond/notification-thread.c | 10 ++- |
| 65 | src/common/utils.c | 75 +++++++++++++++++--- |
| 66 | src/common/utils.h | 4 +- |
| 67 | src/lib/lttng-ctl/lttng-ctl.c | 8 +-- |
| 68 | 7 files changed, 122 insertions(+), 29 deletions(-) |
| 69 | |
| 70 | diff --git a/src/bin/lttng-consumerd/health-consumerd.c b/src/bin/lttng-consumerd/health-consumerd.c |
| 71 | index 1e2f31e4..6045401a 100644 |
| 72 | --- a/src/bin/lttng-consumerd/health-consumerd.c |
| 73 | +++ b/src/bin/lttng-consumerd/health-consumerd.c |
| 74 | @@ -184,8 +184,14 @@ void *thread_manage_health(void *data) |
| 75 | is_root = !getuid(); |
| 76 | if (is_root) { |
| 77 | /* lttng health client socket path permissions */ |
| 78 | - ret = chown(health_unix_sock_path, 0, |
| 79 | - utils_get_group_id(tracing_group_name)); |
| 80 | + gid_t gid; |
| 81 | + |
| 82 | + ret = utils_get_group_id(tracing_group_name, true, &gid); |
| 83 | + if (ret) { |
| 84 | + gid = 0; /* Default to root group. */ |
| 85 | + } |
| 86 | + |
| 87 | + ret = chown(health_unix_sock_path, 0, gid); |
| 88 | if (ret < 0) { |
| 89 | ERR("Unable to set group on %s", health_unix_sock_path); |
| 90 | PERROR("chown"); |
| 91 | diff --git a/src/bin/lttng-relayd/health-relayd.c b/src/bin/lttng-relayd/health-relayd.c |
| 92 | index ba996621..962e88c4 100644 |
| 93 | --- a/src/bin/lttng-relayd/health-relayd.c |
| 94 | +++ b/src/bin/lttng-relayd/health-relayd.c |
| 95 | @@ -105,8 +105,14 @@ static int create_lttng_rundir_with_perm(const char *rundir) |
| 96 | int is_root = !getuid(); |
| 97 | |
| 98 | if (is_root) { |
| 99 | - ret = chown(rundir, 0, |
| 100 | - utils_get_group_id(tracing_group_name)); |
| 101 | + gid_t gid; |
| 102 | + |
| 103 | + ret = utils_get_group_id(tracing_group_name, true, &gid); |
| 104 | + if (ret) { |
| 105 | + gid = 0; /* Default to root group.*/ |
| 106 | + } |
| 107 | + |
| 108 | + ret = chown(rundir, 0, gid); |
| 109 | if (ret < 0) { |
| 110 | ERR("Unable to set group on %s", rundir); |
| 111 | PERROR("chown"); |
| 112 | @@ -256,8 +262,14 @@ void *thread_manage_health(void *data) |
| 113 | is_root = !getuid(); |
| 114 | if (is_root) { |
| 115 | /* lttng health client socket path permissions */ |
| 116 | - ret = chown(health_unix_sock_path, 0, |
| 117 | - utils_get_group_id(tracing_group_name)); |
| 118 | + gid_t gid; |
| 119 | + |
| 120 | + ret = utils_get_group_id(tracing_group_name, true, &gid); |
| 121 | + if (ret) { |
| 122 | + gid = 0; /* Default to root group */ |
| 123 | + } |
| 124 | + |
| 125 | + ret = chown(health_unix_sock_path, 0, gid); |
| 126 | if (ret < 0) { |
| 127 | ERR("Unable to set group on %s", health_unix_sock_path); |
| 128 | PERROR("chown"); |
| 129 | diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c |
| 130 | index fa6fa483..49307064 100644 |
| 131 | --- a/src/bin/lttng-sessiond/main.c |
| 132 | +++ b/src/bin/lttng-sessiond/main.c |
| 133 | @@ -4112,8 +4112,14 @@ static void *thread_manage_health(void *data) |
| 134 | |
| 135 | if (is_root) { |
| 136 | /* lttng health client socket path permissions */ |
| 137 | - ret = chown(config.health_unix_sock_path.value, 0, |
| 138 | - utils_get_group_id(config.tracing_group_name.value)); |
| 139 | + gid_t gid; |
| 140 | + |
| 141 | + ret = utils_get_group_id(config.tracing_group_name.value, true, &gid); |
| 142 | + if (ret) { |
| 143 | + gid = 0; /* Default to root group */ |
| 144 | + } |
| 145 | + |
| 146 | + ret = chown(config.health_unix_sock_path.value, 0, &gid); |
| 147 | if (ret < 0) { |
| 148 | ERR("Unable to set group on %s", config.health_unix_sock_path.value); |
| 149 | PERROR("chown"); |
| 150 | @@ -5238,7 +5244,10 @@ static int set_permissions(char *rundir) |
| 151 | int ret; |
| 152 | gid_t gid; |
| 153 | |
| 154 | - gid = utils_get_group_id(config.tracing_group_name.value); |
| 155 | + ret = utils_get_group_id(config.tracing_group_name.value, true, &gid); |
| 156 | + if (ret) { |
| 157 | + gid = 0; /* Default to root group */ |
| 158 | + } |
| 159 | |
| 160 | /* Set lttng run dir */ |
| 161 | ret = chown(rundir, 0, gid); |
| 162 | @@ -5349,7 +5358,14 @@ static int set_consumer_sockets(struct consumer_data *consumer_data) |
| 163 | goto error; |
| 164 | } |
| 165 | if (is_root) { |
| 166 | - ret = chown(path, 0, utils_get_group_id(config.tracing_group_name.value)); |
| 167 | + gid_t gid; |
| 168 | + |
| 169 | + ret = utils_get_group_id(config.tracing_group_name.value, true, &gid); |
| 170 | + if (ret) { |
| 171 | + gid = 0; /* Default to root group */ |
| 172 | + } |
| 173 | + |
| 174 | + ret = chown(path, 0, gid); |
| 175 | if (ret < 0) { |
| 176 | ERR("Unable to set group on %s", path); |
| 177 | PERROR("chown"); |
| 178 | diff --git a/src/bin/lttng-sessiond/notification-thread.c b/src/bin/lttng-sessiond/notification-thread.c |
| 179 | index 92ac597f..18a264d9 100644 |
| 180 | --- a/src/bin/lttng-sessiond/notification-thread.c |
| 181 | +++ b/src/bin/lttng-sessiond/notification-thread.c |
| 182 | @@ -235,8 +235,14 @@ int notification_channel_socket_create(void) |
| 183 | } |
| 184 | |
| 185 | if (getuid() == 0) { |
| 186 | - ret = chown(sock_path, 0, |
| 187 | - utils_get_group_id(config.tracing_group_name.value)); |
| 188 | + gid_t gid; |
| 189 | + |
| 190 | + ret = utils_get_group_id(config.tracing_group_name.value, true, &gid); |
| 191 | + if (ret) { |
| 192 | + gid = 0; /* Default to root group. */ |
| 193 | + } |
| 194 | + |
| 195 | + ret = chown(sock_path, 0, gid); |
| 196 | if (ret) { |
| 197 | ERR("Failed to set the notification channel socket's group"); |
| 198 | ret = -1; |
| 199 | diff --git a/src/common/utils.c b/src/common/utils.c |
| 200 | index c0bb031e..778bc00f 100644 |
| 201 | --- a/src/common/utils.c |
| 202 | +++ b/src/common/utils.c |
| 203 | @@ -1231,24 +1231,77 @@ size_t utils_get_current_time_str(const char *format, char *dst, size_t len) |
| 204 | } |
| 205 | |
| 206 | /* |
| 207 | - * Return the group ID matching name, else 0 if it cannot be found. |
| 208 | + * Return 0 on success and set *gid to the group_ID matching the passed name. |
| 209 | + * Else -1 if it cannot be found or an error occurred. |
| 210 | */ |
| 211 | LTTNG_HIDDEN |
| 212 | -gid_t utils_get_group_id(const char *name) |
| 213 | +int utils_get_group_id(const char *name, bool warn, gid_t *gid) |
| 214 | { |
| 215 | - struct group *grp; |
| 216 | + static volatile int warn_once; |
| 217 | |
| 218 | - grp = getgrnam(name); |
| 219 | - if (!grp) { |
| 220 | - static volatile int warn_once; |
| 221 | + int ret; |
| 222 | + long sys_len; |
| 223 | + size_t len; |
| 224 | + struct group grp; |
| 225 | + struct group *result; |
| 226 | + char *buffer = NULL; |
| 227 | |
| 228 | - if (!warn_once) { |
| 229 | - WARN("No tracing group detected"); |
| 230 | - warn_once = 1; |
| 231 | + /* Get the system limit if it exists */ |
| 232 | + sys_len = sysconf(_SC_GETGR_R_SIZE_MAX); |
| 233 | + if (sys_len == -1) { |
| 234 | + len = 1024; |
| 235 | + } else { |
| 236 | + len = (size_t) sys_len; |
| 237 | + } |
| 238 | + |
| 239 | + buffer = malloc(len); |
| 240 | + if (!buffer) { |
| 241 | + PERROR("getgrnam_r malloc"); |
| 242 | + ret = -1; |
| 243 | + goto error; |
| 244 | + } |
| 245 | + |
| 246 | + while ((ret = getgrnam_r(name, &grp, buffer, len, &result)) == ERANGE) |
| 247 | + { |
| 248 | + /* Buffer is not big enough, increase its size. */ |
| 249 | + size_t new_len = 2 * len; |
| 250 | + char *new_buffer = NULL; |
| 251 | + if (new_len < len) { |
| 252 | + ERR("getgrnam_r buffer size overflow"); |
| 253 | + ret = -1; |
| 254 | + goto error; |
| 255 | + } |
| 256 | + len = new_len; |
| 257 | + new_buffer = realloc(buffer, len); |
| 258 | + if (!new_buffer) { |
| 259 | + PERROR("getgrnam_r realloc"); |
| 260 | + ret = -1; |
| 261 | + goto error; |
| 262 | } |
| 263 | - return 0; |
| 264 | + buffer = new_buffer; |
| 265 | + } |
| 266 | + if (ret != 0) { |
| 267 | + PERROR("getgrnam_r"); |
| 268 | + ret = -1; |
| 269 | + goto error; |
| 270 | + } |
| 271 | + |
| 272 | + /* Group not found. */ |
| 273 | + if (!result) { |
| 274 | + ret = -1; |
| 275 | + goto error; |
| 276 | + } |
| 277 | + |
| 278 | + *gid = result->gr_gid; |
| 279 | + ret = 0; |
| 280 | + |
| 281 | +error: |
| 282 | + free(buffer); |
| 283 | + if (ret && warn && !warn_once) { |
| 284 | + WARN("No tracing group detected"); |
| 285 | + warn_once = 1; |
| 286 | } |
| 287 | - return grp->gr_gid; |
| 288 | + return ret; |
| 289 | } |
| 290 | |
| 291 | /* |
| 292 | diff --git a/src/common/utils.h b/src/common/utils.h |
| 293 | index 18f19ef1..9c72431d 100644 |
| 294 | --- a/src/common/utils.h |
| 295 | +++ b/src/common/utils.h |
| 296 | @@ -22,6 +22,8 @@ |
| 297 | #include <unistd.h> |
| 298 | #include <stdint.h> |
| 299 | #include <getopt.h> |
| 300 | +#include <stdbool.h> |
| 301 | +#include <sys/types.h> |
| 302 | |
| 303 | #define KIBI_LOG2 10 |
| 304 | #define MEBI_LOG2 20 |
| 305 | @@ -52,7 +54,7 @@ int utils_get_count_order_u64(uint64_t x); |
| 306 | char *utils_get_home_dir(void); |
| 307 | char *utils_get_user_home_dir(uid_t uid); |
| 308 | size_t utils_get_current_time_str(const char *format, char *dst, size_t len); |
| 309 | -gid_t utils_get_group_id(const char *name); |
| 310 | +int utils_get_group_id(const char *name, bool warn, gid_t *gid); |
| 311 | char *utils_generate_optstring(const struct option *long_options, |
| 312 | size_t opt_count); |
| 313 | int utils_create_lock_file(const char *filepath); |
| 314 | diff --git a/src/lib/lttng-ctl/lttng-ctl.c b/src/lib/lttng-ctl/lttng-ctl.c |
| 315 | index 2d84aad9..561b0bcf 100644 |
| 316 | --- a/src/lib/lttng-ctl/lttng-ctl.c |
| 317 | +++ b/src/lib/lttng-ctl/lttng-ctl.c |
| 318 | @@ -208,15 +208,13 @@ end: |
| 319 | LTTNG_HIDDEN |
| 320 | int lttng_check_tracing_group(void) |
| 321 | { |
| 322 | - struct group *grp_tracing; /* no free(). See getgrnam(3) */ |
| 323 | - gid_t *grp_list; |
| 324 | + gid_t *grp_list, tracing_gid; |
| 325 | int grp_list_size, grp_id, i; |
| 326 | int ret = -1; |
| 327 | const char *grp_name = tracing_group; |
| 328 | |
| 329 | /* Get GID of group 'tracing' */ |
| 330 | - grp_tracing = getgrnam(grp_name); |
| 331 | - if (!grp_tracing) { |
| 332 | + if (utils_get_group_id(grp_name, false, &tracing_gid)) { |
| 333 | /* If grp_tracing is NULL, the group does not exist. */ |
| 334 | goto end; |
| 335 | } |
| 336 | @@ -241,7 +239,7 @@ int lttng_check_tracing_group(void) |
| 337 | } |
| 338 | |
| 339 | for (i = 0; i < grp_list_size; i++) { |
| 340 | - if (grp_list[i] == grp_tracing->gr_gid) { |
| 341 | + if (grp_list[i] == tracing_gid) { |
| 342 | ret = 1; |
| 343 | break; |
| 344 | } |
| 345 | -- |
| 346 | 2.17.1 |
| 347 | |