Make prepared statement abstractions respect {mysql,pgsql}-query_digests_keep_comment when computing digests.#5392
Conversation
Summary of ChangesHello @proton-lisandro-pin, 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 enhances the prepared statement abstraction layer for MySQL and PostgreSQL by introducing configurable comment stripping during query digest computation. This change prevents the cache from growing unnecessarily large due to variations in query annotations, aligning the digest calculation with user-defined settings for keeping or stripping comments. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly extends the digest computation for prepared statements to respect the mysql-query_digests_keep_comment and pgsql-query_digests_keep_comment settings. This is achieved by introducing a new strip_query_comments option to the hash computation functions.
However, I've identified a critical bug in both MySQL_PreparedStatement.cpp and PgSQL_PreparedStatement.cpp where an uninitialized variable q is passed to mysql_query_strip_comments and pgsql_query_strip_comments respectively. This will cause undefined behavior and must be fixed. I've provided suggestions to correct this.
proton-lisandro-pin
force-pushed
the
prepared_stmt_cache_comment_fix
branch
from
e45d416 to
b07af3d
Compare
📝 WalkthroughWalkthroughAdds an options-based prepared-statement hashing path for MySQL and PostgreSQL that can optionally strip query comments before computing digests by introducing Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
proton-lisandro-pin
force-pushed
the
prepared_stmt_cache_comment_fix
branch
from
b07af3d to
17a254d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/MySQL_PreparedStatement.cpp`:
- Around line 54-63: The call to mysql_query_strip_comments can return NULL on
allocation failure, so before calling strlen(q), memcpy or free, check the
returned pointer (q) for NULL; if NULL, fall back to copying the original query
into buf (i.e., treat as if strip_query_comments is false) or propagate an
error/return appropriately; update the block around mysql_query_strip_comments
in MySQL_PreparedStatement.cpp to guard uses of q (strlen, memcpy, free) and
ensure buf and l are updated consistently when q is NULL.
In `@lib/PgSQL_PreparedStatement.cpp`:
- Around line 55-64: The call to pgsql_query_strip_comments in
PgSQL_PreparedStatement.cpp may return NULL on allocation failure but the code
immediately calls strlen(q); add a NULL check after pgsql_query_strip_comments
(for the variable q) before dereferencing: if q is NULL, handle the error path
by falling back to copying the original query into buf (as the else branch does)
or logging/returning an error, otherwise use strlen(q), memcpy the stripped
query, and free(q); ensure no dereference of q occurs when NULL.
7d44ead to
b9ed5d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/MySQL_PreparedStatement.cpp`:
- Around line 54-66: The issue is a shadowed variable: remove the inner
declaration so the result of mysql_query_strip_comments(...) is assigned to the
outer nquery (do not re-declare with "char *"), e.g. change the line with
mysql_query_strip_comments to assign to nquery directly; keep the subsequent
query = nquery / query_length = strlen(nquery) logic and ensure the final
free(nquery) runs when nquery != NULL so the allocated buffer is freed. This
modifies the use of nquery and affects the branch that calls
mysql_query_strip_comments, ensuring no memory leak.
In `@lib/PgSQL_PreparedStatement.cpp`:
- Around line 55-67: The bug is a shadowed variable: remove the inner
redeclaration so the call to mysql_query_strip_comments assigns into the outer
nquery (declared at top) instead of creating a new local pointer; i.e., replace
"char *nquery = mysql_query_strip_comments(...)" with an assignment to nquery so
that later "if (nquery != NULL) free(nquery);" releases the buffer referenced by
query after memcpy; apply the same change in the analogous block in
lib/MySQL_PreparedStatement.cpp (look for strip_query_comments,
mysql_query_strip_comments, nquery, query, query_length, buf, and l).
b9ed5d3 to
0b3547b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/MySQL_PreparedStatement.cpp (1)
34-34:⚠️ Potential issue | 🟡 MinorCppcheck-flagged:
bufNULL-dereference on OOM (line 62).
malloc(l)at line 34 has no NULL check. If allocation fails, every subsequent use ofbuf(lines 38, 42, 46, 50, and the new line 62) is UB. CppchecknullPointerArithmeticOutOfMemoryexplicitly flags the new path at line 62.The PgSQL counterpart avoids this entirely with a stack-allocated
std::vector<char>— consider the same approach here:🛡️ Proposed fix: replace `malloc`/`free` with `std::vector` (mirrors PgSQL implementation)
- char *buf = (char *)malloc(l); + std::vector<char> storage(l); + char *buf = storage.data(); l = 0;And remove the paired
free(buf)at line 69:uint64_t hash = SpookyHash::Hash64(buf, l, 0); - free(buf); return hash;Also applies to: 62-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MySQL_PreparedStatement.cpp` at line 34, Replace the raw heap allocation "char *buf = (char *)malloc(l)" and its paired "free(buf)" with a stack-managed std::vector<char> (e.g., std::vector<char> buf(l)); update all uses of buf (indexes/ptrs at sites that reference buf, including the lines previously flagged: lines using buf at 38, 42, 46, 50 and the new use at 62) to reference buf.data() or buf[i] as appropriate, remove the free(buf) call, and add the necessary `#include` <vector>; this mirrors the PgSQL pattern and eliminates the NULL-dereference/OOM UB from malloc/free.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/PgSQL_PreparedStatement.cpp`:
- Line 57: Replace the incorrect MySQL tokenizer call in the PostgreSQL
prepared-statement implementation: in PgSQL_PreparedStatement.cpp where nquery =
mysql_query_strip_comments((char *)query, query_length, false); is used, call
pgsql_query_strip_comments((char *)query, query_length, false) instead so the
PgSQL prepared statement (and any functions that compute digests) use the
PostgreSQL-specific comment stripper; update the single call referencing
mysql_query_strip_comments to pgsql_query_strip_comments and keep the same
arguments (query, query_length, false).
---
Outside diff comments:
In `@lib/MySQL_PreparedStatement.cpp`:
- Line 34: Replace the raw heap allocation "char *buf = (char *)malloc(l)" and
its paired "free(buf)" with a stack-managed std::vector<char> (e.g.,
std::vector<char> buf(l)); update all uses of buf (indexes/ptrs at sites that
reference buf, including the lines previously flagged: lines using buf at 38,
42, 46, 50 and the new use at 62) to reference buf.data() or buf[i] as
appropriate, remove the free(buf) call, and add the necessary `#include` <vector>;
this mirrors the PgSQL pattern and eliminates the NULL-dereference/OOM UB from
malloc/free.
…ests_keep_comment` when computing digests. Fixes an issue where the cache would blow up in size upon prepared statements with query annotations. See sysown#5384 for details.
proton-lisandro-pin
force-pushed
the
prepared_stmt_cache_comment_fix
branch
from
0b3547b to
c1ff09a
Compare
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/MySQL_PreparedStatement.cpp (1)
34-34:⚠️ Potential issue | 🟡 MinorPre-existing:
buf = malloc(l)has no NULL check — Cppcheck error at line 62.If
mallocreturnsNULL, every dereference ofbuf + l(lines 38, 42, 46, 50, 62) crashes. Cppcheck flags line 62 asnullPointerArithmeticOutOfMemory. The PgSQL variant (which usesstd::vector) avoids this class of bug entirely.🛡️ Proposed fix
char *buf = (char *)malloc(l); + if (buf == NULL) { + return 0; // OOM: return 0 hash; caller must treat 0 as invalid + } l = 0;Also applies to: 62-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MySQL_PreparedStatement.cpp` at line 34, The allocation "char *buf = (char *)malloc(l);" in MySQL_PreparedStatement.cpp must be checked for NULL before any use: after malloc(l) test "if (!buf)" and handle failure by returning an error/throwing an exception or logging and cleaning up (avoid proceeding to any arithmetic or dereference of buf, e.g., the subsequent uses of buf + l at call sites in this function). Update the function that allocates buf to perform the NULL check, perform any necessary cleanup (free other resources if allocated), and return an appropriate error code or throw so callers (the prepared statement routine that uses buf) do not continue and cause a crash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/MySQL_PreparedStatement.cpp`:
- Line 34: The allocation "char *buf = (char *)malloc(l);" in
MySQL_PreparedStatement.cpp must be checked for NULL before any use: after
malloc(l) test "if (!buf)" and handle failure by returning an error/throwing an
exception or logging and cleaning up (avoid proceeding to any arithmetic or
dereference of buf, e.g., the subsequent uses of buf + l at call sites in this
function). Update the function that allocates buf to perform the NULL check,
perform any necessary cleanup (free other resources if allocated), and return an
appropriate error code or throw so callers (the prepared statement routine that
uses buf) do not continue and cause a crash.
|
Hi @proton-lisandro-pin . |
|
Closing this |
2 participants
Fixes digest computation for prepared statements by stripping query comments when either
mysql-query_digests_keep_comment(MySQL) orpgsql-query_digests_keep_comment(PostgreSQL) isfalse.This fixes an issue where the cache would blow up in size upon prepared statements with query annotations.
See #5384 for details.
Summary by CodeRabbit