Hadoop Common
  1. Hadoop Common
  2. HADOOP-9439

JniBasedUnixGroupsMapping: fix some crash bugs

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.4-alpha
    • Fix Version/s: 3.0.0, 2.1.0-beta
    • Component/s: native
    • Labels:
      None

      Description

      JniBasedUnixGroupsMapping has some issues.

      • sometimes on error paths variables are freed prior to being initialized
      • re-allocate buffers less frequently (can reuse the same buffer for multiple calls to getgrnam)
      • allow non-reentrant functions to be used, to work around client bugs
      • don't throw IOException from JNI functions if the JNI functions do not declare this checked exception.
      • don't bail out if only one group name among all the ones associated with a user can't be looked up.
      1. HADOOP-9439.008.patch
        36 kB
        Chris Nauroth
      2. HADOOP-9439.007.patch
        34 kB
        Colin Patrick McCabe
      3. HADOOP-9439.006.patch
        34 kB
        Colin Patrick McCabe
      4. HADOOP-9439.005.patch
        34 kB
        Colin Patrick McCabe
      5. HADOOP-9439.003.patch
        37 kB
        Colin Patrick McCabe
      6. HADOOP-9439.001.patch
        35 kB
        Colin Patrick McCabe
      7. HDFS-4640.002.patch
        32 kB
        Colin Patrick McCabe

        Issue Links

          Activity

          Hide
          Colin Patrick McCabe added a comment -

          I should explain the error path comment a bit. When you have something like this:

          if (error)
              goto done;
          MyFoo *foo = NULL;
          done:
              free(foo);
          

          The compiler treats foo as uninitialized on the error path. It is best to move the declarations of stuff that gets freed at the end to the top to avoid this problem.

          Show
          Colin Patrick McCabe added a comment - I should explain the error path comment a bit. When you have something like this: if (error) goto done; MyFoo *foo = NULL; done: free(foo); The compiler treats foo as uninitialized on the error path. It is best to move the declarations of stuff that gets freed at the end to the top to avoid this problem.
          Hide
          Aaron T. Myers added a comment -

          Seems like we should move this JIRA to Common, no? The groups mapping stuff is used by YARN as well, and none of these changes are HDFS-specific.

          Show
          Aaron T. Myers added a comment - Seems like we should move this JIRA to Common, no? The groups mapping stuff is used by YARN as well, and none of these changes are HDFS-specific.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12575606/HDFS-4640.002.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/4151//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/4151//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12575606/HDFS-4640.002.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/4151//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/4151//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -

          moved this to common at ATM's suggestion.

          Show
          Colin Patrick McCabe added a comment - moved this to common at ATM's suggestion.
          Hide
          Colin Patrick McCabe added a comment -

          add the ability to configure JniBasedUnixGroupsMapping to use the non-reentrant getpwent and getgrent functions at runtime. They are buggy on some implementations.

          pass in an empty array in a threadsafe manner, duplicating the optimization we had before where we return a pre-allocated empty array when there are no groups to be found.

          Show
          Colin Patrick McCabe added a comment - add the ability to configure JniBasedUnixGroupsMapping to use the non-reentrant getpwent and getgrent functions at runtime. They are buggy on some implementations. pass in an empty array in a threadsafe manner, duplicating the optimization we had before where we return a pre-allocated empty array when there are no groups to be found.
          Hide
          Chris Nauroth added a comment -

          Hi, Colin. I had filed HADOOP-9312 for a potential memory leak around the lazy initialization of emptyGroups. From my quick scan of the patch here, it looks like this will fix it (at least for the non-Windows codebase). Do you agree? If so, I will relate the 2 jiras, or perhaps file a new jira for porting this patch to the Windows version. Thanks!

          Show
          Chris Nauroth added a comment - Hi, Colin. I had filed HADOOP-9312 for a potential memory leak around the lazy initialization of emptyGroups . From my quick scan of the patch here, it looks like this will fix it (at least for the non-Windows codebase). Do you agree? If so, I will relate the 2 jiras, or perhaps file a new jira for porting this patch to the Windows version. Thanks!
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12575606/HDFS-4640.002.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2371//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2371//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12575606/HDFS-4640.002.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2371//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2371//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -

          yeah, this will definitely fix the memory leak identified in HADOOP-9312.

          Show
          Colin Patrick McCabe added a comment - yeah, this will definitely fix the memory leak identified in HADOOP-9312 .
          Hide
          Todd Lipcon added a comment -
          +  static String empty[] = new String[0];
          

          Should probably be static final, right? And rename to NO_GROUPS or EMPTY_STIRNG_ARRAY or something a little more descriptive


          +  native static String[] getGroupsForUser(String username, String empty[],
          +        boolean reentrant);
          

          I don't get why empty is a parameter here... aside from it being a premature optimization (is allocating an empty array that bad?), it seems like the JNI code could just grab this field itself and return a reference, rather than taking it as a parameter.


          +      groups = getGroupsForUser(user, empty, useReentrant );
          

          Style nit: extra space here before ')'


          +      snprintf(buf, sizeof(buf), "getgrouplist error %d", ret);
          +      THROW(env, "java/lang/RuntimeException", buf);
          

          Can we use strerror here on the return, assuming it's probably a standard errnum?


          +    ret = hadoop_group_info_fetch(ginfo, uinfo->gids[i]);
          +    if (!ret) {
          

          Here if the group info lookup fails for a particular gid, you end up swallowing the error without any warnings, etc. Maybe instead we should just return the numeric gid as a group? This is what the 'groups' shell command does:

          todd@todd-w510:~$ groups todd
          todd : groups: cannot find name for group ID 1050
          1050 adm dialout cdrom audio video plugdev lpadmin admin sambashare mrtest wireshark
          

          • I noticed you use different locks for getgrnam and getpwnam. But when I was working on HADOOP-7156, I found that the buggy implementations were racey in other parts of their code – ie a concurrent getgrnam and a concurrent getpwnam might race with eachother and cause either to fail. I know that you added this function to deal with a crash we saw on a production cluster - did you verify that with the same buggy underlying implementation that the workaround prevents the issue?

          We may have to actually share this lock and configuration with the lock used in NativeIO.c.

          Additionally, when I did that patch, I found it simpler to continue to use the reentrant functions wrapped with a lock – less repeated code, etc.

          Show
          Todd Lipcon added a comment - + static String empty[] = new String [0]; Should probably be static final, right? And rename to NO_GROUPS or EMPTY_STIRNG_ARRAY or something a little more descriptive + native static String [] getGroupsForUser( String username, String empty[], + boolean reentrant); I don't get why empty is a parameter here... aside from it being a premature optimization (is allocating an empty array that bad?), it seems like the JNI code could just grab this field itself and return a reference, rather than taking it as a parameter. + groups = getGroupsForUser(user, empty, useReentrant ); Style nit: extra space here before ')' + snprintf(buf, sizeof(buf), "getgrouplist error %d" , ret); + THROW(env, "java/lang/RuntimeException" , buf); Can we use strerror here on the return, assuming it's probably a standard errnum? + ret = hadoop_group_info_fetch(ginfo, uinfo->gids[i]); + if (!ret) { Here if the group info lookup fails for a particular gid, you end up swallowing the error without any warnings, etc. Maybe instead we should just return the numeric gid as a group? This is what the 'groups' shell command does: todd@todd-w510:~$ groups todd todd : groups: cannot find name for group ID 1050 1050 adm dialout cdrom audio video plugdev lpadmin admin sambashare mrtest wireshark I noticed you use different locks for getgrnam and getpwnam. But when I was working on HADOOP-7156 , I found that the buggy implementations were racey in other parts of their code – ie a concurrent getgrnam and a concurrent getpwnam might race with eachother and cause either to fail. I know that you added this function to deal with a crash we saw on a production cluster - did you verify that with the same buggy underlying implementation that the workaround prevents the issue? We may have to actually share this lock and configuration with the lock used in NativeIO.c. Additionally, when I did that patch, I found it simpler to continue to use the reentrant functions wrapped with a lock – less repeated code, etc.
          Hide
          Colin Patrick McCabe added a comment -

          I don't get why empty is a parameter here... aside from it being a premature optimization (is allocating an empty array that bad?), it seems like the JNI code could just grab this field itself and return a reference, rather than taking it as a parameter.

          This optimization was in the existing code, so I kept it in. But actually, looking at it again, I think you're right. We might as well take it out.

          Style nit: extra space here before getGroupsForUser

          ok.

          Can we use strerror here on the return, assuming it's probably a standard errnum?

          good idea.

          + ret = hadoop_group_info_fetch(ginfo, uinfo->gids[i]);
          + if (!ret) {

          Here if the group info lookup fails for a particular gid, you end up swallowing the error without any warnings, etc. Maybe instead we should just return the numeric gid as a group? This is what the 'groups' shell command does:

          The 'groups' shell command returns nonzero if it can't look up a group (at least according to the internet; I didn't want to mess up my groups settings to test). That would cause ShellBasedUnixGroupsMapping to get "got exception trying to get groups for user." So as far as I know, there is no scenario currently where we treat the numeric gid as a group name.

          We could be bug-compatible with ShellBasedUnixGroupsMapping, and refuse to look up any groups if one fails. However, that doesn't seem like a good idea. Instead, I added some code that fires off a log4j message in that scenario, and continues with the other groups.

          We may have to actually share this lock and configuration with the lock used in NativeIO.c.

          yeah, let's do that.

          Additionally, when I did that patch, I found it simpler to continue to use the reentrant functions wrapped with a lock – less repeated code, etc.

          ok.

          Show
          Colin Patrick McCabe added a comment - I don't get why empty is a parameter here... aside from it being a premature optimization (is allocating an empty array that bad?), it seems like the JNI code could just grab this field itself and return a reference, rather than taking it as a parameter. This optimization was in the existing code, so I kept it in. But actually, looking at it again, I think you're right. We might as well take it out. Style nit: extra space here before getGroupsForUser ok. Can we use strerror here on the return, assuming it's probably a standard errnum? good idea. + ret = hadoop_group_info_fetch(ginfo, uinfo->gids [i] ); + if (!ret) { Here if the group info lookup fails for a particular gid, you end up swallowing the error without any warnings, etc. Maybe instead we should just return the numeric gid as a group? This is what the 'groups' shell command does: The 'groups' shell command returns nonzero if it can't look up a group (at least according to the internet; I didn't want to mess up my groups settings to test). That would cause ShellBasedUnixGroupsMapping to get "got exception trying to get groups for user." So as far as I know, there is no scenario currently where we treat the numeric gid as a group name. We could be bug-compatible with ShellBasedUnixGroupsMapping, and refuse to look up any groups if one fails. However, that doesn't seem like a good idea. Instead, I added some code that fires off a log4j message in that scenario, and continues with the other groups. We may have to actually share this lock and configuration with the lock used in NativeIO.c. yeah, let's do that. Additionally, when I did that patch, I found it simpler to continue to use the reentrant functions wrapped with a lock – less repeated code, etc. ok.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12583352/HADOOP-9439.003.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2544//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2544//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12583352/HADOOP-9439.003.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2544//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2544//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          It looks like you've inverted the meaning of the boolean in some places here:

          +   * @param reentrant        True if we should use the reentrant versions of
          +   *                         getgrent, getpwent, etc.  They are faster, but
          +   *                         buggy in some implementations.
          +   *
          +   * @return                 The set of groups associated with a user.
          +   */
          +  native static String[] getGroupsForUser(String username, boolean reentrant);
          

          suggests that the second parameter being 'false' disables locking. But then:

             public List<String> getGroups(String user) throws IOException {
               String[] groups = new String[0];
               try {
          -      groups = getGroupForUser(user);
          +      groups = getGroupsForUser(user, removeConcurrency);
          

          passes 'true' if you want to disable locking.

          The native implementation seems to agree with the latter (i.e that passing true will introduce the locking)

          +  static private void logError(int groupIdx, String error) {
          +    LOG.error("error looking up the name of group " + groupIdx + ": " + error);
          

          This parameter should be 'groupId' not 'groupIdx'

          +  g_log_error_method = (*env)->GetStaticMethodID(env, clazz, "logError",
          +        "(ILjava/lang/String;)V");
          +  if (!g_log_error_method) {
          +    jthrowable jthr = newRuntimeException(env,
          +        "JniBasedUnixGroupsMapping#anchorNative: failed to look "
          +        "up JniBasedUnixGroupsMapping#logError method\n");
          +    (*env)->Throw(env, jthr);
          

          No need to throw an exception here - GetStaticMethodID already throws NoSuchMethodError if it fails. Same with the FindClass call below, and I assume NewGlobalRef as well (at least I've never seen this pattern of checking the result of NewGlobalRef).

          +  error_msg = (*env)->NewStringUTF(env, terror(ret));
          +  if (!error_msg) {
          +    (*env)->ExceptionClear(env);
          +    return;
          +  }
          

          Why are you ignoring exceptions in this method? Add a comment explaining this.

          • In general, you don't need to DeleteLocalRef inside short-lived JNI methods. It just adds clutter to the code – they're automatically deleted at the end of the method. It's only important if you plan on doing a lot of allocation/freeing inside the method and need to let GC collect the stuff before the method returns.
          • Can you explain how you tested the various error code paths here in the JNI? eg did you create a user which has some invalid groups? I'm nervous about missing some bug until we hit it in production.
          Show
          Todd Lipcon added a comment - It looks like you've inverted the meaning of the boolean in some places here: + * @param reentrant True if we should use the reentrant versions of + * getgrent, getpwent, etc. They are faster, but + * buggy in some implementations. + * + * @ return The set of groups associated with a user. + */ + native static String [] getGroupsForUser( String username, boolean reentrant); suggests that the second parameter being 'false' disables locking. But then: public List< String > getGroups( String user) throws IOException { String [] groups = new String [0]; try { - groups = getGroupForUser(user); + groups = getGroupsForUser(user, removeConcurrency); passes 'true' if you want to disable locking. The native implementation seems to agree with the latter (i.e that passing true will introduce the locking) + static private void logError( int groupIdx, String error) { + LOG.error( "error looking up the name of group " + groupIdx + ": " + error); This parameter should be 'groupId' not 'groupIdx' + g_log_error_method = (*env)->GetStaticMethodID(env, clazz, "logError" , + "(ILjava/lang/ String ;)V" ); + if (!g_log_error_method) { + jthrowable jthr = newRuntimeException(env, + "JniBasedUnixGroupsMapping#anchorNative: failed to look " + "up JniBasedUnixGroupsMapping#logError method\n" ); + (*env)->Throw(env, jthr); No need to throw an exception here - GetStaticMethodID already throws NoSuchMethodError if it fails. Same with the FindClass call below, and I assume NewGlobalRef as well (at least I've never seen this pattern of checking the result of NewGlobalRef). + error_msg = (*env)->NewStringUTF(env, terror(ret)); + if (!error_msg) { + (*env)->ExceptionClear(env); + return ; + } Why are you ignoring exceptions in this method? Add a comment explaining this. In general, you don't need to DeleteLocalRef inside short-lived JNI methods. It just adds clutter to the code – they're automatically deleted at the end of the method. It's only important if you plan on doing a lot of allocation/freeing inside the method and need to let GC collect the stuff before the method returns. Can you explain how you tested the various error code paths here in the JNI? eg did you create a user which has some invalid groups? I'm nervous about missing some bug until we hit it in production.
          Hide
          Todd Lipcon added a comment -

          Also, looks like you missed the following:

          <Todd> We may have to actually share this lock and configuration with the lock used in NativeIO.c.

          <Colin> yeah, let's do that.

          Instead you're just sharing between the user_info and group_info, but not the lock used by NativeIO.c (which is actually a java monitor lock rather than a pthread mutex).

          Given the above, why not something like the following:

          • move the lock object from NativeIO.c to be defined in the NativeIO.java class as a static field
          • change the Java code in both places to just do something like:
          if (shouldLock) {
            synchronized (lockObject) {
              return myJniCall(...);
            }
          } else {
            return myJniCall(...);
          }
          

          Would that end up simpler?

          Show
          Todd Lipcon added a comment - Also, looks like you missed the following: <Todd> We may have to actually share this lock and configuration with the lock used in NativeIO.c. <Colin> yeah, let's do that. Instead you're just sharing between the user_info and group_info, but not the lock used by NativeIO.c (which is actually a java monitor lock rather than a pthread mutex). Given the above, why not something like the following: move the lock object from NativeIO.c to be defined in the NativeIO.java class as a static field change the Java code in both places to just do something like: if (shouldLock) { synchronized (lockObject) { return myJniCall(...); } } else { return myJniCall(...); } Would that end up simpler?
          Hide
          Colin Patrick McCabe added a comment -

          Thanks, very thorough review.

          I fixed the naming of the parameters to getGroupsForUser and logError. Although it didn't affect correctness, it certainly was very confusing.

          You're right that GetStaticMethodID and FindClass throw exceptions on failure. There is no need to throw another one (and it is probably actually harmful). Thanks for finding that. However, NewGlobalRef does not throw an exception, but merely returns NULL when you're out of memory.

          I will add a comment clarifying why we ignore exceptions in logError.

          I agree that we should probably just use the existing monitor lock from NativeIO.java. That way, I don't have to modify that code. I wasn't aware that there were other folks poking the user/group functions in Hadoop. Right now, it looks like NativeIO#getUserName is only called from tests calling NativeIO#POSIX#getFstat, but of course that may change in the future.

          Invalid groups are a sore point for ShellBasedUnixGroupsMapping. If any invalid groups are associated with a user, the "groups" program will fail with a non-zero return code, and no information is returned. For JniBasedUnixGroupsMapping, I would prefer to return the groups that were valid, rather than nothing at all. I suppose this is debatable, though. I can test creating such invalid groups.

          I understand that sometimes it's unnecessary, but I'd rather have DeleteLocalRef used for all allocations. For one thing, in libhdfs, it really is necessary everywhere (the JNI invocation API never automatically disposes of Java references that are made in the invoking C code). It confuses people when they copy a piece of code from one part of the source tree to another and it suddenly becomes incorrect. For another thing, the spec only says that the JVM has to provide at least 16 local references at once, which is not very many at all. It's only one line of overhead per Java reference, and DeleteLocalRef already ignores NULLs, so I'd rather just have a consistent style everywhere, than try to be clever.

          Show
          Colin Patrick McCabe added a comment - Thanks, very thorough review. I fixed the naming of the parameters to getGroupsForUser and logError. Although it didn't affect correctness, it certainly was very confusing. You're right that GetStaticMethodID and FindClass throw exceptions on failure. There is no need to throw another one (and it is probably actually harmful). Thanks for finding that. However, NewGlobalRef does not throw an exception, but merely returns NULL when you're out of memory. I will add a comment clarifying why we ignore exceptions in logError. I agree that we should probably just use the existing monitor lock from NativeIO.java. That way, I don't have to modify that code. I wasn't aware that there were other folks poking the user/group functions in Hadoop. Right now, it looks like NativeIO#getUserName is only called from tests calling NativeIO#POSIX#getFstat , but of course that may change in the future. Invalid groups are a sore point for ShellBasedUnixGroupsMapping . If any invalid groups are associated with a user, the " groups " program will fail with a non-zero return code, and no information is returned. For JniBasedUnixGroupsMapping , I would prefer to return the groups that were valid, rather than nothing at all. I suppose this is debatable, though. I can test creating such invalid groups. I understand that sometimes it's unnecessary, but I'd rather have DeleteLocalRef used for all allocations. For one thing, in libhdfs, it really is necessary everywhere (the JNI invocation API never automatically disposes of Java references that are made in the invoking C code). It confuses people when they copy a piece of code from one part of the source tree to another and it suddenly becomes incorrect. For another thing, the spec only says that the JVM has to provide at least 16 local references at once, which is not very many at all. It's only one line of overhead per Java reference, and DeleteLocalRef already ignores NULLs, so I'd rather just have a consistent style everywhere, than try to be clever.
          Hide
          Colin Patrick McCabe added a comment -

          Invalid groups are a sore point for ShellBasedUnixGroupsMapping. If any invalid groups are associated with a user, the "groups" program will fail with a non-zero return code, and no information is returned

          Actually, I just tested the behavior of ShellBasedUnixGroupsMapping when a group can't be resolved. Apparently it just creates an empty array entry for that group, but returns all the others (at least on Linux.) I think the reason is because although the "groups" command has the behavior I described, we are now using the "id -Gn" command, which ignores illegal groups.

          I also verified that this patch works in the case where a group can't be resolved.

          Show
          Colin Patrick McCabe added a comment - Invalid groups are a sore point for ShellBasedUnixGroupsMapping. If any invalid groups are associated with a user, the "groups" program will fail with a non-zero return code, and no information is returned Actually, I just tested the behavior of ShellBasedUnixGroupsMapping when a group can't be resolved. Apparently it just creates an empty array entry for that group, but returns all the others (at least on Linux.) I think the reason is because although the "groups" command has the behavior I described, we are now using the "id -Gn" command, which ignores illegal groups. I also verified that this patch works in the case where a group can't be resolved.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12586769/HADOOP-9439.005.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2621//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2621//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12586769/HADOOP-9439.005.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2621//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2621//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -
          • seems like pw_lock_object is declared in two .c files... one should probably be marked extern
          • The following code is smelly to me:
          int i, ret, nvalid, pw_lock_locked = 0;
          

          because only the last variable is actually initialized. I don't think you end up using any of the uninitialized variables, so it's not a true bug, but I think it's worth separating them out into their own lines. Do we not allow C99 style declaration in the middle of a function in our JNI code? I've always liked that better than the original C style of declaring all at the top.

          Show
          Todd Lipcon added a comment - seems like pw_lock_object is declared in two .c files... one should probably be marked extern The following code is smelly to me: int i, ret, nvalid, pw_lock_locked = 0; because only the last variable is actually initialized. I don't think you end up using any of the uninitialized variables, so it's not a true bug, but I think it's worth separating them out into their own lines. Do we not allow C99 style declaration in the middle of a function in our JNI code? I've always liked that better than the original C style of declaring all at the top.
          Hide
          Colin Patrick McCabe added a comment -

          I don't like C99 style declarations because they tend to introduce bugs. When you have some cleanup label at the end of the function, if your goto passes over the variable declaration, the variable will be treated as undefined. So, for example, if you have:

          int i = 4;

          Show
          Colin Patrick McCabe added a comment - I don't like C99 style declarations because they tend to introduce bugs. When you have some cleanup label at the end of the function, if your goto passes over the variable declaration, the variable will be treated as undefined. So, for example, if you have: int i = 4;
          Hide
          Colin Patrick McCabe added a comment -

          ok, these keyboard shortcuts defeated me again. I wish I could turn that off. Anyway, if you have:

            goto cleanup:
            struct foo *foo= calloc(1, sizeof(*foo));
            ...
          cleanup:
            free(foo);
          

          then you're freeing an uninitialized pointer. Not good.

          I also don't like declarations in the middle because of a philosophical reason: it tends to lead to excessively long functions, rather than encouraging modularity.

          As to the extern thing, yeah, I agree. It's rather odd that the linker does that kind of symbol merging, but apparently it does.

          I don't see the point of putting all variable declarations on separate lines. Java allows declaration of variables on the same line as well. C+0x allows things like vector<> to be initialized with a reasonable syntax, so you are finally allowed to admit that this is a good thing, you C+ advocate, you.

          Show
          Colin Patrick McCabe added a comment - ok, these keyboard shortcuts defeated me again. I wish I could turn that off. Anyway, if you have: goto cleanup: struct foo *foo= calloc(1, sizeof(*foo)); ... cleanup: free(foo); then you're freeing an uninitialized pointer. Not good. I also don't like declarations in the middle because of a philosophical reason: it tends to lead to excessively long functions, rather than encouraging modularity. As to the extern thing, yeah, I agree. It's rather odd that the linker does that kind of symbol merging, but apparently it does. I don't see the point of putting all variable declarations on separate lines. Java allows declaration of variables on the same line as well. C+ 0x allows things like vector<> to be initialized with a reasonable syntax, so you are finally allowed to admit that this is a good thing, you C + advocate, you.
          Hide
          Todd Lipcon added a comment -

          I don't see the point of putting all variable declarations on separate lines. Java allows declaration of variables on the same line as well

          The point is that when a lot of people see int a, b, c = 0; they think that all three variables are getting set to 0, but in fact it's only c that is set. Putting on separate lines makes it more obvious – probably not important if you expect mostly veteran C programmers to read the code, but given that a lot of folks in the Hadoop community rarely look at C, I figured it would be more obvious this way.

          In Java, you always get a compile time error for accessing an uninitialized variable, whereas in C it's just a warning, which I'm certain we'll miss in the compile spew of a Hadoop build. So, probably better to explicitly initialize all the variables, whether on one line or multiple.

          Show
          Todd Lipcon added a comment - I don't see the point of putting all variable declarations on separate lines. Java allows declaration of variables on the same line as well The point is that when a lot of people see int a, b, c = 0; they think that all three variables are getting set to 0, but in fact it's only c that is set. Putting on separate lines makes it more obvious – probably not important if you expect mostly veteran C programmers to read the code, but given that a lot of folks in the Hadoop community rarely look at C, I figured it would be more obvious this way. In Java, you always get a compile time error for accessing an uninitialized variable, whereas in C it's just a warning, which I'm certain we'll miss in the compile spew of a Hadoop build. So, probably better to explicitly initialize all the variables, whether on one line or multiple.
          Hide
          Colin Patrick McCabe added a comment -
          • declare pw_lock_object as extern
          • put declaration of pw_lock_locked on a separate line
          Show
          Colin Patrick McCabe added a comment - declare pw_lock_object as extern put declaration of pw_lock_locked on a separate line
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12588505/HADOOP-9439.006.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2671//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2671//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12588505/HADOOP-9439.006.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2671//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2671//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -

          We've been seeing a lot of people hit segfaults that we believe result from non-threadsafe implementations of the getpwuid, etc interfaces. Let's change the default to locking.

          Show
          Colin Patrick McCabe added a comment - We've been seeing a lot of people hit segfaults that we believe result from non-threadsafe implementations of the getpwuid, etc interfaces. Let's change the default to locking.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12588656/HADOOP-9439.007.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

          org.apache.hadoop.ipc.TestSaslRPC

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2676//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2676//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12588656/HADOOP-9439.007.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.ipc.TestSaslRPC +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2676//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2676//console This message is automatically generated.
          Hide
          Chris Nauroth added a comment -

          The patch caused unsatisfied link errors on Windows. The problem was most easily visible as a test failure in TestJNIGroupsMapping.

          It would be valuable to port this to the Windows side. The Windows implementation was largely based on the prior code, so it's subject to the same problems, such as the problems listed in the description here and the memory leak I reported in HADOOP-9312. Unfortunately, I'm not available to do a full port and test it right now. (Any other volunteers?)

          Meanwhile, I'm uploading version 8 of the patch, which is the minimal work required to prevent breaking Windows. The only thing I changed in addition to Colin's patch is JniBasedUnixGroupsMappingWin.c. I handled the signature change on getGroupsForUser. I stubbed anchorNative to do nothing and left a comment explaining that we need the full port of this patch later.

          Show
          Chris Nauroth added a comment - The patch caused unsatisfied link errors on Windows. The problem was most easily visible as a test failure in TestJNIGroupsMapping . It would be valuable to port this to the Windows side. The Windows implementation was largely based on the prior code, so it's subject to the same problems, such as the problems listed in the description here and the memory leak I reported in HADOOP-9312 . Unfortunately, I'm not available to do a full port and test it right now. (Any other volunteers?) Meanwhile, I'm uploading version 8 of the patch, which is the minimal work required to prevent breaking Windows. The only thing I changed in addition to Colin's patch is JniBasedUnixGroupsMappingWin.c. I handled the signature change on getGroupsForUser . I stubbed anchorNative to do nothing and left a comment explaining that we need the full port of this patch later.
          Hide
          Chris Nauroth added a comment -

          Do we not allow C99 style declaration in the middle of a function in our JNI code? I've always liked that better than the original C style of declaring all at the top.

          One more thing about this: I think Visual Studio still does not support C99. In the Windows native code, we're declaring all variables at the top of the function, and it's a compilation error to put declarations in the middle. With conditional compilation, we could potentially do C99 in the Linux path and C89 in the Windows path, but this might cause confusion.

          This isn't an issue for this patch, but I thought I'd mention it.

          Show
          Chris Nauroth added a comment - Do we not allow C99 style declaration in the middle of a function in our JNI code? I've always liked that better than the original C style of declaring all at the top. One more thing about this: I think Visual Studio still does not support C99. In the Windows native code, we're declaring all variables at the top of the function, and it's a compilation error to put declarations in the middle. With conditional compilation, we could potentially do C99 in the Linux path and C89 in the Windows path, but this might cause confusion. This isn't an issue for this patch, but I thought I'd mention it.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12588669/HADOOP-9439.008.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

          org.apache.hadoop.ipc.TestSaslRPC

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2677//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2677//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12588669/HADOOP-9439.008.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-common-project/hadoop-common: org.apache.hadoop.ipc.TestSaslRPC +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2677//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2677//console This message is automatically generated.
          Hide
          Chris Nauroth added a comment -

          The TestSaslRPC failure appears to be unrelated. Aaron T. Myers, do you think it's related to this? http://svn.apache.org/viewvc?view=revision&revision=1494702

          Show
          Chris Nauroth added a comment - The TestSaslRPC failure appears to be unrelated. Aaron T. Myers , do you think it's related to this? http://svn.apache.org/viewvc?view=revision&revision=1494702
          Hide
          Colin Patrick McCabe added a comment -

          Thanks, Chris.

          Show
          Colin Patrick McCabe added a comment - Thanks, Chris.
          Hide
          Aaron T. Myers added a comment -

          Yea, it's unrelated. I'm fixing it.

          Thanks.

          Show
          Aaron T. Myers added a comment - Yea, it's unrelated. I'm fixing it. Thanks.
          Hide
          Todd Lipcon added a comment -

          +1, latest rev of the patch lgtm.

          Show
          Todd Lipcon added a comment - +1, latest rev of the patch lgtm.
          Hide
          Colin Patrick McCabe added a comment -

          committed to trunk, branch-2, branch-2.1

          Show
          Colin Patrick McCabe added a comment - committed to trunk, branch-2, branch-2.1
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk-Commit #4004 (See https://builds.apache.org/job/Hadoop-trunk-Commit/4004/)
          HADOOP-9439. JniBasedUnixGroupsMapping: fix some crash bugs (Colin Patrick McCabe) (Revision 1496112)

          Result = SUCCESS
          cmccabe : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1496112
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/CMakeLists.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/JniBasedUnixGroupsMapping.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.h
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/net/unix/DomainSocket.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMappingWin.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/getGroup.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.h
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.h
          Show
          Hudson added a comment - Integrated in Hadoop-trunk-Commit #4004 (See https://builds.apache.org/job/Hadoop-trunk-Commit/4004/ ) HADOOP-9439 . JniBasedUnixGroupsMapping: fix some crash bugs (Colin Patrick McCabe) (Revision 1496112) Result = SUCCESS cmccabe : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1496112 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/CMakeLists.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/JniBasedUnixGroupsMapping.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.h /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/net/unix/DomainSocket.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMappingWin.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/getGroup.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.h /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.h
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Yarn-trunk #251 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/251/)
          HADOOP-9439. JniBasedUnixGroupsMapping: fix some crash bugs (Colin Patrick McCabe) (Revision 1496112)

          Result = FAILURE
          cmccabe : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1496112
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/CMakeLists.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/JniBasedUnixGroupsMapping.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.h
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/net/unix/DomainSocket.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMappingWin.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/getGroup.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.h
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.h
          Show
          Hudson added a comment - Integrated in Hadoop-Yarn-trunk #251 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/251/ ) HADOOP-9439 . JniBasedUnixGroupsMapping: fix some crash bugs (Colin Patrick McCabe) (Revision 1496112) Result = FAILURE cmccabe : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1496112 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/CMakeLists.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/JniBasedUnixGroupsMapping.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.h /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/net/unix/DomainSocket.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMappingWin.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/getGroup.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.h /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.h
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1441 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1441/)
          HADOOP-9439. JniBasedUnixGroupsMapping: fix some crash bugs (Colin Patrick McCabe) (Revision 1496112)

          Result = FAILURE
          cmccabe : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1496112
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/CMakeLists.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/JniBasedUnixGroupsMapping.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.h
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/net/unix/DomainSocket.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMappingWin.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/getGroup.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.h
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.h
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1441 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1441/ ) HADOOP-9439 . JniBasedUnixGroupsMapping: fix some crash bugs (Colin Patrick McCabe) (Revision 1496112) Result = FAILURE cmccabe : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1496112 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/CMakeLists.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/JniBasedUnixGroupsMapping.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.h /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/net/unix/DomainSocket.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMappingWin.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/getGroup.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.h /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.h
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1468 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1468/)
          HADOOP-9439. JniBasedUnixGroupsMapping: fix some crash bugs (Colin Patrick McCabe) (Revision 1496112)

          Result = FAILURE
          cmccabe : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1496112
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/CMakeLists.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/JniBasedUnixGroupsMapping.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.h
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/net/unix/DomainSocket.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMappingWin.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/getGroup.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.h
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.c
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.h
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1468 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1468/ ) HADOOP-9439 . JniBasedUnixGroupsMapping: fix some crash bugs (Colin Patrick McCabe) (Revision 1496112) Result = FAILURE cmccabe : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1496112 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/CMakeLists.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/nativeio/NativeIO.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/JniBasedUnixGroupsMapping.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/exception.h /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/io/nativeio/NativeIO.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/net/unix/DomainSocket.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMapping.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/JniBasedUnixGroupsMappingWin.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/getGroup.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_group_info.h /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.c /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/native/src/org/apache/hadoop/security/hadoop_user_info.h

            People

            • Assignee:
              Colin Patrick McCabe
              Reporter:
              Colin Patrick McCabe
            • Votes:
              0 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development