Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

c/driver_manager: Python bindings can segfault if AdbcDatabaseInit() errors #2264

Closed
paleolimbot opened this issue Oct 21, 2024 · 1 comment · Fixed by #2266
Closed

c/driver_manager: Python bindings can segfault if AdbcDatabaseInit() errors #2264

paleolimbot opened this issue Oct 21, 2024 · 1 comment · Fixed by #2266
Labels
Type: bug Something isn't working

Comments

@paleolimbot
Copy link
Member

paleolimbot commented Oct 21, 2024

What happened?

When testing the framework driver in #2186, the driver manager segfaulted when the database init errored. The segfault happened here:

return error->private_driver->ErrorGetDetailCount(error);

...because the function pointer for the error detail was nullptr.

I think this is because the driver had been released on a call to SetOption():

database->private_driver->release(database->private_driver, error);

I am wondering if we hadn't seen this because typically database options are checked when the connection is initialized (or because drivers weren't zeroing the function pointers on release until using the framework).

An easy stopgap would be to also check that error->private_driver->ErrorGetDetail != nullptr here:

int AdbcErrorGetDetailCount(const struct AdbcError* error) {
if (error->vendor_code == ADBC_ERROR_VENDOR_CODE_PRIVATE_DATA && error->private_data &&
error->private_driver) {
return error->private_driver->ErrorGetDetailCount(error);
}
return 0;
}

..or to zero out the error->private_driver after releasing the driver.

Stack Trace

No response

How can we reproduce the bug?

No response

Environment/Setup

No response

@paleolimbot paleolimbot added the Type: bug Something isn't working label Oct 21, 2024
@lidavidm
Copy link
Member

I think we should make both checks

This sort of thing is also why unloading drivers in the driver manager is hard/has been punted on: errors stick around and need a way to be released...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: bug Something isn't working
Projects
None yet
2 participants