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

Harden against Postgres interactions with C++ destructors #93

Open
JelteF opened this issue Aug 6, 2024 · 2 comments
Open

Harden against Postgres interactions with C++ destructors #93

JelteF opened this issue Aug 6, 2024 · 2 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@JelteF
Copy link
Collaborator

JelteF commented Aug 6, 2024

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 use sigsetjmp and longjump to traverse the the stack until the next PG_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 by lock_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-postgres

@JelteF JelteF added the bug Something isn't working label Aug 6, 2024
@wuputah wuputah changed the title Harden against Postgres its terrible interaction with C++ destructors Harden against Postgres interactions with C++ destructors Aug 7, 2024
@JelteF JelteF added this to the 0.1.0 milestone Aug 22, 2024
@mkaruza mkaruza self-assigned this Sep 11, 2024
@wuputah wuputah removed the bug Something isn't working label Sep 27, 2024
@wuputah
Copy link
Collaborator

wuputah commented Sep 27, 2024

-bug as this does not have a specific bug report to fix, but instead is an approach to improve overall error handling. That said, we have agreed that this is a good idea and we want to move towards having all interactions between C and C++ be in a single file similar to how you would use FFI -- but we won't try to catch memory allocation errors.

@JelteF JelteF added the enhancement New feature or request label Sep 30, 2024
Y-- added a commit that referenced this issue Oct 7, 2024
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)
@JelteF
Copy link
Collaborator Author

JelteF commented Oct 15, 2024

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.

@JelteF JelteF modified the milestones: 0.1.0 code complete, 0.2.0 Oct 15, 2024
@JelteF JelteF modified the milestones: 0.2.0, 0.3.0 Dec 10, 2024
@JelteF JelteF modified the milestones: 0.3.0, 0.4.0 Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants