duckdb / duckdb/pg_duckdb

Harden against Postgres interactions with C++ destructors

Open
#93 2 comments 2 reactions 1 assignee Claimed by @mkaruza View on GitHub
enhancement
Dominant language
C++
Stars
3.2k
Forks
204
PR merge metrics
No merged PRs in 30d

Description

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

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.