8139390: Very long classname in jimage causes SIGSEGV
authorrriggs
Fri, 30 Oct 2015 11:12:20 -0400
changeset 33492 051df634418c
parent 33408 7884ecc0cc6f
child 33493 442660a9b96f
child 33497 3fadd7701c2f
8139390: Very long classname in jimage causes SIGSEGV Summary: Correct issues with ImageNativeSubstrate and JImageReadTest Reviewed-by: mchung
jdk/src/java.base/share/native/libjimage/ImageNativeSubstrate.cpp
jdk/src/java.base/share/native/libjimage/jimage.cpp
jdk/test/jdk/internal/jimage/JImageReadTest.java
--- a/jdk/src/java.base/share/native/libjimage/ImageNativeSubstrate.cpp	Thu Oct 29 12:39:08 2015 -0700
+++ b/jdk/src/java.base/share/native/libjimage/ImageNativeSubstrate.cpp	Fri Oct 30 11:12:20 2015 -0400
@@ -26,8 +26,7 @@
 #include <string.h>
 
 #include "jni.h"
-#include "jni_util.h"
-#include "jdk_util.h"
+
 #include "endian.hpp"
 #include "imageDecompressor.hpp"
 #include "imageFile.hpp"
@@ -39,6 +38,17 @@
 
 extern bool MemoryMapImage;
 
+/////////////////////////////////////////////////////////////////////////////
+
+// Static function for primitive throw since libjimage is not linked with libjava
+static void JNICALL ThrowByName(JNIEnv *env, const char *name, const char *msg)
+{
+    jclass cls = (env)->FindClass(name);
+
+    if (cls != 0) /* Otherwise an exception has already been thrown */
+        (env)->ThrowNew(cls, msg);
+}
+
 // jdk.internal.jimage /////////////////////////////////////////////////////////
 
 // Java entry to open an image file for sharing.
@@ -446,6 +456,23 @@
     jlong size = 0;
     jlong ret = 0;
 
+    if (moduleName == NULL) {
+        ThrowByName(env, "java/lang/NullPointerException", "moduleName");
+        return 0;
+    }
+    if (version == NULL) {
+        ThrowByName(env, "java/lang/NullPointerException", "version");
+        return 0;
+    }
+    if (path == NULL) {
+        ThrowByName(env, "java/lang/NullPointerException", "path");
+        return 0;
+    }
+    if (output_size == NULL) {
+        ThrowByName(env, "java/lang/NullPointerException", "size");
+        return 0;
+    }
+
     do {
         native_module = env->GetStringUTFChars(moduleName, NULL);
         if (native_module == NULL)
@@ -529,25 +556,47 @@
         // Store if there is room in the array
         // Concatenate to get full path
         char fullpath[IMAGE_MAX_PATH];
-        fullpath[0] = '\0';
-        if (*module != '\0') {
-            strncpy(fullpath, "/", IMAGE_MAX_PATH - 1);
-            strncat(fullpath, module, IMAGE_MAX_PATH - 1);
-            strncat(fullpath, "/", IMAGE_MAX_PATH - 1);
+        size_t moduleLen = strlen(module);
+        size_t packageLen = strlen(package);
+        size_t nameLen = strlen(name);
+        size_t extLen = strlen(extension);
+        size_t index;
+
+        if (1 + moduleLen + 1 + packageLen + 1 + nameLen + 1 + extLen + 1 > IMAGE_MAX_PATH) {
+            ThrowByName(env, "java/lang/InternalError", "concatenated name too long");
+            return true;
         }
-        if (*package != '\0') {
-            strncat(fullpath, package, IMAGE_MAX_PATH - 1);
-            strncat(fullpath, "/", IMAGE_MAX_PATH - 1);
+
+        index = 0;
+        if (moduleLen > 0) {
+            fullpath[index++] = '/';
+            memcpy(&fullpath[index], module, moduleLen);
+            index += moduleLen;
+            fullpath[index++] = '/';
         }
-        strncat(fullpath, name, IMAGE_MAX_PATH - 1);
-        if (*extension != '\0') {
-            strncat(fullpath, ".", IMAGE_MAX_PATH - 1);
-            strncat(fullpath, extension, IMAGE_MAX_PATH - 1);
+        if (packageLen > 0) {
+            memcpy(&fullpath[index], package, packageLen);
+            index += packageLen;
+            fullpath[index++] = '/';
+        }
+        memcpy(&fullpath[index], name, nameLen);
+        index += nameLen;
+        if (extLen > 0) {
+            fullpath[index++] = '.';
+            memcpy(&fullpath[index], extension, extLen);
+            index += extLen;
         }
+        fullpath[index++] = '\0';
+
         jobject str = env->NewStringUTF(fullpath);
-        JNU_CHECK_EXCEPTION_RETURN(env, true);
+        if (env->ExceptionCheck()) {
+            return true;
+        }
+
         env->SetObjectArrayElement(vdata->array, vdata->size, str);
-        JNU_CHECK_EXCEPTION_RETURN(env, true);
+        if (env->ExceptionCheck()) {
+            return true;
+        }
     }
     vdata->size++; // always count so the total size is returned
     return true;
@@ -584,7 +633,10 @@
     jstring module = NULL;
 
     native_package = env->GetStringUTFChars(package_name, NULL);
-    JNU_CHECK_EXCEPTION_RETURN(env, NULL);
+    if (env->ExceptionCheck()) {
+        return NULL;
+    }
+
 
     native_module = JIMAGE_PackageToModule((JImageFile*) jimageHandle, native_package);
     if (native_module != NULL) {
--- a/jdk/src/java.base/share/native/libjimage/jimage.cpp	Thu Oct 29 12:39:08 2015 -0700
+++ b/jdk/src/java.base/share/native/libjimage/jimage.cpp	Fri Oct 30 11:12:20 2015 -0400
@@ -102,14 +102,29 @@
 extern "C" JImageLocationRef JIMAGE_FindResource(JImageFile* image,
         const char* module_name, const char* version, const char* name,
         jlong* size) {
-    ImageLocation location;
+    // Concatenate to get full path
     char fullpath[IMAGE_MAX_PATH];
+    size_t moduleNameLen = strlen(module_name);
+    size_t nameLen = strlen(name);
+    size_t index;
+
+    // TBD:   assert(moduleNameLen > 0 && "module name must be non-empty");
+    assert(nameLen > 0 && "name must non-empty");
 
-    // Concatenate to get full path
-    strncpy(fullpath, "/", IMAGE_MAX_PATH - 1);
-    strncat(fullpath, module_name, IMAGE_MAX_PATH - 1);
-    strncat(fullpath, "/", IMAGE_MAX_PATH - 1);
-    strncat(fullpath, name, IMAGE_MAX_PATH - 1);
+    // If the concatenated string is too long for the buffer, return not found
+    if (1 + moduleNameLen + 1 + nameLen + 1 > IMAGE_MAX_PATH) {
+        return 0L;
+    }
+
+    index = 0;
+    fullpath[index++] = '/';
+    memcpy(&fullpath[index], module_name, moduleNameLen);
+    index += moduleNameLen;
+    fullpath[index++] = '/';
+    memcpy(&fullpath[index], name, nameLen);
+    index += nameLen;
+    fullpath[index++] = '\0';
+
     JImageLocationRef loc =
             (JImageLocationRef) ((ImageFileReader*) image)->find_location_index(fullpath, (u8*) size);
     return loc;
--- a/jdk/test/jdk/internal/jimage/JImageReadTest.java	Thu Oct 29 12:39:08 2015 -0700
+++ b/jdk/test/jdk/internal/jimage/JImageReadTest.java	Fri Oct 30 11:12:20 2015 -0400
@@ -22,7 +22,9 @@
  */
 
 /*
+ * @test
  * @modules java.base/jdk.internal.jimage
+ * @run testng JImageReadTest
  * @summary Unit test for libjimage JIMAGE_Open/Read/Close
  */
 
@@ -57,8 +59,7 @@
                 {"java.base", "java/lang/String.class"},
                 {"java.base", "java/lang/Object.class"},
                 {"java.base", "sun/reflect/generics/tree/TypeArgument.class"},
-                {"jdk.jdeps", "com/sun/tools/javap/StackMapWriter$StackMapBuilder.class"},
-                {"jdk.hotspot.agent", "sa.properties"},
+                {"java.base", "sun/net/www/content-types.properties"},
                 {"java.logging", "java/util/logging/Logger.class"},
                 {"java.base", "java/NOSUCHCLASS/yyy.class"},    // non-existent
                 {"NOSUCHMODULE", "java/lang/Class.class"},    // non-existent
@@ -165,8 +166,10 @@
         int count = ImageNativeSubstrate.JIMAGE_Resources(jimageHandle,
                 names);
         System.out.printf(" count: %d, a class: %s\n", count, names[0]);
-        Assert.assertTrue(max > 31000,
-                "missing entries, should be more than 31000, reported: " + count);
+        int minEntryCount = 16000;
+        Assert.assertTrue(max > minEntryCount,
+                "missing entries, should be more than " + minEntryCount +
+                ", reported: " + count);
         Assert.assertTrue(count == max,
                 "unexpected count of entries, count: " + count
                         + ", max: " + max);
@@ -310,6 +313,7 @@
     static boolean isMetaName(String name) {
         return name.startsWith("/modules")
                 || name.startsWith("/packages")
+                || name.startsWith("META-INF/services")
                 || name.equals("bootmodules.jdata");
     }
 
@@ -362,6 +366,24 @@
         System.out.printf(" %s: %d names%n", fname, names.length);
     }
 
+    @Test
+    static void test5_nameTooLong() throws IOException {
+        long[] size = new long[1];
+        String moduleName = "FictiousModuleName";
+        String className = String.format("A%09999d", 1);
+
+        long jimageHandle = ImageNativeSubstrate.JIMAGE_Open(imageFile);
+        Assert.assertTrue(jimageHandle != 0, "JIMAGE_Open failed: id: " + jimageHandle);
+
+        long locationHandle =
+                ImageNativeSubstrate.JIMAGE_FindResource(jimageHandle,
+                        moduleName, "9.0", className, size);
+
+        Assert.assertEquals(0, locationHandle, "Too long name should have failed");
+
+        ImageNativeSubstrate.JIMAGE_Close(jimageHandle);
+    }
+
     // main method to run standalone from jtreg
 
     @Test(enabled=false)