microsoft / microsoft/onnxruntime
[Java] copyStringTensorToArray() leaks tempBuffer on three early-return paths, and does not delete the jstring it creates per element
- Dominant language
- C++
- Stars
- 21.9k
- Forks
- 4.2k
- Avg merge
- 4d 11h
- Merged PRs (30d)
- 184
Description
### Describe the issue
The string tensor conversion helper allocates `tempBuffer` up front and has a `string_tensor_cleanup:` label that frees its native buffers, but three early-return paths bypass that label. The conversion loop also creates one Java string per tensor element without checking the result or deleting the reference.
File: `java/src/main/native/OrtJniUtil.c`
Function: `copyStringTensorToArray`
### 1. `tempBuffer` leaks on three early returns
```c
size_t bufferSize = 16;
char * tempBuffer = malloc(bufferSize);
if (tempBuffer == NULL) { ... }
// Get the buffer size needed
size_t totalStringLength = 0;
OrtErrorCode code = checkOrtStatus(jniEnv, api, api->GetStringTensorDataLength(tensor, &totalStringLength));
if (code != ORT_OK) {
return code; /* (1) tempBuffer leaked */
}
char * characterBuffer = malloc(sizeof(char)*(totalStringLength+length));
if (characterBuffer == NULL) {
throwOrtException(jniEnv, 1, "Not enough memory");
return ORT_FAIL; /* (2) tempBuffer leaked */
}
size_t * offsets = allocarray(sizeof(size_t), length+1);
if (offsets == NULL) {
free((void*)characterBuffer);
throwOrtException(jniEnv, 1, "Not enough memory");
return ORT_FAIL; /* (3) tempBuffer leaked */
}
```
Path (3) is the clearest about the intent: it remembers to free `characterBuffer` but not `tempBuffer`. Path (1) matters most in practice, because it is reachable whenever `GetStringTensorDataLength()` returns a non-OK status — an ordinary runtime error rather than an out-of-memory condition — so this leak does not require memory pressure to trigger.
The function already has the right cleanup path; these three returns simply do not use it. The realloc-failure path a few lines below does the right thing and jumps to `string_tensor_cleanup`.
### 2. Temporary `jstring` references are not deleted, and `NewStringUTF()` is unchecked
```c
for (size_t i = 0; i < length; i++) {
...
jobject tempString = (*jniEnv)->NewStringUTF(jniEnv,tempBuffer);
(*jniEnv)->SetObjectArrayElement(jniEnv,outputArray,safecast_size_t_to_jsize(i),tempString);
}
```
`SetObjectArrayElement()` stores the string in `outputArray` but does not consume the local reference. These references are reclaimed when the native method returns, so this is not a leak that persists across calls — the problem is that `length` is the tensor element count, which is data-dependent and unbounded, so converting one large string tensor grows the local reference table far beyond what the JVM pre-allocates.
The return value of `NewStringUTF()` is also unchecked; on failure the code continues into `SetObjectArrayElement()` with a pending exception and keeps iterating.
This is on the inference path rather than a setup path: `copyStringTensorToArray()` is reached from `OnnxTensor.getValue()` (via `createStringArrayFromTensor`) and from `OnnxMap` key/value extraction.
**Expected behavior:** every allocation in the function is released on every exit path, as the `string_tensor_cleanup:` label was written to do; each per-element `jstring` is checked and released after being stored into the output array.
**Actual behavior:** `tempBuffer` is leaked on three of the function's exit paths, one of which is a plain API-failure path; and `length` local references stay live for the duration of the call.
**Suggested fix** — route the three early returns through the existing cleanup label so every allocation has one matching free:
```c
OrtErrorCode code = checkOrtStatus(jniEnv, api, api->GetStringTensorDataLength(tensor, &totalStringLength));
if (code != ORT_OK) {
goto string_tensor_cleanup;
}
```
The label already null-checks `tempBuffer`; `characterBuffer` and `offsets` would need to be initialised to `NULL` and null-checked there too. For the loop, check the result, set the element, then delete the reference:
```c
jobject tempString = (*jniEnv)->NewStringUTF(jniEnv, tempBuffer);
if (tempString == NULL) {
code = ORT_FAIL; /* exception already pending */
goto string_tensor_cleanup;
}
(*jniEnv)->SetObjectArrayElement(
jniEnv, outputArray, safecast_size_t_to_jsize(i), tempString);
(*jniEnv)->DeleteLocalRef(jniEnv, tempString);
```
### To reproduce
This is a source-level defect, confirmed by reading the code at the commit below. Inference results are unaffected, which is why functional tests do not catch it.
Static confirmation:
1. Open `java/src/main/native/OrtJniUtil.c` and find `copyStringTensorToArray`.
2. Note `tempBuffer` is allocated at the top of the function and that the function ends with a `string_tensor_cleanup:` label which frees it.
3. Confirm the three `return` statements after that allocation — the `GetStringTensorDataLength()` failure, the `characterBuffer` allocation failure, and the `offsets` allocation failure — return directly instead of jumping to the label.
4. In the conversion loop, confirm there is no `DeleteLocalRef()` for `tempString` and no null check on `NewStringUTF()`.
Runtime observation (optional):
1. Run any model whose output is a string tensor, or a ZipMap-style model producing a map output, under `valgrind --leak-check=full`, with a build where `GetStringTensorDataLength()` fails; observe the 16-byte `tempBuffer` block reported as definitely lost.
2. For the reference growth, run `java -Xcheck:jni` and call `OnnxTensor.getValue()` on a string tensor with a large element count; observe the local reference table warning.
### Urgency
_No response_
### Platform
Linux
### OS Version
Ubuntu 22.04.5 LTS
### ONNX Runtime Installation
Built from Source
### ONNX Runtime Version or Commit ID
cf5e9eba49b07b4b4ab71ed7903cded9f1446690
### ONNX Runtime API
Python
### Architecture
X64
### Execution Provider
Default CPU
### Execution Provider Library Version
_No response_
Contributor guide
Research direction
Start in java/src/main/native/OrtJniUtil.c at copyStringTensorToArray, then trace callers from OnnxTensor.getValue() through createStringArrayFromTensor and the OnnxMap extraction paths. Verify each exit reaches string_tensor_cleanup and each per-element NewStringUTF reference is handled, then exercise the Java string-tensor or map path with -Xcheck:jni or Valgrind; done means no skipped cleanup or unchecked temporary reference remains.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c, java
- Domain
- machine-learning
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100