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

[erlend-aasland]

https://bugs.python.org/issue43249

[erlend-aasland]

@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]

@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]

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]

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

[bedevere-bot]

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]

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

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

[berkerpeksag]

Thank you!