Fix potential 'use-after-free' by Admin 'SHOW PROCESSLIST'#5180
Fix potential 'use-after-free' by Admin 'SHOW PROCESSLIST'#5180renecannao merged 2 commits intov3.0from
Conversation
Accesses by 'stats___mysql_processlist' to 'myconn->query.ptr' could lead to invalid memory accesses, as the pointed query could already have been free by the session after being issued.
As per the GCC manual, calls to 'strn*' function family shall be replaced by 'memcpy' when truncation (no null termination) is expected.
|
Summary of ChangesHello @JavierJF, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical use-after-free vulnerability in ProxySQL that could be triggered by concurrent Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a critical use-after-free vulnerability that occurs during SHOW PROCESSLIST on the admin interface when a query is being rewritten. By switching from the potentially stale myconn->query.ptr to the session-scoped CurrentQuery.QueryPointer, the race condition is resolved. The change from strncpy to memcpy is also a good improvement for safer memory handling. I've suggested one minor change for consistency.
| memcpy(res, query_ptr, query_len - 3); | ||
| memcpy(res + (query_len - 3), "...", 3); | ||
| } else { | ||
| strncpy(res, query_ptr, query_len); |
There was a problem hiding this comment.
For consistency with the changes in the if (trunc_query) block, it would be better to use memcpy here as well instead of strncpy. While the current code is safe due to the manual null-termination on line 8153, using memcpy consistently makes the intent clearer (copying a known number of bytes) and is generally safer, reducing reliance on the specifics of strncpy's behavior.
memcpy(res, query_ptr, query_len);|
Can one of the admins verify this patch? |
wazir-ahmed
left a comment
There was a problem hiding this comment.
@JavierJF We have to fix PgSQL_Session::get_current_query() too.
@JavierJF We have to fix PgSQL_Session::get_current_query() too.
|
@wazir-ahmed thanks for the heads-up, your comment is addressed in #5182. Maybe we could reduce duplication in between sessions, but that looks like a future rework. |
Issue
The following
use-after-freeis very simple to reproduce:SET statementsor any other query that is internally re-writed.SHOW PROCESSLISTto the Admin interface.The following 'use-after-free' is likely to take place:
Solution
Instead of using the query present in
myconn, which could be allocated and freed fromSessionin case of rewrites. We use the query present inSession(CurrentQuery), which scope is well defined, and shouldn't point to invalid memory.A small compilation warning was also fixed.