microsoft / microsoft/onnxruntime

[Java] copyStringTensorToArray() leaks tempBuffer on three early-return paths, and does not delete the jstring it creates per element

Open
#31,705 1 comment 0 reactions 0 assignees View on GitHub
api:Java stale
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.