Copilot commented on code in PR #4396:
URL: https://github.com/apache/arrow-adbc/pull/4396#discussion_r3410629293
##########
java/driver/jni/src/main/cpp/jni_wrapper.cc:
##########
@@ -351,18 +357,28 @@
Java_org_apache_arrow_adbc_driver_jni_impl_NativeAdbc_openDatabase(
JniStringView key_str(env, key);
JniStringView value_str(env, value);
- CHECK_ADBC_ERROR(AdbcDatabaseSetOption(db.get(), key_str.value,
value_str.value,
- &error_guard.error),
- error_guard.error);
+ result = AdbcDatabaseSetOption(db.get(), key_str.value, value_str.value,
+ &error_guard.error);
+ if (result != ADBC_STATUS_OK) {
+ std::ignore = AdbcDatabaseRelease(db.get(), nullptr);
+ }
Review Comment:
`std::ignore` is defined in `<tuple>`, which isn't included in this file. To
avoid relying on transitive includes (and potential build breaks), prefer an
explicit `(void)` cast when intentionally ignoring a return value.
##########
java/driver/jni/src/main/cpp/jni_wrapper.cc:
##########
@@ -328,13 +328,19 @@
Java_org_apache_arrow_adbc_driver_jni_impl_NativeAdbc_openDatabase(
jobjectArray parameters) {
AdbcErrorGuard error_guard;
try {
+ jclass nativeHandleKlass = RequireImplClass(env, "NativeDatabaseHandle");
+ jmethodID nativeHandleCtor = RequireMethod(env, nativeHandleKlass,
"<init>", "(J)V");
+
auto db = std::make_unique<struct AdbcDatabase>();
std::memset(db.get(), 0, sizeof(struct AdbcDatabase));
CHECK_ADBC_ERROR(AdbcDatabaseNew(db.get(), &error_guard.error),
error_guard.error);
- CHECK_ADBC_ERROR(AdbcDriverManagerDatabaseSetLoadFlags(
- db.get(), ADBC_LOAD_FLAG_DEFAULT, &error_guard.error),
- error_guard.error);
+ auto result = AdbcDriverManagerDatabaseSetLoadFlags(db.get(),
ADBC_LOAD_FLAG_DEFAULT,
+ &error_guard.error);
+ if (result != ADBC_STATUS_OK) {
+ std::ignore = AdbcDatabaseRelease(db.get(), nullptr);
+ }
Review Comment:
`std::ignore` is defined in `<tuple>`, which isn't included in this file. To
avoid relying on transitive includes (and potential build breaks), prefer an
explicit `(void)` cast when intentionally ignoring a return value.
##########
java/driver/jni/src/main/cpp/jni_wrapper.cc:
##########
@@ -351,18 +357,28 @@
Java_org_apache_arrow_adbc_driver_jni_impl_NativeAdbc_openDatabase(
JniStringView key_str(env, key);
JniStringView value_str(env, value);
- CHECK_ADBC_ERROR(AdbcDatabaseSetOption(db.get(), key_str.value,
value_str.value,
- &error_guard.error),
- error_guard.error);
+ result = AdbcDatabaseSetOption(db.get(), key_str.value, value_str.value,
+ &error_guard.error);
+ if (result != ADBC_STATUS_OK) {
+ std::ignore = AdbcDatabaseRelease(db.get(), nullptr);
+ }
+ CHECK_ADBC_ERROR(result, error_guard.error);
}
- CHECK_ADBC_ERROR(AdbcDatabaseInit(db.get(), &error_guard.error),
error_guard.error);
+ result = AdbcDatabaseInit(db.get(), &error_guard.error);
+ if (result != ADBC_STATUS_OK) {
+ std::ignore = AdbcDatabaseRelease(db.get(), nullptr);
+ }
Review Comment:
`std::ignore` is defined in `<tuple>`, which isn't included in this file. To
avoid relying on transitive includes (and potential build breaks), prefer an
explicit `(void)` cast when intentionally ignoring a return value.
##########
java/driver/jni/src/main/cpp/jni_wrapper.cc:
##########
@@ -443,12 +472,14 @@
Java_org_apache_arrow_adbc_driver_jni_impl_NativeAdbc_openStatement(
CHECK_ADBC_ERROR(AdbcStatementNew(conn, stmt.get(), &error_guard.error),
error_guard.error);
- jclass native_handle_class = RequireImplClass(env,
"NativeStatementHandle");
- jmethodID native_handle_ctor =
- RequireMethod(env, native_handle_class, "<init>", "(J)V");
jobject object =
env->NewObject(native_handle_class, native_handle_ctor,
static_cast<jlong>(reinterpret_cast<uintptr_t>(stmt.get())));
+ if (object == nullptr || env->ExceptionCheck()) {
+ // Failed to construct Java object: try to release ADBC handle
+ std::ignore = AdbcStatementRelease(stmt.get(), nullptr);
+ return nullptr;
+ }
Review Comment:
`std::ignore` is defined in `<tuple>`, which isn't included in this file. To
avoid relying on transitive includes (and potential build breaks), prefer an
explicit `(void)` cast when intentionally ignoring a return value.
##########
java/driver/jni/src/main/cpp/jni_wrapper.cc:
##########
@@ -351,18 +357,28 @@
Java_org_apache_arrow_adbc_driver_jni_impl_NativeAdbc_openDatabase(
JniStringView key_str(env, key);
JniStringView value_str(env, value);
- CHECK_ADBC_ERROR(AdbcDatabaseSetOption(db.get(), key_str.value,
value_str.value,
- &error_guard.error),
- error_guard.error);
+ result = AdbcDatabaseSetOption(db.get(), key_str.value, value_str.value,
+ &error_guard.error);
+ if (result != ADBC_STATUS_OK) {
+ std::ignore = AdbcDatabaseRelease(db.get(), nullptr);
+ }
+ CHECK_ADBC_ERROR(result, error_guard.error);
}
- CHECK_ADBC_ERROR(AdbcDatabaseInit(db.get(), &error_guard.error),
error_guard.error);
+ result = AdbcDatabaseInit(db.get(), &error_guard.error);
+ if (result != ADBC_STATUS_OK) {
+ std::ignore = AdbcDatabaseRelease(db.get(), nullptr);
+ }
+ CHECK_ADBC_ERROR(result, error_guard.error);
- jclass nativeHandleKlass = RequireImplClass(env, "NativeDatabaseHandle");
- jmethodID nativeHandleCtor = RequireMethod(env, nativeHandleKlass,
"<init>", "(J)V");
jobject object =
env->NewObject(nativeHandleKlass, nativeHandleCtor,
static_cast<jlong>(reinterpret_cast<uintptr_t>(db.get())));
+ if (object == nullptr || env->ExceptionCheck()) {
+ // Failed to construct Java object: try to release ADBC handle
+ std::ignore = AdbcDatabaseRelease(db.get(), nullptr);
+ return nullptr;
+ }
Review Comment:
`std::ignore` is defined in `<tuple>`, which isn't included in this file. To
avoid relying on transitive includes (and potential build breaks), prefer an
explicit `(void)` cast when intentionally ignoring a return value.
##########
java/driver/jni/src/main/cpp/jni_wrapper.cc:
##########
@@ -351,18 +357,28 @@
Java_org_apache_arrow_adbc_driver_jni_impl_NativeAdbc_openDatabase(
JniStringView key_str(env, key);
JniStringView value_str(env, value);
- CHECK_ADBC_ERROR(AdbcDatabaseSetOption(db.get(), key_str.value,
value_str.value,
- &error_guard.error),
- error_guard.error);
+ result = AdbcDatabaseSetOption(db.get(), key_str.value, value_str.value,
+ &error_guard.error);
+ if (result != ADBC_STATUS_OK) {
+ std::ignore = AdbcDatabaseRelease(db.get(), nullptr);
+ }
+ CHECK_ADBC_ERROR(result, error_guard.error);
Review Comment:
`openDatabase` now releases the ADBC database on ADBC call failures and on
`NewObject` failure, but it can still leak after `AdbcDatabaseNew()` if a C++
exception is thrown during parameter processing (e.g., `JniStringView` throws
on OOM / failed `GetStringUTFChars`). Since `AdbcDatabaseNew` allocates
`private_data` (see ADBC docs), this should be paired with
`AdbcDatabaseRelease` on *any* exit path before ownership is transferred to
Java. Consider introducing a small RAII scope guard (or a `unique_ptr` with a
custom deleter) that calls `AdbcDatabaseRelease(db.get(), nullptr)` unless
`db.release()` has been reached.
##########
java/driver/jni/src/main/cpp/jni_wrapper.cc:
##########
@@ -398,15 +418,20 @@
Java_org_apache_arrow_adbc_driver_jni_impl_NativeAdbc_openConnection(
CHECK_ADBC_ERROR(AdbcConnectionNew(conn.get(), &error_guard.error),
error_guard.error);
- CHECK_ADBC_ERROR(AdbcConnectionInit(conn.get(), db, &error_guard.error),
- error_guard.error);
+ auto result = AdbcConnectionInit(conn.get(), db, &error_guard.error);
+ if (result != ADBC_STATUS_OK) {
+ std::ignore = AdbcConnectionRelease(conn.get(), nullptr);
+ }
Review Comment:
`std::ignore` is defined in `<tuple>`, which isn't included in this file. To
avoid relying on transitive includes (and potential build breaks), prefer an
explicit `(void)` cast when intentionally ignoring a return value.
##########
java/driver/jni/src/main/cpp/jni_wrapper.cc:
##########
@@ -398,15 +418,20 @@
Java_org_apache_arrow_adbc_driver_jni_impl_NativeAdbc_openConnection(
CHECK_ADBC_ERROR(AdbcConnectionNew(conn.get(), &error_guard.error),
error_guard.error);
- CHECK_ADBC_ERROR(AdbcConnectionInit(conn.get(), db, &error_guard.error),
- error_guard.error);
+ auto result = AdbcConnectionInit(conn.get(), db, &error_guard.error);
+ if (result != ADBC_STATUS_OK) {
+ std::ignore = AdbcConnectionRelease(conn.get(), nullptr);
+ }
+ CHECK_ADBC_ERROR(result, error_guard.error);
- jclass native_handle_class = RequireImplClass(env,
"NativeConnectionHandle");
- jmethodID native_handle_ctor =
- RequireMethod(env, native_handle_class, "<init>", "(J)V");
jobject object =
env->NewObject(native_handle_class, native_handle_ctor,
static_cast<jlong>(reinterpret_cast<uintptr_t>(conn.get())));
+ if (object == nullptr || env->ExceptionCheck()) {
+ // Failed to construct Java object: try to release ADBC handle
+ std::ignore = AdbcConnectionRelease(conn.get(), nullptr);
+ return nullptr;
+ }
Review Comment:
`std::ignore` is defined in `<tuple>`, which isn't included in this file. To
avoid relying on transitive includes (and potential build breaks), prefer an
explicit `(void)` cast when intentionally ignoring a return value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
