From 7874f1d7182d80eb72c699eaa9ab8cc4cfec95ab Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Thu, 18 Dec 2014 13:36:25 -0800 Subject: [PATCH] Split the shared group data from the shared passwd data. Found by the toybox id(1) which calls both getpwuid(3) and getgrgid(3) before looking at either result. The use of a shared buffer in this code meant that even on a single thread, the data for any of the passwd functions would be clobbered by the data for any of the group functions (or vice versa). This might seem like an insufficient fix, but POSIX explicitly says (for getpwnam) that the result "might be overwritten by a subsequent call to getpwent(), getpwnam(), or getpwuid()" and likewise for other members of that group, plus equivalent text for the group-related functions. Change-Id: I2272f47e91f72e043fdaf7c169fa9f6978ff4370 --- libc/bionic/stubs.cpp | 82 ++++++++++++++++++++++----------------- libc/private/bionic_tls.h | 5 ++- 2 files changed, 50 insertions(+), 37 deletions(-) diff --git a/libc/bionic/stubs.cpp b/libc/bionic/stubs.cpp index 88e5ac5ed..ab6793556 100644 --- a/libc/bionic/stubs.cpp +++ b/libc/bionic/stubs.cpp @@ -42,18 +42,44 @@ #include "private/libc_logging.h" #include "private/ThreadLocalBuffer.h" -GLOBAL_INIT_THREAD_LOCAL_BUFFER(stubs); +// POSIX seems to envisage an implementation where the functions are +// implemented by brute-force searching with getpwent(3), and the +// functions are implemented similarly with getgrent(3). This means that it's +// okay for all the functions to share state, and all the +// functions to share state, but functions can't clobber +// functions' state and vice versa. -struct stubs_state_t { - passwd passwd_; +GLOBAL_INIT_THREAD_LOCAL_BUFFER(group); + +struct group_state_t { group group_; char* group_members_[2]; - char app_name_buffer_[32]; char group_name_buffer_[32]; +}; + +static group_state_t* __group_state() { + LOCAL_INIT_THREAD_LOCAL_BUFFER(group_state_t*, group, sizeof(group_state_t)); + if (group_tls_buffer != NULL) { + memset(group_tls_buffer, 0, sizeof(group_state_t)); + group_tls_buffer->group_.gr_mem = group_tls_buffer->group_members_; + } + return group_tls_buffer; +} + +GLOBAL_INIT_THREAD_LOCAL_BUFFER(passwd); + +struct passwd_state_t { + passwd passwd_; + char app_name_buffer_[32]; char dir_buffer_[32]; char sh_buffer_[32]; }; +static passwd_state_t* __passwd_state() { + LOCAL_INIT_THREAD_LOCAL_BUFFER(passwd_state_t*, passwd, sizeof(passwd_state_t)); + return passwd_tls_buffer; +} + static int do_getpw_r(int by_name, const char* name, uid_t uid, passwd* dst, char* buf, size_t byte_count, passwd** result) { @@ -91,7 +117,7 @@ static int do_getpw_r(int by_name, const char* name, uid_t uid, // pw_passwd and pw_gecos are non-POSIX and unused (always NULL) in bionic. dst->pw_passwd = NULL; -#ifdef __LP64__ +#if defined(__LP64__) dst->pw_gecos = NULL; #endif @@ -113,17 +139,7 @@ int getpwuid_r(uid_t uid, passwd* pwd, return do_getpw_r(0, NULL, uid, pwd, buf, byte_count, result); } -static stubs_state_t* __stubs_state() { - LOCAL_INIT_THREAD_LOCAL_BUFFER(stubs_state_t*, stubs, sizeof(stubs_state_t)); - - if (stubs_tls_buffer != NULL) { - memset(stubs_tls_buffer, 0, sizeof(stubs_state_t)); - stubs_tls_buffer->group_.gr_mem = stubs_tls_buffer->group_members_; - } - return stubs_tls_buffer; -} - -static passwd* android_iinfo_to_passwd(stubs_state_t* state, +static passwd* android_iinfo_to_passwd(passwd_state_t* state, const android_id_info* iinfo) { snprintf(state->dir_buffer_, sizeof(state->dir_buffer_), "/"); snprintf(state->sh_buffer_, sizeof(state->sh_buffer_), "/system/bin/sh"); @@ -145,7 +161,7 @@ static group* android_iinfo_to_group(group* gr, return gr; } -static passwd* android_id_to_passwd(stubs_state_t* state, unsigned id) { +static passwd* android_id_to_passwd(passwd_state_t* state, unsigned id) { for (size_t n = 0; n < android_id_count; ++n) { if (android_ids[n].aid == id) { return android_iinfo_to_passwd(state, android_ids + n); @@ -154,7 +170,7 @@ static passwd* android_id_to_passwd(stubs_state_t* state, unsigned id) { return NULL; } -static passwd* android_name_to_passwd(stubs_state_t* state, const char* name) { +static passwd* android_name_to_passwd(passwd_state_t* state, const char* name) { for (size_t n = 0; n < android_id_count; ++n) { if (!strcmp(android_ids[n].name, name)) { return android_iinfo_to_passwd(state, android_ids + n); @@ -297,9 +313,7 @@ static void print_app_name_from_gid(const gid_t gid, char* buffer, const int buf // AID_ISOLATED_START to AID_USER-1 -> u0_i1234 // AID_USER+ -> u1_radio, u1_a1234, u2_i1234, etc. // returns a passwd structure (sets errno to ENOENT on failure). -static passwd* app_id_to_passwd(uid_t uid, stubs_state_t* state) { - passwd* pw = &state->passwd_; - +static passwd* app_id_to_passwd(uid_t uid, passwd_state_t* state) { if (uid < AID_APP) { errno = ENOENT; return NULL; @@ -316,18 +330,18 @@ static passwd* app_id_to_passwd(uid_t uid, stubs_state_t* state) { snprintf(state->sh_buffer_, sizeof(state->sh_buffer_), "/system/bin/sh"); + passwd* pw = &state->passwd_; pw->pw_name = state->app_name_buffer_; pw->pw_dir = state->dir_buffer_; pw->pw_shell = state->sh_buffer_; pw->pw_uid = uid; pw->pw_gid = uid; - return pw; } // Translate a gid into the corresponding app_ // group structure (sets errno to ENOENT on failure). -static group* app_id_to_group(gid_t gid, stubs_state_t* state) { +static group* app_id_to_group(gid_t gid, group_state_t* state) { if (gid < AID_APP) { errno = ENOENT; return NULL; @@ -339,13 +353,11 @@ static group* app_id_to_group(gid_t gid, stubs_state_t* state) { gr->gr_name = state->group_name_buffer_; gr->gr_gid = gid; gr->gr_mem[0] = gr->gr_name; - return gr; } - passwd* getpwuid(uid_t uid) { // NOLINT: implementing bad function. - stubs_state_t* state = __stubs_state(); + passwd_state_t* state = __passwd_state(); if (state == NULL) { return NULL; } @@ -358,7 +370,7 @@ passwd* getpwuid(uid_t uid) { // NOLINT: implementing bad function. } passwd* getpwnam(const char* login) { // NOLINT: implementing bad function. - stubs_state_t* state = __stubs_state(); + passwd_state_t* state = __passwd_state(); if (state == NULL) { return NULL; } @@ -372,12 +384,12 @@ passwd* getpwnam(const char* login) { // NOLINT: implementing bad function. // All users are in just one group, the one passed in. int getgrouplist(const char* /*user*/, gid_t group, gid_t* groups, int* ngroups) { - if (*ngroups < 1) { - *ngroups = 1; - return -1; - } - groups[0] = group; - return (*ngroups = 1); + if (*ngroups < 1) { + *ngroups = 1; + return -1; + } + groups[0] = group; + return (*ngroups = 1); } char* getlogin() { // NOLINT: implementing bad function. @@ -386,7 +398,7 @@ char* getlogin() { // NOLINT: implementing bad function. } group* getgrgid(gid_t gid) { // NOLINT: implementing bad function. - stubs_state_t* state = __stubs_state(); + group_state_t* state = __group_state(); if (state == NULL) { return NULL; } @@ -399,7 +411,7 @@ group* getgrgid(gid_t gid) { // NOLINT: implementing bad function. } group* getgrnam(const char* name) { // NOLINT: implementing bad function. - stubs_state_t* state = __stubs_state(); + group_state_t* state = __group_state(); if (state == NULL) { return NULL; } diff --git a/libc/private/bionic_tls.h b/libc/private/bionic_tls.h index 4a3516683..944f95790 100644 --- a/libc/private/bionic_tls.h +++ b/libc/private/bionic_tls.h @@ -85,7 +85,8 @@ enum { * ttyname libc (GLOBAL_INIT_THREAD_LOCAL_BUFFER) * strerror libc (GLOBAL_INIT_THREAD_LOCAL_BUFFER) * strsignal libc (GLOBAL_INIT_THREAD_LOCAL_BUFFER) - * stubs libc (GLOBAL_INIT_THREAD_LOCAL_BUFFER) + * passwd libc (GLOBAL_INIT_THREAD_LOCAL_BUFFER) + * group libc (GLOBAL_INIT_THREAD_LOCAL_BUFFER) * _res_key libc * je_thread_allocated_tsd jemalloc * je_arenas_tsd jemalloc @@ -95,7 +96,7 @@ enum { * */ -#define LIBC_TLS_RESERVED_SLOTS 11 +#define LIBC_TLS_RESERVED_SLOTS 12 #if defined(USE_JEMALLOC) /* jemalloc uses 5 keys for itself. */