-
Notifications
You must be signed in to change notification settings - Fork 66
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
Harden against Postgres interactions with C++ destructors #93
Comments
|
When one calls `elog(ERROR,...` it `longjmp` out of the current stack leaving no chance for the C++ to be executed. This PR is a first iteration toward what we want to rationalize (cf. #93 ): * keep a function with a pure C++ "flavor" (object creation, exceptions, etc.) * call it in a new wrapper that copies out the error message and uses PG mechanism to throw the error This fixes #120, even though there still seem to be a small leak with DuckDB's `PrepareQuery` leftover (to be followed-up separately)
I'm going to move this issue to 0.2.0, so we can start stability testing for 0.1.0. This is an important issue, but we fixed the worst cases. We'll fix more in the next release. |
While looking at the the C++ guidelines in the CONTRIBUTING.md file proposed in #90, I realized/remembered that Postgres C and regular C++ are not automatically the good friends that the project currently assumes they are.
Specifically if Postgres throws an
ERROR
, it will usesigsetjmp
andlongjump
to traverse the the stack until the nextPG_TRY
. This is really problematic when considering the way C++ destructors work. Because the longjump will jump over these destructors. This can cause many different problems like memory leaks or locks acquired bylock_guard
not being released.The other way around is similarly bad. Any C++ exceptions that are not caught will skip over any cleanup that Postgres does in
PG_CATCH
. And thus probably killing the whole Postgres process tree.This is all listed in the postgres docs as well: https://www.postgresql.org/docs/current/xfunc-c.html#EXTEND-CPP
The only way to fix these problems in a managable way I think, is by making the boundaries of calling C++ from C and C from C++ much clearer than they are now (possibly by never including both duckdb headers and postgres headers in the same files). And then at these boundaries convert C++ exceptions to Postgres errors and the other way around.
P.S. This same problem applies to Rust and was worked around by
pgrx
by implementing panic -> pgerror and pgerror -> panic conversion at every FFI boundary: https://github.com/pgcentralfoundation/pgrx/blob/develop/docs/src/design-decisions.md#protecting-the-rust-stack-read-lying-to-postgresThe text was updated successfully, but these errors were encountered: