[jira] [Commented] (HADOOP-9439) JniBasedUnixGroupsMapping: fix some crash bugs

["Todd Lipcon (JIRA)" <ji...@apache.org>]

    [ 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