[ https://issues.apache.org/jira/browse/HADOOP-9439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13677450#comment-13677450 ]
Todd Lipcon commented on HADOOP-9439: ------------------------------------- It looks like you've inverted the meaning of the boolean in some places here: {code} + * @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); {code} suggests that the second parameter being 'false' disables locking. But then: {code} public List<String> getGroups(String user) throws IOException { String[] groups = new String[0]; try { - groups = getGroupForUser(user); + groups = getGroupsForUser(user, removeConcurrency); {code} 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) {code} + static private void logError(int groupIdx, String error) { + LOG.error("error looking up the name of group " + groupIdx + ": " + error); {code} This parameter should be 'groupId' not 'groupIdx' {code} + 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); {code} 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). {code} + error_msg = (*env)->NewStringUTF(env, terror(ret)); + if (!error_msg) { + (*env)->ExceptionClear(env); + return; + } {code} 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. > JniBasedUnixGroupsMapping: fix some crash bugs > ---------------------------------------------- > > Key: HADOOP-9439 > URL: https://issues.apache.org/jira/browse/HADOOP-9439 > Project: Hadoop Common > Issue Type: Bug > Components: native > Affects Versions: 2.0.4-alpha > Reporter: Colin Patrick McCabe > Assignee: Colin Patrick McCabe > Priority: Minor > Attachments: HADOOP-9439.001.patch, HADOOP-9439.003.patch, > HDFS-4640.002.patch > > > 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. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators For more information on JIRA, see: http://www.atlassian.com/software/jira