This is an automated email from the ASF dual-hosted git repository.
lidavidm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-adbc.git
The following commit(s) were added to refs/heads/main by this push:
new 131b3d260 fix(c/driver_manager): look for correct init symbol name
(#3681)
131b3d260 is described below
commit 131b3d2606a4e940ec5d221dc1fadd7cffda6d35
Author: David Li <[email protected]>
AuthorDate: Wed Nov 5 08:55:28 2025 +0900
fix(c/driver_manager): look for correct init symbol name (#3681)
- Add sanity assertions
- Actually test this end-to-end
- Populate lib_path properly so the right init symbol name gets
calculated
IMO, the control flow now is very convoluted because it intermixes
searching and loading, which led to this regression since it's hard to
tell if the return value is actually fully initialized. I'm still not
sure all code paths are correct. I may try to factor searching/loading
out separately.
Closes #3680.
---
c/driver_manager/CMakeLists.txt | 20 +++++++++++
c/driver_manager/adbc_driver_manager.cc | 54 +++++++++++++++++++++-------
c/driver_manager/adbc_driver_manager_test.cc | 31 ++++++++++++++++
c/driver_manager/entrypoint.c | 23 ++++++++++++
c/validation/adbc_validation_util.h | 13 +++++++
go/adbc/drivermgr/adbc_driver_manager.cc | 54 +++++++++++++++++++++-------
6 files changed, 169 insertions(+), 26 deletions(-)
diff --git a/c/driver_manager/CMakeLists.txt b/c/driver_manager/CMakeLists.txt
index 198c3f979..25fb6fe8c 100644
--- a/c/driver_manager/CMakeLists.txt
+++ b/c/driver_manager/CMakeLists.txt
@@ -84,6 +84,16 @@ if(ADBC_BUILD_TESTS)
set(TEST_LINK_LIBS adbc_driver_manager_static)
endif()
+ add_library(adbc_driver_entrypoint SHARED entrypoint.c)
+ target_include_directories(adbc_driver_entrypoint SYSTEM
+ PRIVATE ${REPOSITORY_ROOT}/c/include/)
+ target_compile_definitions(adbc_driver_entrypoint PRIVATE ADBC_EXPORTING)
+
+ add_library(adbc_driver_no_entrypoint SHARED entrypoint.c)
+ target_include_directories(adbc_driver_no_entrypoint SYSTEM
+ PRIVATE ${REPOSITORY_ROOT}/c/include/)
+ target_compile_definitions(adbc_driver_no_entrypoint PRIVATE ADBC_EXPORTING)
+
add_test_case(driver_manager_test
PREFIX
adbc
@@ -96,6 +106,16 @@ if(ADBC_BUILD_TESTS)
adbc_validation
${TEST_LINK_LIBS})
target_compile_features(adbc-driver-manager-test PRIVATE cxx_std_17)
+ add_dependencies(adbc-driver-manager-test adbc_driver_entrypoint
+ adbc_driver_no_entrypoint)
+
+ target_compile_definitions(adbc-driver-manager-test
+ PRIVATE
ADBC_DRIVER_MANAGER_ENTRYPOINT_TEST_LIB="${CMAKE_BINARY_DIR}/driver_manager/${CMAKE_SHARED_LIBRARY_PREFIX}adbc_driver_entrypoint${CMAKE_SHARED_LIBRARY_SUFFIX}"
+ )
+
+ target_compile_definitions(adbc-driver-manager-test
+ PRIVATE
ADBC_DRIVER_MANAGER_NO_ENTRYPOINT_TEST_LIB="${CMAKE_BINARY_DIR}/driver_manager/${CMAKE_SHARED_LIBRARY_PREFIX}adbc_driver_no_entrypoint${CMAKE_SHARED_LIBRARY_SUFFIX}"
+ )
if(ADBC_DRIVER_SQLITE)
target_compile_definitions(adbc-driver-manager-test
diff --git a/c/driver_manager/adbc_driver_manager.cc
b/c/driver_manager/adbc_driver_manager.cc
index 3beec2f0b..29a4b6a0f 100644
--- a/c/driver_manager/adbc_driver_manager.cc
+++ b/c/driver_manager/adbc_driver_manager.cc
@@ -40,8 +40,10 @@
#include <algorithm>
#include <array>
+#include <cassert>
#include <cctype>
#include <cerrno>
+#include <cstdio>
#include <cstring>
#include <filesystem>
#include <string>
@@ -170,6 +172,24 @@ void SetError(struct AdbcError* error, const std::string&
message) {
error->release = ReleaseError;
}
+void AppendError(struct AdbcError* error, const std::string& message) {
+ if (!error) return;
+ if (!error->release || !error->message) {
+ SetError(error, message);
+ return;
+ }
+
+ size_t original_length = std::strlen(error->message);
+ size_t combined_length = original_length + 1 + message.size() + 1;
+ char* new_message = new char[combined_length];
+ std::ignore = std::snprintf(new_message, combined_length, "%s\n%s",
error->message,
+ message.c_str());
+
+ error->release(error);
+ error->message = new_message;
+ error->release = ReleaseError;
+}
+
// Copies src_error into error and releases src_error
void SetError(struct AdbcError* error, struct AdbcError* src_error) {
if (!error) return;
@@ -230,7 +250,7 @@ using char_type = char;
/// \brief The location and entrypoint of a resolved driver.
struct DriverInfo {
std::string manifest_file;
- int64_t manifest_version;
+ int64_t manifest_version = 0;
std::string driver_name;
std::filesystem::path lib_path;
std::string entrypoint;
@@ -410,16 +430,14 @@ AdbcStatusCode LoadDriverManifest(const
std::filesystem::path& driver_manifest,
info.lib_path = path->get();
return ADBC_STATUS_OK;
- } else {
- std::string message = "Driver path not found in manifest '";
- message += driver_manifest.string();
- message += "' for current architecture '";
- message += adbc::CurrentArch();
- message += "'. Value was not a string";
- SetError(error, std::move(message));
- return ADBC_STATUS_INVALID_ARGUMENT;
}
- return ADBC_STATUS_OK;
+ std::string message = "Driver path not found in manifest '";
+ message += driver_manifest.string();
+ message += "' for current architecture '";
+ message += adbc::CurrentArch();
+ message += "'. Value was not a string";
+ SetError(error, std::move(message));
+ return ADBC_STATUS_INVALID_ARGUMENT;
} else if (auto* path = driver.as_string()) {
info.lib_path = path->get();
if (info.lib_path.empty()) {
@@ -569,6 +587,7 @@ struct ManagedLibrary {
// if the extension is not .toml, then just try to load the provided
// path as if it was an absolute path to a driver library
+ info.lib_path = driver_path;
return Load(driver_path.c_str(), {}, error);
}
@@ -584,8 +603,8 @@ struct ManagedLibrary {
}
driver_path.replace_extension("");
- info.lib_path = driver_path;
// otherwise just try to load the provided path as if it was an absolute
path
+ info.lib_path = driver_path;
return Load(driver_path.c_str(), {}, error);
}
@@ -614,6 +633,10 @@ struct ManagedLibrary {
// not an absolute path, no extension. Let's search the configured paths
// based on the options
+ // FindDriver will set info.lib_path
+ // XXX(lidavidm): the control flow in this call chain is excessively
+ // convoluted and it's hard to determine if DriverInfo is fully
+ // initialized or not in all non-error paths
return FindDriver(driver_path, load_options, additional_search_paths,
info, error);
}
@@ -691,6 +714,7 @@ struct ManagedLibrary {
// Don't pass error here - it'll be suppressed anyways
auto status = Load(full_path.c_str(), {}, nullptr);
if (status == ADBC_STATUS_OK) {
+ info.lib_path = full_path;
return status;
}
}
@@ -941,7 +965,7 @@ struct ManagedLibrary {
message += name;
message += ") failed: ";
GetWinError(&message);
- SetError(error, message);
+ AppendError(error, message);
return ADBC_STATUS_INTERNAL;
}
#else
@@ -951,7 +975,7 @@ struct ManagedLibrary {
message += name;
message += ") failed: ";
message += dlerror();
- SetError(error, message);
+ AppendError(error, message);
return ADBC_STATUS_INTERNAL;
}
#endif // defined(_WIN32)
@@ -1459,6 +1483,9 @@ std::string
InternalAdbcDriverManagerDefaultEntrypoint(const std::string& driver
/// - adbc_driver_sqlite.dll -> AdbcDriverSqliteInit
/// - proprietary_driver.dll -> AdbcProprietaryDriverInit
+ // N.B.(https://github.com/apache/arrow-adbc/issues/3680): sanity checks
+ assert(!driver.empty());
+
// Potential path -> filename
// Treat both \ and / as directory separators on all platforms for simplicity
std::string filename;
@@ -2569,6 +2596,7 @@ AdbcStatusCode AdbcFindLoadDriver(const char*
driver_name, const char* entrypoin
status = library.Lookup(info.entrypoint.c_str(), &load_handle, error);
} else {
auto name =
InternalAdbcDriverManagerDefaultEntrypoint(info.lib_path.string());
+ assert(!name.empty());
status = library.Lookup(name.c_str(), &load_handle, error);
if (status != ADBC_STATUS_OK) {
status = library.Lookup(kDefaultEntrypoint, &load_handle, error);
diff --git a/c/driver_manager/adbc_driver_manager_test.cc
b/c/driver_manager/adbc_driver_manager_test.cc
index 73aa5383f..85a53c04a 100644
--- a/c/driver_manager/adbc_driver_manager_test.cc
+++ b/c/driver_manager/adbc_driver_manager_test.cc
@@ -171,6 +171,37 @@ TEST_F(DriverManager, MultiDriverTest) {
error->release(&error.value);
}
+TEST_F(DriverManager, NoDefaultEntrypoint) {
+#if !defined(ADBC_DRIVER_MANAGER_ENTRYPOINT_TEST_LIB)
+ GTEST_SKIP() << "ADBC_DRIVER_MANAGER_ENTRYPOINT_TEST_LIB is not defined";
+#else
+ adbc_validation::Handle<struct AdbcError> error;
+ adbc_validation::Handle<struct AdbcDriver> driver;
+ // Must fail with expected status
+ ASSERT_THAT(AdbcLoadDriver(ADBC_DRIVER_MANAGER_ENTRYPOINT_TEST_LIB, nullptr,
+ ADBC_VERSION_1_1_0, &driver.value, &error.value),
+ IsStatus(ADBC_STATUS_IO, &error.value));
+
+#endif // !defined(ADBC_DRIVER_MANAGER_ENTRYPOINT_TEST_LIB)
+}
+
+TEST_F(DriverManager, NoDefaultEntrypointFound) {
+#if !defined(ADBC_DRIVER_MANAGER_NO_ENTRYPOINT_TEST_LIB)
+ GTEST_SKIP() << "ADBC_DRIVER_MANAGER_NO_ENTRYPOINT_TEST_LIB is not defined";
+#else
+ adbc_validation::Handle<struct AdbcError> error;
+ adbc_validation::Handle<struct AdbcDriver> driver;
+ ASSERT_THAT(AdbcLoadDriver(ADBC_DRIVER_MANAGER_NO_ENTRYPOINT_TEST_LIB,
nullptr,
+ ADBC_VERSION_1_1_0, &driver.value, &error.value),
+ IsStatus(ADBC_STATUS_INTERNAL, &error.value));
+ // Both symbols should not be found, should be mentioned in error message
+ ASSERT_THAT(error->message,
+ ::testing::HasSubstr("(AdbcDriverNoEntrypointInit) failed"));
+ ASSERT_THAT(error->message, ::testing::HasSubstr("(AdbcDriverInit) failed"));
+
+#endif // !defined(ADBC_DRIVER_MANAGER_NO_ENTRYPOINT_TEST_LIB)
+}
+
class SqliteQuirks : public adbc_validation::DriverQuirks {
public:
AdbcStatusCode SetupDatabase(struct AdbcDatabase* database,
diff --git a/c/driver_manager/entrypoint.c b/c/driver_manager/entrypoint.c
new file mode 100644
index 000000000..0e869676e
--- /dev/null
+++ b/c/driver_manager/entrypoint.c
@@ -0,0 +1,23 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow-adbc/adbc.h"
+
+ADBC_EXPORT AdbcStatusCode AdbcDriverEntrypointInit(int version, void*
raw_driver,
+ struct AdbcError* error) {
+ return ADBC_STATUS_IO;
+}
diff --git a/c/validation/adbc_validation_util.h
b/c/validation/adbc_validation_util.h
index d3eab643d..57f3859f4 100644
--- a/c/validation/adbc_validation_util.h
+++ b/c/validation/adbc_validation_util.h
@@ -123,6 +123,19 @@ struct Releaser<struct AdbcDatabase> {
}
};
+template <>
+struct Releaser<struct AdbcDriver> {
+ static void Release(struct AdbcDriver* value) {
+ if (value->release) {
+ struct AdbcError error = {};
+ auto status = value->release(value, &error);
+ if (status != ADBC_STATUS_OK) {
+ FAIL() << StatusCodeToString(status) << ": " << ToString(&error);
+ }
+ }
+ }
+};
+
template <>
struct Releaser<struct AdbcStatement> {
static void Release(struct AdbcStatement* value) {
diff --git a/go/adbc/drivermgr/adbc_driver_manager.cc
b/go/adbc/drivermgr/adbc_driver_manager.cc
index 3beec2f0b..29a4b6a0f 100644
--- a/go/adbc/drivermgr/adbc_driver_manager.cc
+++ b/go/adbc/drivermgr/adbc_driver_manager.cc
@@ -40,8 +40,10 @@
#include <algorithm>
#include <array>
+#include <cassert>
#include <cctype>
#include <cerrno>
+#include <cstdio>
#include <cstring>
#include <filesystem>
#include <string>
@@ -170,6 +172,24 @@ void SetError(struct AdbcError* error, const std::string&
message) {
error->release = ReleaseError;
}
+void AppendError(struct AdbcError* error, const std::string& message) {
+ if (!error) return;
+ if (!error->release || !error->message) {
+ SetError(error, message);
+ return;
+ }
+
+ size_t original_length = std::strlen(error->message);
+ size_t combined_length = original_length + 1 + message.size() + 1;
+ char* new_message = new char[combined_length];
+ std::ignore = std::snprintf(new_message, combined_length, "%s\n%s",
error->message,
+ message.c_str());
+
+ error->release(error);
+ error->message = new_message;
+ error->release = ReleaseError;
+}
+
// Copies src_error into error and releases src_error
void SetError(struct AdbcError* error, struct AdbcError* src_error) {
if (!error) return;
@@ -230,7 +250,7 @@ using char_type = char;
/// \brief The location and entrypoint of a resolved driver.
struct DriverInfo {
std::string manifest_file;
- int64_t manifest_version;
+ int64_t manifest_version = 0;
std::string driver_name;
std::filesystem::path lib_path;
std::string entrypoint;
@@ -410,16 +430,14 @@ AdbcStatusCode LoadDriverManifest(const
std::filesystem::path& driver_manifest,
info.lib_path = path->get();
return ADBC_STATUS_OK;
- } else {
- std::string message = "Driver path not found in manifest '";
- message += driver_manifest.string();
- message += "' for current architecture '";
- message += adbc::CurrentArch();
- message += "'. Value was not a string";
- SetError(error, std::move(message));
- return ADBC_STATUS_INVALID_ARGUMENT;
}
- return ADBC_STATUS_OK;
+ std::string message = "Driver path not found in manifest '";
+ message += driver_manifest.string();
+ message += "' for current architecture '";
+ message += adbc::CurrentArch();
+ message += "'. Value was not a string";
+ SetError(error, std::move(message));
+ return ADBC_STATUS_INVALID_ARGUMENT;
} else if (auto* path = driver.as_string()) {
info.lib_path = path->get();
if (info.lib_path.empty()) {
@@ -569,6 +587,7 @@ struct ManagedLibrary {
// if the extension is not .toml, then just try to load the provided
// path as if it was an absolute path to a driver library
+ info.lib_path = driver_path;
return Load(driver_path.c_str(), {}, error);
}
@@ -584,8 +603,8 @@ struct ManagedLibrary {
}
driver_path.replace_extension("");
- info.lib_path = driver_path;
// otherwise just try to load the provided path as if it was an absolute
path
+ info.lib_path = driver_path;
return Load(driver_path.c_str(), {}, error);
}
@@ -614,6 +633,10 @@ struct ManagedLibrary {
// not an absolute path, no extension. Let's search the configured paths
// based on the options
+ // FindDriver will set info.lib_path
+ // XXX(lidavidm): the control flow in this call chain is excessively
+ // convoluted and it's hard to determine if DriverInfo is fully
+ // initialized or not in all non-error paths
return FindDriver(driver_path, load_options, additional_search_paths,
info, error);
}
@@ -691,6 +714,7 @@ struct ManagedLibrary {
// Don't pass error here - it'll be suppressed anyways
auto status = Load(full_path.c_str(), {}, nullptr);
if (status == ADBC_STATUS_OK) {
+ info.lib_path = full_path;
return status;
}
}
@@ -941,7 +965,7 @@ struct ManagedLibrary {
message += name;
message += ") failed: ";
GetWinError(&message);
- SetError(error, message);
+ AppendError(error, message);
return ADBC_STATUS_INTERNAL;
}
#else
@@ -951,7 +975,7 @@ struct ManagedLibrary {
message += name;
message += ") failed: ";
message += dlerror();
- SetError(error, message);
+ AppendError(error, message);
return ADBC_STATUS_INTERNAL;
}
#endif // defined(_WIN32)
@@ -1459,6 +1483,9 @@ std::string
InternalAdbcDriverManagerDefaultEntrypoint(const std::string& driver
/// - adbc_driver_sqlite.dll -> AdbcDriverSqliteInit
/// - proprietary_driver.dll -> AdbcProprietaryDriverInit
+ // N.B.(https://github.com/apache/arrow-adbc/issues/3680): sanity checks
+ assert(!driver.empty());
+
// Potential path -> filename
// Treat both \ and / as directory separators on all platforms for simplicity
std::string filename;
@@ -2569,6 +2596,7 @@ AdbcStatusCode AdbcFindLoadDriver(const char*
driver_name, const char* entrypoin
status = library.Lookup(info.entrypoint.c_str(), &load_handle, error);
} else {
auto name =
InternalAdbcDriverManagerDefaultEntrypoint(info.lib_path.string());
+ assert(!name.empty());
status = library.Lookup(name.c_str(), &load_handle, error);
if (status != ADBC_STATUS_OK) {
status = library.Lookup(kDefaultEntrypoint, &load_handle, error);
