Remove GVL unlocking at all functions which process data modifiable in a second thread#723
Remove GVL unlocking at all functions which process data modifiable in a second thread#723larskanis wants to merge 1 commit into
Conversation
larskanis
changed the title
2facd7a to
b9a2e30
Compare
…n a second thread This removes possible VM crashs when data to be sent is modified/cleared in a second thread. It works by keeping the GVL lock for libpq functions that don't immediately process all the data and don't make a copy of it. These are the `PQsend*`, `PQexec*` and some related functions. Since pg-1.3 all the blocking functions or states are avoided by using the non-blocking API of libpq. Therefore holding the GVL somewhat longer shouldn't matter that much. Having some libpq function with and without unlocked GVL, results in `rb_thread_call_with_gvl()` sometimes needed and sometimes not to process callbacks. Therefore `ruby_thread_has_gvl_p()` is used to check if it's needed on ruby<4.0. In ruby-4.0+ `rb_thread_call_with_gvl()` doesn't care about whether GVL is already locked or not, so that it can be called in both cases. Fixes #721
cbandy
left a comment
cbandy
left a comment
There was a problem hiding this comment.
📝 PR title says GLV instead of GVL
|
|
||
|
|
||
| /* | ||
| * Definitions of blocking functions and their parameters |
There was a problem hiding this comment.
It might be good to explain how one decides which functions to include/exclude from this list.
| function(PQsendQuery, GVL_TYPE_NONVOID, int, const char *, query) \ | ||
| function(PQsendQueryParams, GVL_TYPE_NONVOID, int, int, resultFormat) \ | ||
| function(PQsendPrepare, GVL_TYPE_NONVOID, int, const Oid *, paramTypes) \ | ||
| function(PQsendQueryPrepared, GVL_TYPE_NONVOID, int, int, resultFormat) \ | ||
| function(PQsendDescribePrepared, GVL_TYPE_NONVOID, int, const char *, stmt) \ | ||
| function(PQsendDescribePortal, GVL_TYPE_NONVOID, int, const char *, portal) \ | ||
| function(PQsendClosePrepared, GVL_TYPE_NONVOID, int, const char *, stmt) \ | ||
| function(PQsendClosePortal, GVL_TYPE_NONVOID, int, const char *, portal) \ |
There was a problem hiding this comment.
These are async, and docs describe them as returns 1 if it was able to dispatch the request, and 0 if not. Is it just a socket write? What can block during these?
There was a problem hiding this comment.
The PQsend* functions primary allocate and copy the data to be send into libpq's internal buffers. They try to send data, but that isn't enforced. Sending is repeatedly done by other functions, like PQflush or enforced by PQgetResult. So PQsend* functions don't block, unless libpq is broken.
larskanis
changed the title
|
I tend to say "no" to this PR. There's already the install option to avoid releasing GVL at all, which disables this kind of attack: gem inst pg -- --disable-gvl-unlockAs written here it's questionable if this kind of attack is necessary to be solved. And responses seem to acknowledge that. |
2 participants
This removes possible VM crashs when data to be sent is modified/cleared in a second thread.
It works by keeping the GVL lock for libpq functions that don't immediately process all the data and don't make a copy of it.
These are the
PQsend*,PQexec*and some related functions.Since pg-1.3 all the blocking functions or states are avoided by using the non-blocking API of libpq.
Therefore holding the GVL somewhat longer shouldn't matter that much.
Having some libpq function with and without unlocked GVL, results in
rb_thread_call_with_gvl()sometimes needed and sometimes not to process callbacks.Therefore
ruby_thread_has_gvl_p()is used to check if it's needed on ruby<4.0.In ruby-4.0+
rb_thread_call_with_gvl()doesn't care about whether GVL is already locked or not, so that it can be called in both cases.Fixes #721