Skip to content

WIP [update java bindings v0.14.0]#3663

Closed
Kuntal271 wants to merge 1 commit intoWasmEdge:masterfrom
Kuntal271:work/java
Closed

WIP [update java bindings v0.14.0]#3663
Kuntal271 wants to merge 1 commit intoWasmEdge:masterfrom
Kuntal271:work/java

Conversation

@Kuntal271
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown
Member

juntao commented Aug 16, 2024

Hello, I am a code review agent on flows.network. Here are my reviews of changed source code files in this PR.


bindings/java/wasmedge-java/src/main/java/org/wasmedge/NativeUtils.java

Potential issues

  1. The loadLibraryFromJar method does not handle the case where the resource path (path) might not exist in the JAR file, which can lead to a NullPointerException when trying to open the InputStream.

  2. There is no error handling for the case where the temporary directory creation fails due to insufficient permissions or disk space. This could cause a RuntimeException to be thrown, potentially causing the application to crash.

  3. The isPosixCompliant method might return false negatives on certain systems that support POSIX file attributes but are not identified as such by Java's FileSystems class. This can lead to unnecessary retention of temporary files on non-POSIX compliant systems.

Summary of changes

  1. Added a class-level documentation that describes the utility of the NativeUtils class, which is to load native libraries from JAR files.
  2. Updated the loadLibraryFromJar method's documentation for clarity and added a paragraph explaining its functionality in more detail.
  3. Enhanced error handling in the createTempDirectory method by wrapping the mkdir() call inside an if-statement and throwing a RuntimeException with a descriptive message if directory creation fails.

bindings/java/wasmedge-java/wasmedge-jni/lib/FunctionTypeContext.c

Potential issues

  1. The ConvertToJavaValueType function does not handle the case where it cannot find the Java class for value type (valueTypeClass == NULL). It should return an appropriate error message or throw an exception in this scenario.

  2. In the ConvertToJavaFunctionList function, there is no check to ensure that the jFunc object returned by ConvertToJavaFunctionType is not null before attempting to add it to the Java list (jFuncList). This could lead to a null pointer dereference error if ConvertToJavaFunctionType returns null.

  3. The ConvertToJavaFunctionType function does not handle the case where it cannot find the Java constructor for the FunctionTypeContext class (constructor == NULL). It should return an appropriate error message or throw an exception in this scenario.

Summary of changes

  • The data type of WasmEdge_ValType has been changed from an enumeration to a structure.
  • The function ConvertToJavaValueType now checks if the input WasmEdge_ValType* is NULL and returns NULL if it is.
  • The function ConvertToJavaFunctionType now correctly frees the memory allocated for the return type list and parameter type list to prevent memory leaks.

bindings/java/wasmedge-java/wasmedge-jni/lib/GlobalTypeContext.c

Potential issues

  1. The switch statement in the Java_org_wasmedge_GlobalTypeContext_nativeInit function is missing a default case to handle unexpected values of 'type'. This could lead to undefined behavior if an unsupported value is passed as 'valueType' from Java code.

  2. In the Java_org_wasmedge_GlobalTypeContext_nativeGetValueType function, there is no error handling for the case where the class "org/wasmedge/WasmEdgeException" cannot be found. This could lead to a crash if an exception needs to be thrown but the class is not available.

  3. The createJGlobalTypeContext function is not used in the provided code snippet, which might indicate that it's either unused or a remnant of previous implementation. Unused functions can clutter the codebase and potentially lead to confusion.

Summary of changes

  1. The nativeInit function now converts the input valueType from a Java integer to a WasmEdge_ValType before creating the global type context. This allows for more flexible and explicit handling of different value types in WebAssembly.
  2. The nativeGetValueType function has been updated to return the value type as a Java integer instead of a WasmEdge_ValType. It also includes error handling for unexpected or unknown value types.
  3. The codebase now includes additional error checking and exception throwing mechanisms, which will help improve debugging and error reporting in case of unexpected scenarios.

bindings/java/wasmedge-java/wasmedge-jni/lib/TableInstanceContext.c

Potential issues

  1. In the Java_org_wasmedge_TableInstanceContext_getData function, there is no error checking for the result of WasmEdge_TableInstanceGetData. If this function fails, it may lead to undefined behavior when trying to convert val to a Java value.

  2. In the createJTableInstanceContext function, if tabInstance is NULL, the function returns NULL without any error handling in the calling code. This might cause a null pointer dereference in the Java code that calls this native method.

  3. The Java_org_wasmedge_TableInstanceContext_close function does not check the result of WasmEdge_TableInstanceDelete. If this function fails, it may lead to resource leaks or other undefined behavior.

Summary of changes

  1. The switch-case block that initializes the WasmEdge_Value variable based on the value type has been replaced with an if-else block for better readability and performance.
  2. The initialization of WasmEdge_Value now uses explicit numeric values (0x7F, 0x7E, etc.) instead of enum constants (WasmEdge_ValType_I32, WasmEdge_ValType_I64, etc.), which may improve code clarity.
  3. The function Java_org_wasmedge_TableInstanceContext_getSize is not included in the changes provided, so no specific change can be mentioned for this function.

bindings/java/wasmedge-java/wasmedge-jni/lib/TableTypeContext.c

Potential issues

  1. The code does not check if the jmethodID returned by GetMethodID is null, which could lead to a segmentation fault if the method IDs are not found.
  2. The setPointer and getTableTypeContext functions are not defined in the provided source code, which could cause compilation errors.
  3. The findJavaClass and findJavaMethod functions are also not defined in the provided source code, which could lead to runtime errors if these functions are expected to be available but are not actually implemented.

Summary of changes

  1. The function Java_org_wasmedge_TableTypeContext_nativeInit has been significantly modified to convert a Java integer type (jint) into a WasmEdge value type (WasmEdge_ValType). This is done through a switch-case statement that maps the jint values to their corresponding WasmEdge_ValType.

  2. The function Java_org_wasmedge_TableTypeContext_nativeGetRefType has been updated to convert the returned WasmEdge value type (WasmEdge_ValType) back into a Java integer type (jint). This is done through a series of if-else statements that check the type of the WasmEdge_ValType and return the corresponding jint value.

  3. Error handling has been added to Java_org_wasmedge_TableTypeContext_nativeGetRefType function in case an unexpected WasmEdge_ValType is encountered. If this occurs, a new "WasmEdgeException" will be thrown with an appropriate error message.

bindings/java/wasmedge-java/wasmedge-jni/lib/ValueType.c

Potential issues

  1. The atoint128_t function does not handle negative numbers correctly because it multiplies the result by -1 after the loop, which means all numbers will be converted to negative values if the input string starts with a '-' sign.

  2. The u128toa function has a buffer overflow issue because it writes more than 40 characters into the static buffer buf. This can lead to undefined behavior and potential security vulnerabilities.

  3. In the JavaValueToWasmEdgeValue function, there is no error handling for unsupported or invalid types. If an unrecognized type is encountered, it will result in undefined behavior. It would be beneficial to add a default case that throws an exception or returns an error value when the input type is not supported.

Summary of changes

  1. The function JavaValueToWasmEdgeValue has been simplified by directly using the integer value of the WasmEdge_ValType enum instead of converting it from a jobject. This removes the need for finding and calling methods on the Java Value class to get the type.

  2. In the function JavaValueToWasmEdgeValue, the switch-case statement has been replaced with an if-else statement, which makes the code more concise and easier to read.

  3. The function WasmEdgeValueToJavaValue has also been simplified by using WasmEdge_ValTypeIs* functions instead of a switch-case statement to determine the type of the WasmEdge_Value. This change improves readability and maintainability of the code.

bindings/java/wasmedge-java/wasmedge-jni/lib/common.c

Potential issues

  1. The function throwNoSuchMethodError does not check if the class exClass is NULL before using it to throw a new exception, which could lead to a segmentation fault or undefined behavior.
  2. In the function getStringVal, memory is allocated for buf but never freed, leading to a memory leak.
  3. The function JStringArrayToPtr allocates memory for ptr but does not free it in the corresponding ReleaseCString function, causing a memory leak.

Summary of changes

  1. The parseValueTypes function now checks for memory allocation failure and handles it appropriately.
  2. The parseValueTypes function now uses type-specific functions like WasmEdge_ValTypeGenI32(), WasmEdge_ValTypeGenF64(), etc., to set the value types, which improves readability and maintainability.
  3. The setJavaValueObject function now uses type-specific check functions like WasmEdge_ValTypeIsI32(), WasmEdge_ValTypeIsF64(), etc., to handle different data types, which enhances the robustness of the code.

bindings/java/wasmedge-java/wasmedge-jni/lib/common.h

Potential issues

  1. Issue: The function getFloatVal is declared to return a long, but it actually returns a float value. This could lead to incorrect results and potential crashes if the returned value is used in a context that expects a long.

  2. Issue: In the macro definition for GETTER(NAME), there's no error checking or handling when getPointer returns NULL, which might cause undefined behavior or crashes if it's dereferenced without proper validation.

  3. Issue: The function ReleaseCString is declared to take a jarray as its second argument, but it seems like it's intended to free memory allocated for an array of C strings (char**). This could lead to memory leaks or crashes if the wrong type of object is passed to this function.

Summary of changes

  1. Changed the return type of the parseValueTypes function from an unspecified enum to a WasmEdge_ValType*.
  2. Removed the void keyword from the declaration of the setJavaDoubleValue and setJavaStringValue functions, suggesting that they may now have a return value (though this is not explicitly stated in the provided patch).
  3. The third change is not explicitly mentioned in the given patch snippet but it's important to note that the function declarations are being moved from a header file into a source file, which might indicate a refactoring or modularization effort.

@github-actions github-actions Bot added the binding-java Java Bindings label Aug 16, 2024
@dannypsnl
Copy link
Copy Markdown
Member

@Kuntal271 please fix the DCO check, you need to leave your email at Sign-Off line in commit message, then fix the conflicts.

Comment on lines +18 to +41
switch(type){
case 0x7F:
finalType = WasmEdge_ValTypeGenI32();
break;
case 0x7E:
finalType = WasmEdge_ValTypeGenI64();
break;
case 0x7D:
finalType = WasmEdge_ValTypeGenF32();
break;
case 0x7C:
finalType = WasmEdge_ValTypeGenF64();
break;
case 0x7B:
finalType = WasmEdge_ValTypeGenV128();
break;
case 0x70:
finalType = WasmEdge_ValTypeGenFuncRef();
break;
case 0x6F:
finalType = WasmEdge_ValTypeGenExternRef();
break;
}

Copy link
Copy Markdown
Member

@dannypsnl dannypsnl Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@q82419 I'm wondering does there exist a more automatic solution? Since component model will put tons of repeated work here at future (very predictable).

Comment on lines 63 to +65
JNIEXPORT jint JNICALL Java_org_wasmedge_GlobalTypeContext_nativeGetValueType(
JNIEnv *env, jobject thisObject) {
return WasmEdge_GlobalTypeGetValType(getGlobalTypeContext(env, thisObject));
WasmEdge_GlobalTypeContext *globalTypeContext = getGlobalTypeContext(env, thisObject);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say if this is no longer been used by any internal works here, then remove this, since user should invoke IsXXX to check to avoid leak more details of implementation.

@hydai hydai marked this pull request as draft August 16, 2024 13:19
@dannypsnl
Copy link
Copy Markdown
Member

Please rebase to the latest changes.

@hydai
Copy link
Copy Markdown
Member

hydai commented Jan 19, 2025

Hi @Kuntal271
Are you going to work on this PR, or is it no longer being maintained?

@Kuntal271
Copy link
Copy Markdown
Author

I’m a bit preoccupied these days and might not be able to work on the PR immediately. I’ll make progress on it whenever I get some time. If there’s a deadline or if it’s urgent, feel free to take it over and proceed with it.

@hydai
Copy link
Copy Markdown
Member

hydai commented Jun 22, 2025

Closing due to #4191

@hydai hydai closed this Jun 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

binding-java Java Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants