Skip to content

bpo-43249: sqlite3_column_bytes() must follow sqlite_column_blob()#24562

Merged
berkerpeksag merged 2 commits intopython:masterfrom
erlend-aasland:bpo-43249
Feb 18, 2021
Merged

bpo-43249: sqlite3_column_bytes() must follow sqlite_column_blob()#24562
berkerpeksag merged 2 commits intopython:masterfrom
erlend-aasland:bpo-43249

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Feb 18, 2021 •
edited by bedevere-bot
Loading

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Feb 18, 2021 •
edited
Loading

@berkerpeksag Do we need a NEWS item for this one? I'm trying to formulate something, but I find it hard to describe. Maybe something like "Improve SQLite API usage"-ish. What do you think?

Also, if you permit, I'd like to add a small commit to this PR: Remove the broadly scoped val_str in favour of narrow scoped variables for the two other use cases.

diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c
index 486d25bbec..55491cf2f0 100644
--- a/Modules/_sqlite/cursor.c
+++ b/Modules/_sqlite/cursor.c
@@ -249,7 +249,6 @@ _pysqlite_fetch_one_row(pysqlite_Cursor* self)
     PyObject* converter;
     PyObject* converted;
     Py_ssize_t nbytes;
-    const char* val_str;
     char buf[200];
     const char* colname;
     PyObject* error_msg;
@@ -284,12 +283,12 @@ _pysqlite_fetch_one_row(pysqlite_Cursor* self)
          * Ref. https://sqlite.org/c3ref/column_blob.html
          */
         if (converter != Py_None) {
-            val_str = (const char*)sqlite3_column_blob(self->statement->st, i);
+            const char *blob = (const char*)sqlite3_column_blob(self->statement->st, i);
             nbytes = sqlite3_column_bytes(self->statement->st, i);
-            if (!val_str) {
+            if (!blob) {
                 converted = Py_NewRef(Py_None);
             } else {
-                item = PyBytes_FromStringAndSize(val_str, nbytes);
+                item = PyBytes_FromStringAndSize(blob, nbytes);
                 if (!item)
                     goto error;
                 converted = PyObject_CallOneArg(converter, item);
@@ -306,10 +305,10 @@ _pysqlite_fetch_one_row(pysqlite_Cursor* self)
             } else if (coltype == SQLITE_FLOAT) {
                 converted = PyFloat_FromDouble(sqlite3_column_double(self->statement->st, i));
             } else if (coltype == SQLITE_TEXT) {
-                val_str = (const char*)sqlite3_column_text(self->statement->st, i);
+                const char *text = (const char*)sqlite3_column_text(self->statement->st, i);
                 nbytes = sqlite3_column_bytes(self->statement->st, i);
                 if (self->connection->text_factory == (PyObject*)&PyUnicode_Type) {
-                    converted = PyUnicode_FromStringAndSize(val_str, nbytes);
+                    converted = PyUnicode_FromStringAndSize(text, nbytes);
                     if (!converted && PyErr_ExceptionMatches(PyExc_UnicodeDecodeError)) {
                         PyErr_Clear();
                         colname = sqlite3_column_name(self->statement->st, i);
@@ -317,7 +316,7 @@ _pysqlite_fetch_one_row(pysqlite_Cursor* self)
                             colname = "<unknown column name>";
                         }
                         PyOS_snprintf(buf, sizeof(buf) - 1, "Could not decode to UTF-8 column '%s' with text '%s'",
-                                     colname , val_str);
+                                     colname , text);
                         error_msg = PyUnicode_Decode(buf, strlen(buf), "ascii", "replace");
                         if (!error_msg) {
                             PyErr_SetString(pysqlite_OperationalError, "Could not decode to UTF-8");
@@ -327,11 +326,11 @@ _pysqlite_fetch_one_row(pysqlite_Cursor* self)
                         }
                     }
                 } else if (self->connection->text_factory == (PyObject*)&PyBytes_Type) {
-                    converted = PyBytes_FromStringAndSize(val_str, nbytes);
+                    converted = PyBytes_FromStringAndSize(text, nbytes);
                 } else if (self->connection->text_factory == (PyObject*)&PyByteArray_Type) {
-                    converted = PyByteArray_FromStringAndSize(val_str, nbytes);
+                    converted = PyByteArray_FromStringAndSize(text, nbytes);
                 } else {
-                    converted = PyObject_CallFunction(self->connection->text_factory, "y#", val_str, nbytes);
+                    converted = PyObject_CallFunction(self->connection->text_factory, "y#", text, nbytes);
                 }
             } else {
                 /* coltype == SQLITE_BLOB */

@berkerpeksag
Copy link
Member

berkerpeksag commented Feb 18, 2021

@berkerpeksag Do we need a NEWS item for this one? I'm trying to formulate something, but I find it hard to describe. Maybe something like "Improve SQLite API usage"-ish. What do you think?

Nope, unless we have a test case that can be failed without this fix, I'd say it's an implementation detail and we don't need to document it.

Also, if you permit, I'd like to add a small commit to this PR [...]

It would be great if you could submit it as separate PR (no need to create a separate issue; you can just link that to this PR or we can treat it as a follow-up and use the same BPO issue) I guess the reason for using the same variable was restrictions imposed by C89 and/or old MSVC compiler.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Feb 18, 2021 •
edited
Loading

Nope, unless we have a test case that can be failed without this fix, I'd say it's an implementation detail and we don't need to document it.

Not testable, AFAIK => no NEWS it is.

It would be great if you could submit it as separate PR (no need to create a separate issue; you can just link that to this PR or we can treat it as a follow-up and use the same BPO issue) I guess the reason for using the same variable was restrictions imposed by C89 and/or old MSVC compiler.

Sure, I'll do that when this PR is merged. Yes, this code predates the "C99-upgrade" of PEP 7 :)

berkerpeksag
berkerpeksag requested changes Feb 18, 2021
View reviewed changes
Copy link
Member

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

Minor comment on the comment, but other than that LGTM!

@bedevere-bot
Copy link

bedevere-bot commented Feb 18, 2021

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Feb 18, 2021

I have made the requested changes; please review again

@bedevere-bot
Copy link

bedevere-bot commented Feb 18, 2021

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.

@berkerpeksag berkerpeksag merged commit 47feb1f into python:master Feb 18, 2021
@berkerpeksag
Copy link
Member

berkerpeksag commented Feb 18, 2021

Thank you!

@erlend-aasland erlend-aasland deleted the bpo-43249 branch February 18, 2021 15:57
@erlend-aasland erlend-aasland mentioned this pull request Feb 18, 2021
Merged
@erlend-aasland erlend-aasland mentioned this pull request Mar 2, 2021
Merged
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@adorilson
bpo-43249: sqlite3_column_bytes() must follow sqlite_column_blob() (p…
4417d7c 
…ythonGH-24562)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag berkerpeksag approved these changes

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@erlend-aasland @berkerpeksag @bedevere-bot @the-knights-who-say-ni