Home Original page

gh-79579: Improve DML query detection in sqlite3 by erlend-aasland · Pull Request #93623 · python/cpython

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

Open

erlend-aasland wants to merge 14 commits into python:main

base: main
Choose a base branch

from erlend-aasland:sqlite-rowcount/gh-79579-alt

Conversation

erlend-aasland

Strip whitespace and comments from queries in order to harden DML query
detection.

Resolves #79579

@erlend-aasland

@erlend-aasland

If the sqlite3_normalized_sql API becomes enabled by default in the future, we can get rid of our own parser helper, and just use sqlite3_normalized_sql to strip whitespace and comments. We'll see what the future brings.

erlend-aasland

erlend-aasland

@bedevere-bot

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 5918fbe 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

erlend-aasland

animalize

Copy link

Contributor

@animalize animalize left a comment

Please merge after a few days, maybe there is something not currently thought of.

- normalise switch cases
- improve NEWS entry accuracy

erlend-aasland

}

const char *p = lstrip_sql(sql_cstr);
if (p != NULL) {
is_dml = (PyOS_strnicmp(p, "insert", 6) == 0)
|| (PyOS_strnicmp(p, "update", 6) == 0)
|| (PyOS_strnicmp(p, "delete", 6) == 0)
|| (PyOS_strnicmp(p, "replace", 7) == 0);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this remark in the sqlite3_changes docs:

auxiliary changes caused by triggers, foreign key actions or REPLACE constraint resolution are not counted.

This is a separate issue, out of scope for this PR.

@erlend-aasland

Thanks for reviewing, Ma Lin. Highly appreciated 🙏🏻

The buildbot run for 5918fbe completed without failures.

I'll let this PR sit around for some days to give Serhiy a chance to review. I'll merge sometime next week.

@animalize

As I said before, I'm not a deep user of SQL. So when in very complex situations, there may be things that I can't think of.
But I will try my best to learn and understand.

erlend-aasland
parse_remaining_sql_state state = NORMAL;

for (;;) {
switch (*pos) {
case 0:
return 0;
return NULL;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: this case makes the return NULL at the end of the function unreachable. This can be easily fixed with a tiny refactor, where case 0 is removed:

lstrip_sql(const char *sql)
{
    parse_remaining_sql_state state = NORMAL;

    for (const char *pos = sql; *pos; pos++) {
        ...
    }

    return NULL;

I'll add that in a separate PR.

@animalize