FIX: Validate driver-provided fetch sizes - #803
gargsaumya wants to merge 8 commits into
Conversation
…nto saumya/native-fetch-validation
…nto saumya/native-fetch-validation # Conflicts: # mssql_python/pybind/ddbc_bindings.cpp
…nto saumya/native-fetch-validation # Conflicts: # mssql_python/pybind/ddbc_bindings.cpp
| SQLSetStmtAttr_ptr(hStmt, SQL_ATTR_ROW_ARRAY_SIZE, (SQLPOINTER)(intptr_t)fetchSize, 0); | ||
| SQLSetStmtAttr_ptr(hStmt, SQL_ATTR_ROWS_FETCHED_PTR, &numRowsFetched, 0); | ||
| SQLULEN numRowsFetched = 0; | ||
| FetchStateGuard fetchStateGuard(hStmt, &numRowsFetched, fetchSize); |
There was a problem hiding this comment.
On the non-LOB fetchall() error path, this guard runs statement cleanup before Python calls check_error(). Those ODBC calls replace the failed fetch’s diagnostics, losing its SQLSTATE and driver message. A specific DataError or OperationalError can consequently become DatabaseError: Unknown DDBC error.
Please capture the diagnostics before cleanup and preserve the existing DB-API exception mapping, while retaining cleanup before the buffers are destroyed.
Gaurav Sharma (bewithgaurav)
left a comment
Gaurav Sharma (bewithgaurav)
left a comment
There was a problem hiding this comment.
the driver-provided size validation is well scoped and the normal fetch and arrow paths hold up. I found one suggestion worth incorporating before merge, so requesting changes.
| ret); | ||
| return ret; | ||
| } | ||
| for (SQLUSMALLINT col = 0; col < numCols; ++col) { |
There was a problem hiding this comment.
suggestion: these checks are the only safety net when a driver returns malformed row counts, indicators, or buffer lengths, but this diff adds no tests and this stacked branch has no coverage run. the normal fetch and arrow tests pass without exercising any rejection path.
can we add a fake ODBC callback test seam for oversized row counts, odd SQLWCHAR lengths, out-of-capacity indicators, and SQL_NO_TOTAL with no progress?
| return left * right; | ||
| } | ||
|
|
||
| size_t CheckedFetchAdd(size_t left, size_t right, const char* errorMessage) { |
There was a problem hiding this comment.
optional: CheckedFetchAdd and CheckedFetchMultiply only forward to the existing checked-size helpers without adding fetch-specific behavior. using CheckedAddSize and CheckedMultiplySize directly keeps one vocabulary for the same overflow contract.
3 participants
Work Item / Issue Reference
Summary
Stacked on #802 so this review contains only driver-provided fetch validation.