GH-1205: Fix byte-array element leak in FromSchemaByteArray

[PG1204]

What's Changed

FromSchemaByteArray() in dataset/src/main/cpp/jni_util.cc acquires the Java byte-array elements via GetByteArrayElements(), but the matching ReleaseByteArrayElements() was only reached on the success path. When arrow::ipc::ReadSchema() failed, ARROW_ASSIGN_OR_RAISE returned early and skipped the release, leaking the pinned/copied array buffer on every call with malformed or incompatible serialized schema bytes (e.g. via createDataset()).

This releases the elements through an RAII guard so they are always freed on every exit path - success or error, and stays correct if additional early returns are added later.

Closes #1205.

[xborder]

Seems reasonable. From what I was researching, doesn't seem like it is a leak easy to hit and it may only leak a few KBs.

I'd probably just try to add a test that reproduces the issue an ensure that it passes with the fix

[PG1204]

Seems reasonable. From what I was researching, doesn't seem like it is a leak easy to hit and it may only leak a few KBs.

I'd probably just try to add a test that reproduces the issue an ensure that it passes with the fix

@xborder Agreed it's low-impact and hard to hit. I've added a regression test (TestFromSchemaByteArray) that feeds malformed schema bytes to the native createDataset path, which is the exact branch that used to skip ReleaseByteArrayElements, and confirms it now fails gracefully with the fix in place.

[xborder]

@PG1204 I don't think this is testing that we fixed the memory leak. This is just testing the error is thrown. Shouldn't it check no memory was leaked?

[PG1204]

@PG1204 I don't think this is testing that we fixed the memory leak. This is just testing the error is thrown. Shouldn't it check no memory was leaked?

@xborder you're right, that only proves the error path is hit, not that the leak is gone. The catch is what leaks: the buffer wraps the pointer from GetByteArrayElements, which on HotSpot copies the array into a JVM-internal malloc'd buffer, and that copy is what leaked. It's not tracked by NativeMemoryPool, not on the Java heap, and has no portable Java API; it's only visible via process RSS.

So the only way I see to assert "no leak" from Java is an RSS-growth check: loop the call many times with a large payload and assert RSS stays bounded (unfixed balloons by GBs, fixed stays flat). That works on Linux but needs gating on macOS/Windows and is a bit flaky.

Would you prefer (1) that RSS-based test, or (2) dropping the Java assertion and verifying manually with a local leak-checker (valgrind/ASAN), keeping the current test as a graceful-failure guard? Note CI doesn't run a sanitizer today, so option 2 wouldn't be enforced automatically. Happy either way, just want to avoid landing a flaky test.

[lidavidm]

I don't think that's reasonable to do. I doubt this is a code path that gets exercised often anyways?

[PG1204]

I don't think that's reasonable to do. I doubt this is a code path that gets exercised often anyways?

agreed, thanks. I'll skip the RSS-based test then, it'd be flaky and isn't worth it for this path. I'll keep the RAII fix plus the existing lightweight test as a graceful-failure regression guard (malformed schema bytes -> exception, no crash), and I'll reword its javadoc so it no longer claims to assert the leak itself. Let me know if you'd rather want to drop the test entirely.

[lidavidm]

@PG1204 can you rebase?

(Also, are you using AI assistance at all? I believe ASF may start requiring disclosure soon.)

[PG1204]

@PG1204 can you rebase?

(Also, are you using AI assistance at all? I believe ASF may start requiring disclosure soon.)

@lidavidm rebase done.
yeah i'm using AI assistance, would you like me to disclose it in the PR description?

[jbonofre]

Thanks for the fix. LGTM!

FromSchemaByteArray now releases the pinned byte array elements through a unique_ptr guard, so the release also happens when ReadSchema fails. The guard is declared before the Buffer and BufferReader, so it is destroyed after them and the buffers never outlive the pinned bytes. ReadSchema copies what it needs into the returned Schema, so nothing refers to released memory. The null return from GetByteArrayElements is handled correctly. createDataset is the only caller, and it wraps the call in JniGetOrThrow, so the failure surfaces as a Java exception.

Nit: TestFromSchemaByteArray only checks that the error path throws. It doesn't verify that the elements are released, which the javadoc already says. That seems fine for a JNI leak fix.