From 360c8d1a18f80451d88191380a199816ef12d7ac Mon Sep 17 00:00:00 2001 From: Kihwal Lee Date: Mon, 21 Apr 2014 18:27:16 +0000 Subject: [PATCH] HADOOP-10522. JniBasedUnixGroupMapping mishandles errors. Contributed by Kihwal Lee. git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1588949 13f79535-47bb-0310-9956-ffa450edef68 --- .../hadoop-common/CHANGES.txt | 2 + .../security/JniBasedUnixGroupsMapping.c | 14 ++++- .../hadoop/security/hadoop_group_info.c | 55 +++++++++++++------ .../apache/hadoop/security/hadoop_user_info.c | 54 ++++++++++++------ 4 files changed, 86 insertions(+), 39 deletions(-) diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index a04f51d57fd..3d7752a41a7 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/hadoop-common-project/hadoop-common/CHANGES.txt @@ -423,6 +423,8 @@ Release 2.4.1 - UNRELEASED HADOOP-10490. TestMapFile and TestBloomMapFile leak file descriptors. (cnauroth) + HADOOP-10522. JniBasedUnixGroupMapping mishandles errors. (kihwal) + Release 2.4.0 - 2014-04-07 INCOMPATIBLE CHANGES diff --git a/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c b/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c index 55adb59bb58..72f092959a3 100644 --- a/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c +++ b/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c @@ -120,10 +120,18 @@ Java_org_apache_hadoop_security_JniBasedUnixGroupsMapping_getGroupsForUser goto done; } ret = hadoop_user_info_fetch(uinfo, username); - if (ret == ENOENT) { - jgroups = (*env)->NewObjectArray(env, 0, g_string_clazz, NULL); + if (ret) { + if (ret == ENOENT) { + jgroups = (*env)->NewObjectArray(env, 0, g_string_clazz, NULL); + } else { // handle other errors + char buf[128]; + snprintf(buf, sizeof(buf), "getgrouplist: error looking up user. %d (%s)", + ret, terror(ret)); + THROW(env, "java/lang/RuntimeException", buf); + } goto done; } + ginfo = hadoop_group_info_alloc(); if (!ginfo) { THROW(env, "java/lang/OutOfMemoryError", NULL); @@ -135,7 +143,7 @@ Java_org_apache_hadoop_security_JniBasedUnixGroupsMapping_getGroupsForUser THROW(env, "java/lang/OutOfMemoryError", NULL); } else { char buf[128]; - snprintf(buf, sizeof(buf), "getgrouplist error %d (%s)", + snprintf(buf, sizeof(buf), "getgrouplist: error looking up groups. %d (%s)", ret, terror(ret)); THROW(env, "java/lang/RuntimeException", buf); } diff --git a/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.c b/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.c index 5d269fe38d1..a671528587b 100644 --- a/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.c +++ b/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.c @@ -27,6 +27,8 @@ #include #include +#define MAX_GROUP_LOOKUP_TRIES 5 + struct hadoop_group_info *hadoop_group_info_alloc(void) { struct hadoop_group_info *ginfo; @@ -84,31 +86,48 @@ static int getgrgid_error_translate(int err) int hadoop_group_info_fetch(struct hadoop_group_info *ginfo, gid_t gid) { struct group *group; - int err; + int ret, i; size_t buf_sz; char *nbuf; hadoop_group_info_clear(ginfo); - for (;;) { - do { - group = NULL; - err = getgrgid_r(gid, &ginfo->group, ginfo->buf, - ginfo->buf_sz, &group); - } while ((!group) && (err == EINTR)); - if (group) { - return 0; + for (i = 0, ret = 0; i < MAX_GROUP_LOOKUP_TRIES; i++) { + // If the previous call returned ERANGE, increase the buffer size + if (ret == ERANGE) { + buf_sz = ginfo->buf_sz * 2; + nbuf = realloc(ginfo->buf, buf_sz); + if (!nbuf) { + return ENOMEM; + } + ginfo->buf = nbuf; + ginfo->buf_sz = buf_sz; } - if (err != ERANGE) { - return getgrgid_error_translate(errno); + + // The following call returns errno. Reading the global errno wihtout + // locking is not thread-safe. + group = NULL; + ret = getgrgid_r(gid, &ginfo->group, ginfo->buf, + ginfo->buf_sz, &group); + switch(ret) { + case 0: + if (!group) { + // The underlying library likely has a bug. + return EIO; + } + return 0; + case EINTR: + case ERANGE: + // Retry on these errors. + // EINTR: a signal was handled and this thread was allowed to continue. + // ERANGE: the buffer was not big enough. + break; + default: + // Lookup failed. + return getgrgid_error_translate(ret); } - buf_sz = ginfo->buf_sz * 2; - nbuf = realloc(ginfo->buf, buf_sz); - if (!nbuf) { - return ENOMEM; - } - ginfo->buf = nbuf; - ginfo->buf_sz = buf_sz; } + // Did not succeed after the retries. Return the last error. + return getgrgid_error_translate(ret); } #ifdef GROUP_TESTING diff --git a/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.c b/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.c index 1bd7bdc6c1e..e8405d7d4bb 100644 --- a/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.c +++ b/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.c @@ -28,6 +28,7 @@ #include #define INITIAL_GIDS_SIZE 32 +#define MAX_USER_LOOKUP_TRIES 5 struct hadoop_user_info *hadoop_user_info_alloc(void) { @@ -95,31 +96,48 @@ int hadoop_user_info_fetch(struct hadoop_user_info *uinfo, const char *username) { struct passwd *pwd; - int err; + int ret, i; size_t buf_sz; char *nbuf; hadoop_user_info_clear(uinfo); - for (;;) { - do { - pwd = NULL; - err = getpwnam_r(username, &uinfo->pwd, uinfo->buf, + for (i = 0, ret = 0; i < MAX_USER_LOOKUP_TRIES; i++) { + // If the previous call returned ERANGE, increase the buffer size + if (ret == ERANGE) { + buf_sz = uinfo->buf_sz * 2; + nbuf = realloc(uinfo->buf, buf_sz); + if (!nbuf) { + return ENOMEM; + } + uinfo->buf = nbuf; + uinfo->buf_sz = buf_sz; + } + + // The following call returns errno. Reading the global errno wihtout + // locking is not thread-safe. + pwd = NULL; + ret = getpwnam_r(username, &uinfo->pwd, uinfo->buf, uinfo->buf_sz, &pwd); - } while ((!pwd) && (errno == EINTR)); - if (pwd) { - return 0; + switch(ret) { + case 0: + if (!pwd) { + // The underlying library likely has a bug. + return EIO; + } + return 0; + case EINTR: + case ERANGE: + // Retry on these errors. + // EINTR: a signal was handled and this thread was allowed to continue. + // ERANGE: the buffer was not big enough. + break; + default: + // Lookup failed. + return getpwnam_error_translate(ret); } - if (err != ERANGE) { - return getpwnam_error_translate(errno); - } - buf_sz = uinfo->buf_sz * 2; - nbuf = realloc(uinfo->buf, buf_sz); - if (!nbuf) { - return ENOMEM; - } - uinfo->buf = nbuf; - uinfo->buf_sz = buf_sz; } + // Did not succeed after the retries. Return the last error. + return getpwnam_error_translate(ret); } static int put_primary_gid_first(struct hadoop_user_info *uinfo)