Repository navigation
FIX: Validate caller-controlled native buffer sizes - #802
Open
gargsaumya wants to merge 47 commits into
Open
gargsaumya wants to merge 47 commits into
gargsaumya wants to merge 47 commits into
Conversation
PR Performance Report
|
| Database task | Unix / SQL Server 2022 | Unix / SQL Server 2025 |
|---|---|---|
| Insertion with explicit input sizes | 24.8% slower | 21.6% slower |
The largest recorded phase increases for these tasks are shown below. Phase timings are supporting evidence, not root-cause proof.
Coverage: 2 of 2 environments completed. Advisory result; does not block merging.
Measured timings
| Environment | Database task | Before | After | Change |
|---|---|---|---|---|
| Unix / SQL Server 2022 | Insertion with explicit input sizes | 485.150 ms | 604.766 ms | +24.8% |
| Unix / SQL Server 2025 | Insertion with explicit input sizes | 473.629 ms | 582.821 ms | +21.6% |
Performance diagnostics
Phase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed.
Unix / SQL Server 2022
Insertion with explicit input sizes: py::execute::cpp_call +23.758 ms; ddbc::SQLExecute_wrap +23.641 ms; ddbc::BindParameters +0.881 ms.
Unix / SQL Server 2025
Insertion with explicit input sizes: py::execute::cpp_call +13.819 ms; ddbc::SQLExecute_wrap +13.705 ms; py::execute::post_execute +0.017 ms.
All database tasks and timings
Unix / SQL Server 2022
| Database task | Before | After | Paired change | Result |
|---|---|---|---|---|
| Connection opening | 11.240 ms | 10.728 ms | -0.6% | no signal |
| SELECT queries | 1.077 ms | 1.113 ms | +1.7% | no signal |
| Row insertion | 32.691 ms | 32.483 ms | -0.5% | no signal |
| Executemany inserts | 161.633 ms | 181.948 ms | +13.6% | no signal |
| Fetch-all queries | 121.362 ms | 122.258 ms | +0.9% | no signal |
| Row-by-row fetching | 14.491 ms | 14.432 ms | +1.4% | no signal |
| Batched row fetching | 117.548 ms | 118.625 ms | +0.7% | no signal |
| Transaction commit and rollback | 107.107 ms | 107.207 ms | +0.4% | no signal |
| Arrow row fetching | 90.649 ms | 96.398 ms | +6.8% | no signal |
| 100,000-row insertion | 445.303 ms | 439.361 ms | -2.0% | no signal |
| Row fetching in batches of 100 | 122.741 ms | 124.538 ms | +1.6% | no signal |
| Row fetching in batches of 10,000 | 134.755 ms | 129.586 ms | -0.7% | no signal |
| Repeated positional queries | 32.224 ms | 32.453 ms | +1.9% | no signal |
| Repeated named-parameter queries | 34.505 ms | 34.508 ms | +0.0% | no signal |
| Legacy 100,000-row insertion | 353.848 ms | 355.241 ms | +0.3% | no signal |
| Insertion with explicit input sizes | 485.150 ms | 604.766 ms | +24.8% | consistent slowdown |
| Joined aggregation queries | 175.974 ms | 176.483 ms | +0.1% | no signal |
| Large joined-result fetching | 193.007 ms | 194.740 ms | -0.1% | no signal |
| 1.2-million-row fetching | 3458.852 ms | 3465.294 ms | +0.6% | no signal |
| Common table expression queries | 5.418 ms | 5.410 ms | +1.4% | no signal |
| 256 KiB VARCHAR(MAX) / fetchall() | 1.707 ms | 1.465 ms | -14.2% | no signal |
| 10,000 scalar values / fetchval() (debug disabled) | 107.410 ms | 107.947 ms | +1.6% | no signal |
Unix / SQL Server 2025
| Database task | Before | After | Paired change | Result |
|---|---|---|---|---|
| Connection opening | 99.397 ms | 98.681 ms | -2.0% | no signal |
| SELECT queries | 1.090 ms | 1.064 ms | -3.7% | no signal |
| Row insertion | 34.031 ms | 34.256 ms | +1.2% | no signal |
| Executemany inserts | 152.750 ms | 171.743 ms | +12.8% | no signal |
| Fetch-all queries | 120.472 ms | 121.242 ms | +0.6% | no signal |
| Row-by-row fetching | 14.647 ms | 14.597 ms | -1.4% | no signal |
| Batched row fetching | 117.468 ms | 118.151 ms | +0.7% | no signal |
| Transaction commit and rollback | 114.561 ms | 114.374 ms | +0.0% | no signal |
| Arrow row fetching | 93.120 ms | 98.232 ms | +6.0% | no signal |
| 100,000-row insertion | 436.726 ms | 432.111 ms | -0.5% | no signal |
| Row fetching in batches of 100 | 121.518 ms | 123.748 ms | +2.5% | no signal |
| Row fetching in batches of 10,000 | 138.252 ms | 127.388 ms | -6.4% | no signal |
| Repeated positional queries | 33.559 ms | 33.420 ms | +0.3% | no signal |
| Repeated named-parameter queries | 35.887 ms | 36.118 ms | +0.9% | no signal |
| Legacy 100,000-row insertion | 344.183 ms | 347.024 ms | +0.8% | no signal |
| Insertion with explicit input sizes | 473.629 ms | 582.821 ms | +21.6% | consistent slowdown |
| Joined aggregation queries | 161.496 ms | 160.842 ms | -0.1% | no signal |
| Large joined-result fetching | 181.620 ms | 181.513 ms | +0.2% | no signal |
| 1.2-million-row fetching | 3392.021 ms | 3423.966 ms | +0.9% | no signal |
| Common table expression queries | 5.115 ms | 5.210 ms | +2.1% | no signal |
| 256 KiB VARCHAR(MAX) / fetchall() | 1.446 ms | 1.382 ms | -4.3% | no signal |
| 10,000 scalar values / fetchval() (debug disabled) | 106.023 ms | 107.799 ms | +1.4% | no signal |
Build and measurement details
PR head: 44017f9e31d9288ee1dde2c318d8d2a0ea0fc811
Base: 9424705251baf8ded41a8a4968216b1fd9b00929
Measured merge: 6f205f329e185764f89466feeeeb3df61a0689d5
- Unix / SQL Server 2022: Python 3.12.3, x86_64, SQL 16.0.4305.5; 5 paired comparisons and 1 warmup.
- Unix / SQL Server 2025: Python 3.12.3, x86_64, SQL 17.0.5015.4; 5 paired comparisons and 1 warmup.
A consistent change requires more than 20% median paired movement, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold in the same direction. A slowdown without enough pair agreement is reported as inconsistent.
The displayed change is the median of paired before-and-after ratios. It is not recalculated from the two displayed median runtimes.
Both revisions use profiling-enabled builds on the same agent and database, with alternating order and discarded warmups. Results are diagnostic and do not represent production-wheel latency.
Raw samples and logs are attached to the ADO run as profiler-* artifacts.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved row-count and binary validation gaps remain, and an existing Arrow batch-size test requires reconciliation.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Hardens Python-to-native boundary validation for row counts, allocations, metadata, and binary inputs.
Changes:
- Adds row-count, boolean, and input-size validation.
- Adds native allocation overflow and metadata checks.
- Tightens binary and wide-character handling with regression tests.
| File | Description |
|---|---|
tests/test_024_bulkcopy_arrow.py |
Tests boolean batch-size rejection. |
tests/test_004_cursor.py |
Tests cursor size and metadata validation. |
mssql_python/pybind/ddbc_bindings.cpp |
Adds native allocation and parameter validation. |
mssql_python/cursor.py |
Validates row counts and input sizes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Native paths still permit dangerous negative or extremely large allocations, and arrow_reader() does not reject invalid sizes synchronously.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 4
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/cursor.pyLines 58-78 58 MAX_NATIVE_PARAMETER_SIZE: int = 256 * 1024 * 1024
59
60
61 def _encoded_length_exceeds(value: str, encoding: str, limit: int) -> bool:
! 62 encoder = codecs.getincrementalencoder(encoding)(errors="strict")
! 63 total = 0
! 64 chunk_size = 4096
! 65 value_length = str.__len__(value)
! 66 for offset in range(0, value_length, chunk_size):
! 67 end = min(offset + chunk_size, value_length)
! 68 chunk = str.__getitem__(value, slice(offset, end))
! 69 total += len(encoder.encode(chunk, final=end == value_length))
! 70 if total > limit:
! 71 return True
! 72 if value_length == 0:
! 73 total += len(encoder.encode("", final=True))
! 74 return total > limit
75
76
77 def _utf16_length_exceeds(value: str, limit: int) -> bool:
78 bounded_value = str.__getitem__(value, slice(0, limit + 1))Lines 2673-2681 2673 binary_values = [
2674 value for value in column if isinstance(value, (bytes, bytearray))
2675 ]
2676 if c_type == ddbc_sql_const.SQL_CHAR.value:
! 2677 text_is_large = any(
2678 _encoded_length_exceeds(
2679 value,
2680 encoding_settings["encoding"],
2681 MAX_INLINE_CHAR,Lines 2693-2701 2693 requires_row_fallback = bool(
2694 narrow_text_type and text_values and binary_values
2695 )
2696 if narrow_text_type and binary_values and not text_values:
! 2697 c_type = ddbc_sql_const.SQL_CHAR.value
2698 is_dae = text_is_large or binary_is_large
2699
2700 # Sanitize precision/scale for numeric types
2701 if sql_type in (Lines 2721-2729 2721
2722 # For SQL Server VARBINARY(MAX), we need to use large object binding
2723 if max_binary_size > MAX_INLINE_BINARY:
2724 if sql_type != ddbc_sql_const.SQL_SS_UDT.value:
! 2725 sql_type = ddbc_sql_const.SQL_LONGVARBINARY.value
2726 is_dae = True
2727
2728 # Update column_size to actual maximum size if it's larger
2729 # Always ensure at least a minimum size of 1 for empty stringsmssql_python/pybind/ddbc_bindings.cppLines 325-334 325
326 template <typename ParamType>
327 ParamType* AllocateParamBufferArray(std::vector<std::shared_ptr<void>>& paramBuffers,
328 size_t count) {
! 329 if (count > std::numeric_limits<size_t>::max() / sizeof(ParamType)) {
! 330 ThrowStdException("Parameter buffer size is too large");
331 }
332 std::shared_ptr<ParamType> buffer(new ParamType[count], std::default_delete<ParamType[]>());
333 ParamType* raw = buffer.get();
334 paramBuffers.push_back(buffer);Lines 335-344 335 return raw;
336 }
337
338 size_t CheckedAddSize(size_t left, size_t right, const char* errorMessage) {
! 339 if (left > std::numeric_limits<size_t>::max() - right) {
! 340 ThrowStdException(errorMessage);
341 }
342 return left + right;
343 }Lines 342-351 342 return left + right;
343 }
344
345 size_t CheckedMultiplySize(size_t left, size_t right, const char* errorMessage) {
! 346 if (left != 0 && right > std::numeric_limits<size_t>::max() / left) {
! 347 ThrowStdException(errorMessage);
348 }
349 return left * right;
350 }Lines 350-359 350 }
351
352 size_t CheckedFetchColumnSize(SQLULEN columnSize) {
353 const size_t result = static_cast<size_t>(columnSize);
! 354 if (static_cast<SQLULEN>(result) != columnSize) {
! 355 ThrowStdException("Column size is too large");
356 }
357 return result;
358 }Lines 359-368 359
360 SQLLEN CheckedFetchBufferLength(size_t elementCount, size_t elementSize) {
361 const size_t byteCount =
362 CheckedMultiplySize(elementCount, elementSize, "Column fetch stride is too large");
! 363 if (byteCount > static_cast<size_t>(std::numeric_limits<SQLLEN>::max())) {
! 364 ThrowStdException("Column fetch stride is too large");
365 }
366 return static_cast<SQLLEN>(byteCount);
367 }Lines 421-431 421 case SQL_C_CHAR:
422 case SQL_C_BINARY:
423 return CheckedAddSize(info.columnSize, 1, "Character parameter size is too large");
424 case SQL_C_BIT:
! 425 return sizeof(char);
! 426 case SQL_C_STINYINT:
! 427 case SQL_C_USHORT:
428 return sizeof(unsigned short);
429 case SQL_C_SBIGINT:
430 case SQL_C_SLONG:
431 case SQL_C_UBIGINT:Lines 433-456 433 return sizeof(int64_t);
434 case SQL_C_FLOAT:
435 return sizeof(float);
436 case SQL_C_TYPE_DATE:
! 437 return sizeof(SQL_DATE_STRUCT);
! 438 case SQL_C_TYPE_TIME:
439 return sizeof(SQL_TIME_STRUCT);
440 case SQL_C_TYPE_TIMESTAMP:
441 return sizeof(SQL_TIMESTAMP_STRUCT);
442 case SQL_C_SS_TIMESTAMPOFFSET:
! 443 return sizeof(DateTimeOffset);
! 444 case SQL_C_NUMERIC:
445 return sizeof(SQL_NUMERIC_STRUCT);
446 case SQL_C_GUID:
447 return sizeof(SQLGUID);
448 case SQL_C_DEFAULT:
! 449 return sizeof(char);
! 450 default:
451 ThrowStdException("Unsupported C type for parameter array allocation");
! 452 }
453 return 0;
454 }
455
456 template <typename ElementType>Lines 460-469 460 CheckedMultiplySize(rowIndex, rowStride, "Arrow source offset is too large");
461 const size_t rowCapacity = CheckedMultiplySize(
462 rowStride, sizeof(ElementType), "Arrow source capacity is too large");
463 if (offset > buffer.size() || rowStride > buffer.size() - offset ||
! 464 dataBytes > rowCapacity) {
! 465 ThrowStdException("Driver data length exceeds the allocated fetch buffer");
466 }
467 return offset;
468 }Lines 468-477 468 }
469
470 template <typename ElementType>
471 std::unique_ptr<ElementType[]> AllocateUniqueArray(size_t count, const char* errorMessage) {
! 472 if (count > std::numeric_limits<size_t>::max() / sizeof(ElementType)) {
! 473 ThrowStdException(errorMessage);
474 }
475 return std::make_unique<ElementType[]>(count);
476 }Lines 560-568 560 const char* errorMessage) {
561 ReserveNativeFetchBytes(reservedBytes, count, sizeof(ElementType));
562 return AllocateUniqueArray<ElementType>(count, errorMessage);
563 }
! 564
565 std::string DescribeChar(unsigned char ch) {
566 if (ch >= 32 && ch <= 126) {
567 return std::string("'") + static_cast<char>(ch) + "'";
568 } else {Lines 603-612 603 if (!chunk) throw py::error_already_set();
604 py::object encoded = encoder.attr("encode")(chunk, end == length);
605 char* data = nullptr;
606 Py_ssize_t size = 0;
! 607 if (PyBytes_AsStringAndSize(encoded.ptr(), &data, &size) != 0) {
! 608 throw py::error_already_set();
609 }
610 if (size > 0) {
611 SQLRETURN rc = put_data_fn(data, static_cast<SQLLEN>(size));
612 if (!SQL_SUCCEEDED(rc)) return rc;Lines 611-627 611 SQLRETURN rc = put_data_fn(data, static_cast<SQLLEN>(size));
612 if (!SQL_SUCCEEDED(rc)) return rc;
613 }
614 }
! 615 if (length == 0) {
! 616 py::object encoded = encoder.attr("encode")(py::str(), true);
! 617 char* data = nullptr;
! 618 Py_ssize_t size = 0;
! 619 if (PyBytes_AsStringAndSize(encoded.ptr(), &data, &size) != 0) {
! 620 throw py::error_already_set();
! 621 }
! 622 if (size > 0) return put_data_fn(data, static_cast<SQLLEN>(size));
! 623 return put_data_fn(nullptr, 0);
624 }
625 return SQL_SUCCESS;
626 }Lines 633-643 633
634 static SQLRETURN StreamDAEParameter(SQLHSTMT hStmt, const ParamInfo& info,
635 const std::string& charEncoding) {
636 PyObject* value = info.dataPtr.ptr();
! 637 if (!value || value == Py_None) {
! 638 py::gil_scoped_release release;
! 639 return SQLPutData_ptr(hStmt, nullptr, 0);
640 }
641
642 auto putImmutableData = [&](SQLPOINTER data, SQLLEN length) {
643 py::gil_scoped_release release;Lines 645-657 645 };
646 if (PyUnicode_Check(value)) {
647 if (info.paramCType == SQL_C_WCHAR) {
648 return stream_unicode_dae_chunks(value, "utf-16-le", putImmutableData);
! 649 }
! 650 if (info.paramCType == SQL_C_CHAR) {
! 651 return stream_unicode_dae_chunks(value, charEncoding, putImmutableData);
! 652 }
! 653 ThrowStdException("DAE only supports text C types for str values");
654 }
655 if (PyBytes_Check(value)) {
656 return stream_dae_chunks(PyBytes_AS_STRING(value),
657 static_cast<size_t>(PyBytes_GET_SIZE(value)),Lines 663-672 663 std::vector<char> chunk(std::min(static_cast<size_t>(DAE_CHUNK_SIZE), totalBytes));
664 for (size_t offset = 0; offset < totalBytes; offset += chunk.size()) {
665 const size_t currentSize = static_cast<size_t>(PyByteArray_GET_SIZE(value));
666 const size_t length = std::min(chunk.size(), totalBytes - offset);
! 667 if (currentSize < offset + length) {
! 668 ThrowStdException("bytearray changed size during DAE streaming");
669 }
670 std::copy_n(PyByteArray_AS_STRING(value) + offset, length, chunk.data());
671 SQLRETURN rc = putImmutableData(chunk.data(), static_cast<SQLLEN>(length));
672 if (!SQL_SUCCEEDED(rc)) return rc;Lines 671-680 671 SQLRETURN rc = putImmutableData(chunk.data(), static_cast<SQLLEN>(length));
672 if (!SQL_SUCCEEDED(rc)) return rc;
673 }
674 return SQL_SUCCESS;
! 675 }
! 676 ThrowStdException("DAE only supports str, bytes, or bytearray values");
677 return SQL_ERROR;
678 }
679
680 // GH-610: Resolve SQL type for a NULL parameter using per-handle cache.Lines 868-876 868 if (py::isinstance<py::str>(param)) {
869 // Encode Unicode string using the specified encoding
870 try {
871 py::object encoded =
! 872 EncodeUnicodeValue(param.ptr(), charEncoding);
873 encodedStr = encoded.cast<std::string>();
874 LOG("BindParameters: param[%d] SQL_C_CHAR - Encoded with '%s', "
875 "size=%zu bytes",
876 paramIndex, charEncoding.c_str(), encodedStr.size());Lines 1968-1976 1968 }
1969
1970 SQLRETURN SqlHandle::freeHandle() {
1971 PERF_TIMER("SqlHandle::free");
! 1972 clearResult();
1973 bool pythonShuttingDown = is_python_finalizing();
1974 bool skipDuringShutdown = _type == SQL_HANDLE_STMT || _type == SQL_HANDLE_DBC;
1975 #ifdef _WIN32
1976 // The static ENV is destroyed during DLL_PROCESS_DETACH, after PythonLines 2574-2585 2574 SQLRETURN describeRc = PreResolveUdtTypes(hStmt, paramInfos);
2575 if (!SQL_SUCCEEDED(describeRc)) return describeRc;
2576 // GH-627: resolve unknown NULL array param SQL types before binding any param.
2577 PreResolveUnknownNullTypes(handle, hStmt, paramInfos);
! 2578 size_t reservedParameterBytes = 0;
! 2579 for (const ParamInfo& info : paramInfos) {
! 2580 ReserveNativeParameterBytes(reservedParameterBytes, paramSetSize,
! 2581 ParameterArrayElementSize(info));
2582 ReserveNativeParameterBytes(reservedParameterBytes, paramSetSize, sizeof(SQLLEN));
2583 }
2584 for (int paramIndex = 0; paramIndex < columnwise_params.size(); ++paramIndex) {
2585 const py::list& columnValues = columnwise_params[paramIndex].cast<py::list>();Lines 2648-2659 2648 elementWidth, sizeof(SQLWCHAR),
2649 "Wide-character parameter length is too large");
2650 if (bufferBytes > static_cast<size_t>(std::numeric_limits<SQLLEN>::max())) {
2651 ThrowStdException("Wide-character parameter length is too large");
! 2652 }
2653 SQLWCHAR* wcharArray = AllocateParamBufferArray<SQLWCHAR>(
! 2654 tempBuffers,
! 2655 CheckedMultiplySize(paramSetSize, elementWidth,
2656 "Wide-character parameter buffer is too large"));
2657 strLenOrIndArray = AllocateParamBufferArray<SQLLEN>(tempBuffers, paramSetSize);
2658 for (size_t i = 0; i < paramSetSize; ++i) {
2659 if (columnValues[i].is_none()) {Lines 2702-2710 2702 LOG("BindParameterArray: SQL_C_WCHAR bound - "
2703 "param_index=%d",
2704 paramIndex);
2705 dataPtr = wcharArray;
! 2706 bufferLength = static_cast<SQLLEN>(bufferBytes);
2707 break;
2708 }
2709 case SQL_C_TINYINT:
2710 case SQL_C_UTINYINT: {Lines 2773-2785 2773 case SQL_C_BINARY: {
2774 LOG("BindParameterArray: Binding SQL_C_CHAR/BINARY array - "
2775 "param_index=%d, count=%zu, column_size=%zu, encoding='%s'",
2776 paramIndex, paramSetSize, info.columnSize, charEncoding.c_str());
! 2777 const size_t elementWidth = CheckedAddSize(
! 2778 info.columnSize, 1, "Character parameter size is too large");
! 2779 if (elementWidth > static_cast<size_t>(std::numeric_limits<SQLLEN>::max())) {
! 2780 ThrowStdException("Character parameter length is too large");
! 2781 }
2782 char* charArray = AllocateParamBufferArray<char>(
2783 tempBuffers,
2784 CheckedMultiplySize(paramSetSize, elementWidth,
2785 "Character parameter buffer is too large"));Lines 2789-2797 2789 strLenOrIndArray[i] = SQL_NULL_DATA;
2790 std::memset(charArray + i * (info.columnSize + 1), 0,
2791 info.columnSize + 1);
2792 } else {
! 2793 if (info.paramCType == SQL_C_BINARY &&
2794 !py::isinstance<py::bytes>(columnValues[i]) &&
2795 !py::isinstance<py::bytearray>(columnValues[i])) {
2796 ThrowStdException(MakeParamMismatchErrorStr(info.paramCType,
2797 paramIndex));Lines 2806-2815 2806 encoded =
2807 EncodeUnicodeValue(columnValues[i].ptr(), charEncoding);
2808 if (PyBytes_AsStringAndSize(encoded.ptr(), &encodedData,
2809 &encodedSize) != 0) {
! 2810 throw py::error_already_set();
! 2811 }
2812 LOG("BindParameterArray: param[%d] row[%zu] SQL_C_CHAR - "
2813 "Encoded with '%s', "
2814 "size=%zu bytes",
2815 paramIndex, i, charEncoding.c_str(),Lines 2840-2858 2840 ThrowStdException(
2841 MakeParamMismatchErrorStr(info.paramCType, paramIndex));
2842 }
2843
! 2844 const size_t dataSize = static_cast<size_t>(encodedSize);
! 2845 if (dataSize > info.columnSize) {
2846 LOG("BindParameterArray: String/binary too "
2847 "long - param_index=%d, row=%zu, size=%zu, "
2848 "max=%zu",
! 2849 paramIndex, i, dataSize, info.columnSize);
2850 ThrowStdException("Input exceeds column size at index " +
2851 std::to_string(i));
2852 }
! 2853 std::copy_n(encodedData, dataSize, charArray + i * elementWidth);
! 2854 strLenOrIndArray[i] = static_cast<SQLLEN>(dataSize);
2855 }
2856 }
2857 LOG("BindParameterArray: SQL_C_CHAR/BINARY bound - "
2858 "param_index=%d",Lines 2857-2865 2857 LOG("BindParameterArray: SQL_C_CHAR/BINARY bound - "
2858 "param_index=%d",
2859 paramIndex);
2860 dataPtr = charArray;
! 2861 bufferLength = static_cast<SQLLEN>(elementWidth);
2862 break;
2863 }
2864 case SQL_C_BIT: {
2865 LOG("BindParameterArray: Binding SQL_C_BIT array - "Lines 3366-3375 3366 std::vector<ParamInfo> rowParamInfos = paramInfos;
3367 for (size_t paramIndex = 0; paramIndex < rowParamInfos.size(); ++paramIndex) {
3368 if (rowParams[paramIndex].is_none()) {
3369 rowParamInfos[paramIndex].paramCType = SQL_C_DEFAULT;
! 3370 rowParamInfos[paramIndex].isDAE = false;
! 3371 rowParamInfos[paramIndex].dataPtr = py::none();
3372 }
3373 }
3374 std::vector<std::shared_ptr<void>> paramBuffers;
3375 rc = BindParameters(*statementHandle, hStmt, rowParams, rowParamInfos,Lines 3404-3414 3404 return rc;
3405 }
3406 if (rc != SQL_NEED_DATA) break;
3407
! 3408 const ParamInfo* matchedInfo = reinterpret_cast<const ParamInfo*>(token);
! 3409 const ParamInfo* first = rowParamInfos.data();
! 3410 const ParamInfo* last = first + rowParamInfos.size();
3411 if (matchedInfo < first || matchedInfo >= last) {
3412 ThrowStdException(
3413 "SQLExecuteMany: unrecognized paramToken from SQLParamData");
3414 }Lines 3585-3597 3585 table.empty() ? 0 : SQL_NTS, scope, nullable);
3586 }
3587
3588 std::shared_ptr<PendingArrowRows> PendingRowsForNextFetch(SqlHandlePtr handle) {
! 3589 auto pending = handle->pendingArrowRows();
3590 if (pending) {
3591 pending->hasSelectedRow = false;
3592 if (pending->next == pending->count) {
! 3593 handle->setPendingArrowRows(nullptr);
3594 return nullptr;
3595 }
3596 }
3597 return pending;Lines 3595-3603 3595 }
3596 }
3597 return pending;
3598 }
! 3599
3600 SQLRETURN ReadSelectedArrowRow(SqlHandlePtr handle, PendingArrowRows& pending,
3601 SQLUSMALLINT columnCount, py::list& row,
3602 const std::string& charEncoding, py::handle messages);Lines 3630-3638 3630 bool isWideChar, bool isBinary, const std::string& charEncoding,
3631 py::handle messages) {
3632 PERF_TIMER("FetchLobColumnData");
3633 std::vector<char> buffer;
! 3634 size_t dataSize = 0;
3635 size_t reservedBytes = 0;
3636 ReserveNativeFetchBytes(reservedBytes, DAE_CHUNK_SIZE, sizeof(char));
3637 SQLRETURN ret = SQL_SUCCESS_WITH_INFO;
3638 int loopCount = 0;Lines 3703-3712 3703 }
3704 }
3705 if (bytesRead > 0) {
3706 const size_t previousSize = dataSize;
! 3707 dataSize = ExtendNativeFetchBuffer(buffer, dataSize, bytesRead, reservedBytes);
! 3708 std::copy_n(chunk.data(), bytesRead, buffer.data() + previousSize);
3709 LOG("FetchLobColumnData: Appended %zu bytes at loop %d", bytesRead, loopCount);
3710 }
3711 if (ret == SQL_SUCCESS) {
3712 LOG("FetchLobColumnData: SQL_SUCCESS - no more data at loop %d", loopCount);Lines 3721-3731 3721 }
3722 return py::str("");
3723 }
3724 if (isWideChar) {
! 3725 if (dataSize % sizeof(SQLWCHAR) != 0) {
! 3726 ThrowStdException("Wide-character LOB data has an invalid byte length");
! 3727 }
3728 int byteOrder = -1;
3729 PyObject* decoded =
3730 PyUnicode_DecodeUTF16(buffer.data(), static_cast<Py_ssize_t>(dataSize),
3731 "strict", &byteOrder);Lines 4716-4724 4716 const size_t fetchBufferSize = CheckedAddSize(
4717 baseColumnSize, 1, "Column fetch stride is too large");
4718 ResizeNativeFetchBuffer(buffers.wcharBuffers[col - 1],
4719 CheckedMultiplySize(fetchSize, fetchBufferSize,
! 4720 "Native fetch buffer is too large"),
4721 reservedBytes);
4722 ret = SQLBindCol_ptr(
4723 hStmt, col, SQL_C_WCHAR, buffers.wcharBuffers[col - 1].data(),
4724 CheckedFetchBufferLength(fetchBufferSize, sizeof(SQLWCHAR)),Lines 4773-4781 4773 buffers.smallIntBuffers[col - 1].data(), sizeof(SQLSMALLINT),
4774 buffers.indicators[col - 1].data());
4775 break;
4776 case SQL_TINYINT:
! 4777 ResizeNativeFetchBuffer(buffers.charBuffers[col - 1], fetchSize, reservedBytes);
4778 ret = SQLBindCol_ptr(hStmt, col, SQL_C_TINYINT, buffers.charBuffers[col - 1].data(),
4779 sizeof(SQLCHAR), buffers.indicators[col - 1].data());
4780 break;
4781 case SQL_BIT:Lines 4791-4801 4791 case SQL_DECIMAL:
4792 case SQL_NUMERIC:
4793 ResizeNativeFetchBuffer(
4794 buffers.charBuffers[col - 1],
! 4795 CheckedMultiplySize(fetchSize, MAX_DIGITS_IN_NUMERIC,
! 4796 "Native fetch buffer is too large"),
! 4797 reservedBytes);
4798 ret = SQLBindCol_ptr(hStmt, col, SQL_C_CHAR, buffers.charBuffers[col - 1].data(),
4799 MAX_DIGITS_IN_NUMERIC * sizeof(SQLCHAR),
4800 buffers.indicators[col - 1].data());
4801 break;Lines 4815-4823 4815 hStmt, col, SQL_C_TYPE_TIMESTAMP, buffers.timestampBuffers[col - 1].data(),
4816 sizeof(SQL_TIMESTAMP_STRUCT), buffers.indicators[col - 1].data());
4817 break;
4818 case SQL_BIGINT:
! 4819 ResizeNativeFetchBuffer(buffers.bigIntBuffers[col - 1], fetchSize, reservedBytes);
4820 ret =
4821 SQLBindCol_ptr(hStmt, col, SQL_C_SBIGINT, buffers.bigIntBuffers[col - 1].data(),
4822 sizeof(SQLBIGINT), buffers.indicators[col - 1].data());
4823 break;Lines 4847-4861 4847 HandleZeroColumnSizeAtFetch(columnSize);
4848 const size_t fetchBufferSize = CheckedFetchColumnSize(columnSize);
4849 ResizeNativeFetchBuffer(buffers.charBuffers[col - 1],
4850 CheckedMultiplySize(fetchSize, fetchBufferSize,
! 4851 "Native fetch buffer is too large"),
! 4852 reservedBytes);
4853 ret = SQLBindCol_ptr(hStmt, col, SQL_C_BINARY, buffers.charBuffers[col - 1].data(),
! 4854 CheckedFetchBufferLength(fetchBufferSize, 1),
4855 buffers.indicators[col - 1].data());
4856 break;
! 4857 }
4858 case SQL_SS_TIMESTAMPOFFSET:
4859 ResizeNativeFetchBuffer(buffers.datetimeoffsetBuffers[col - 1], fetchSize,
4860 reservedBytes);
4861 ret = SQLBindCol_ptr(hStmt, col, SQL_C_SS_TIMESTAMPOFFSET,Lines 4893-4902 4893 SQLRETURN ConvertBufferedRows(SQLHSTMT hStmt, ColumnBuffers& buffers, const Metadata& columnNames,
4894 py::list& rows, SQLUSMALLINT numCols, SQLULEN& numRowsFetched,
4895 const std::vector<SQLUSMALLINT>& lobColumns,
4896 const std::string& charEncoding = "utf-16le", int charCtype = SQL_C_WCHAR,
! 4897 py::handle messages = {}, SQLULEN firstRow = 0,
! 4898 int bufferRows = 0) {
4899 // Pre-cache column metadata to avoid repeated dictionary lookups.
4900 // The vectors below are consumed later by construct_rows, so they are
4901 // declared at function scope; only the population work is wrapped in the
4902 // cache_column_metadata timer's block (an earlier version put the timer atLines 5313-5321 5313 CheckedAddSize(columnSize, 1, "Native fetch row size is too large"),
5314 sizeof(SQLWCHAR), "Native fetch row size is too large");
5315 break;
5316 case SQL_INTEGER:
! 5317 columnBytes = sizeof(SQLINTEGER);
5318 break;
5319 case SQL_SMALLINT:
5320 columnBytes = sizeof(SQLSMALLINT);
5321 break;Lines 5742-5750 5742 case SQL_DECIMAL:
5743 case SQL_NUMERIC:
5744 return ReadArrowScalar(stmt, ordinal, SQL_C_CHAR, buffers.charBuffers[column],
5745 indicator, reservedBytes, messages, MAX_DIGITS_IN_NUMERIC);
! 5746 case SQL_DOUBLE:
5747 case SQL_FLOAT:
5748 return ReadArrowScalar(stmt, ordinal, SQL_C_DOUBLE, buffers.doubleBuffers[column],
5749 indicator, reservedBytes, messages);
5750 case SQL_TIMESTAMP:Lines 5780-5795 5780 py::handle messages) {
5781 if (pending.hasLobs) {
5782 size_t reservedBytes = pending.reservedBytes;
5783 for (; pending.lobColumn < columnCount; ++pending.lobColumn) {
! 5784 auto column = columns[pending.lobColumn].cast<py::dict>();
! 5785 const SQLRETURN ret = ReadArrowLobColumn(handle->get(), pending,
! 5786 column["DataType"].cast<SQLSMALLINT>(),
! 5787 pending.lobColumn, reservedBytes, messages);
! 5788 CheckFetchError(handle, ret);
! 5789 if (!SQL_SUCCEEDED(ret)) {
! 5790 return ret;
! 5791 }
5792 }
5793 }
5794 return SQL_SUCCESS;
5795 }Lines 5809-5818 5809 if (!SQL_SUCCEEDED(ret)) {
5810 return ret;
5811 }
5812 py::list rows;
! 5813 SQLULEN end = pending.selectedRow + 1;
! 5814 ret = ConvertBufferedRows(handle->get(), pending.buffers, columns, rows, columnCount, end,
5815 {}, charEncoding, pending.charCtype, messages,
5816 pending.selectedRow, pending.fetchSize);
5817 if (SQL_SUCCEEDED(ret)) {
5818 for (auto value : rows[0].cast<py::list>()) {Lines 5839-5855 5839 return ret;
5840 }
5841 SQLULEN end = pending->next + std::min(limit, pending->count - pending->next);
5842 ret = ConvertBufferedRows(handle->get(), pending->buffers, columns, rows, numCols, end,
! 5843 {}, charEncoding, pending->charCtype, messages,
! 5844 pending->next, pending->fetchSize);
! 5845 if (SQL_SUCCEEDED(ret)) {
5846 pending->next = end;
5847 if (pending->next == pending->count) {
5848 handle->setPendingArrowRows(nullptr);
5849 }
! 5850 }
! 5851 return ret;
5852 }
5853
5854 int32_t days_from_civil(int y, int m, int d) {
5855 // Implements the "days_from_civil" algorithm by Howard HinnantLines 5861-5870 5861 const unsigned doe = yoe * 365 + yoe / 4 - yoe / 100 + doy; // [0, 146096]
5862 return era * 146097 + static_cast<int>(doe) - 719468;
5863 }
5864
! 5865 SQLRETURN FetchArrowBatch_wrap(SqlHandlePtr StatementHandle, py::list& capsules,
! 5866 py::handle arrowBatchSizeArg, int charCtype,
5867 py::handle messages = {}) {
5868 PERF_TIMER("FetchArrowBatch_wrap");
5869 const int arrowBatchSize =
5870 ValidateNativeRowCountArgument(arrowBatchSizeArg, "Arrow batch size", true);Lines 5870-5879 5870 ValidateNativeRowCountArgument(arrowBatchSizeArg, "Arrow batch size", true);
5871 const size_t batchSize = static_cast<size_t>(arrowBatchSize);
5872 const size_t offsetCount = CheckedAddSize(batchSize, 1, "Arrow batch size is too large");
5873 const size_t initialVarDataSize =
! 5874 CheckedMultiplySize(batchSize, 42, "Arrow batch size is too large");
! 5875 const size_t bitmapSize =
5876 CheckedAddSize(batchSize, 7, "Arrow batch size is too large") / 8;
5877 // Fetch narrow char data as SQL_C_CHAR if on Linux/macOS and configured by the user
5878 charCtype = EffectiveCharCtypeForFetch(charCtype, "utf-8");
5879 if (arrowBatchSize > 0) {Lines 5885-5893 5885 }
5886
5887 // An overly large fetch size doesn't seem to help performance.
5888 int fetchSize = pending ? pending->fetchSize
! 5889 : arrowBatchSize > 0 ? std::min(arrowBatchSize, 64) : 1;
5890
5891 SQLRETURN ret = SQL_ERROR;
5892 ResultMetadataFailureGuard metadataFailure(StatementHandle->resultMetadata, ret);
5893 SQLHSTMT hStmt = StatementHandle->get();Lines 5970-5980 5970 case SQL_LONGVARBINARY:
5971 format = "Z";
5972 arrowColumnProducer->varVal =
5973 AllocateArrowArray<uint64_t>(offsetCount, reservedBytes,
! 5974 "Arrow offset buffer is too large");
! 5975 EnsureNativeFetchBufferSize(arrowColumnProducer->varData, initialVarDataSize,
! 5976 reservedBytes);
5977 columnVarLen[i] = true;
5978 // start at offset 0
5979 arrowColumnProducer->varVal[0] = 0;
5980 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->varVal.get();Lines 6129-6137 6129 SQLULEN& numRowsFetched = pending->count;
6130 FetchStateGuard fetchStateGuard(StatementHandle, messages);
6131
6132 if (!hasLobColumns && bufferFetchSize > 0) {
! 6133 const size_t before = reservedBytes;
6134 ret = SQLBindColums(hStmt, buffers, columnNames, numCols, fetchSize, reservedBytes,
6135 charCtype, messages);
6136 pending->reservedBytes += reservedBytes - before;
6137 if (!SQL_SUCCEEDED(ret)) {Lines 6194-6208 6194 const SQLLEN indicator = buffers.indicators[col][idxRowSql];
6195 if (indicator < 0) {
6196 ThrowStdException("Unexpected negative data length.");
6197 }
! 6198 size_t bytes = static_cast<size_t>(indicator);
! 6199 const auto type = dataTypes[col];
! 6200 if (type == SQL_GUID) {
! 6201 bytes = 37;
! 6202 } else if (type == SQL_WCHAR || type == SQL_WVARCHAR ||
! 6203 type == SQL_WLONGVARCHAR || type == SQL_SS_XML ||
! 6204 ((type == SQL_CHAR || type == SQL_VARCHAR ||
6205 type == SQL_LONGVARCHAR) && charCtype == SQL_C_WCHAR)) {
6206 if (bytes % sizeof(SQLWCHAR) != 0) {
6207 ThrowStdException("Wide-character data has an invalid byte length");
6208 }Lines 6289-6297 6289 ? buffers.charBuffers[idxCol].size()
6290 : buffers.charBuffers[idxCol].size() /
6291 static_cast<size_t>(fetchSize);
6292 const size_t sourceOffset = CheckedArrowSourceOffset(
! 6293 buffers.charBuffers[idxCol], idxRowSql, sourceStride, dataLen);
6294
6295 std::memcpy(&(*target_vec)[start],
6296 &buffers.charBuffers[idxCol][sourceOffset], dataLen);
6297 }mssql_python/pybind/ddbc_bindings.hmssql_python/pybind/param_detect.hppLines 141-158 141 inline constexpr int MAX_INLINE_BINARY = 8000;
142
143 inline SQLULEN DAEColumnSize(SQLSMALLINT sqlType, SQLULEN actualSize) {
144 switch (sqlType) {
! 145 case SQL_CHAR:
146 case SQL_VARCHAR:
147 return actualSize > MAX_INLINE_CHAR ? 0 : actualSize;
! 148 case SQL_WCHAR:
149 case SQL_WVARCHAR:
150 return actualSize > MAX_INLINE_CHAR ? 0 : actualSize;
! 151 case SQL_BINARY:
152 case SQL_VARBINARY:
153 return actualSize > MAX_INLINE_BINARY ? 0 : actualSize;
! 154 case SQL_LONGVARCHAR:
155 case SQL_WLONGVARCHAR:
156 case SQL_LONGVARBINARY:
157 return actualSize;
158 default:Lines 237-252 237 if (!chunk) throw py::error_already_set();
238 py::object encoded = encoder.attr("encode")(chunk, end == length);
239 const Py_ssize_t encodedSize = PyBytes_GET_SIZE(encoded.ptr());
240 if (encodedSize > MAX_INLINE_BINARY - total) {
! 241 return MAX_INLINE_BINARY + 1;
! 242 }
243 total += encodedSize;
244 }
245 if (length == 0) {
! 246 py::object encoded = encoder.attr("encode")(py::str(), true);
! 247 total = PyBytes_GET_SIZE(encoded.ptr());
! 248 }
249 return total;
250 }
251
252 inline PyObject* FormatDecimalParam(PyObject* params, Py_ssize_t index, PyObject* value) {Lines 275-290 275 }
276
277 inline long long ValidatedInputSizeInteger(PyObject* value, const char* fieldName) {
278 if (!PyLong_Check(value) || PyBool_Check(value)) {
! 279 throw py::type_error(std::string(fieldName) + " must be an integer");
! 280 }
281 int overflow = 0;
282 const long long result = PyLong_AsLongLongAndOverflow(value, &overflow);
283 if ((result == -1 && PyErr_Occurred()) || overflow != 0) {
! 284 PyErr_Clear();
! 285 throw py::value_error(std::string(fieldName) + " is out of range");
! 286 }
287 return result;
288 }
289
290 inline void ValidateInputSizes(PyObject* inputSizes) {Lines 316-325 316 if (sqlType < std::numeric_limits<SQLSMALLINT>::min() ||
317 sqlType > std::numeric_limits<SQLSMALLINT>::max() ||
318 cType < std::numeric_limits<SQLSMALLINT>::min() ||
319 cType > std::numeric_limits<SQLSMALLINT>::max()) {
! 320 throw py::value_error("SQL and C types must fit in SQLSMALLINT");
! 321 }
322 const bool isNumeric = sqlType == SQL_DECIMAL || sqlType == SQL_NUMERIC;
323 py::int_ zero(0);
324 const int negativeColumnSize =
325 PyObject_RichCompareBool(columnSize, zero.ptr(), Py_LT);Lines 325-340 325 PyObject_RichCompareBool(columnSize, zero.ptr(), Py_LT);
326 const int negativeDecimalDigits =
327 PyObject_RichCompareBool(decimalDigits, zero.ptr(), Py_LT);
328 if (negativeColumnSize == -1 || negativeDecimalDigits == -1) {
! 329 throw py::error_already_set();
! 330 }
331 if (negativeColumnSize == 1) {
332 throw py::value_error("column size must be non-negative");
333 }
334 if (negativeDecimalDigits == 1) {
! 335 throw py::value_error("decimal digits must be non-negative");
! 336 }
337 if (!isNumeric) {
338 const unsigned long long requestedSize = PyLong_AsUnsignedLongLong(columnSize);
339 if (requestedSize == static_cast<unsigned long long>(-1) && PyErr_Occurred()) {
340 PyErr_Clear();Lines 340-349 340 PyErr_Clear();
341 throw py::value_error("column size is out of range");
342 }
343 if (requestedSize > std::numeric_limits<SQLULEN>::max()) {
! 344 throw py::value_error("column size is out of range");
! 345 }
346 const unsigned long long requestedDigits =
347 PyLong_AsUnsignedLongLong(decimalDigits);
348 if (requestedDigits == static_cast<unsigned long long>(-1) && PyErr_Occurred()) {
349 PyErr_Clear();Lines 350-359 350 throw py::value_error("decimal digits are out of range");
351 }
352 if (requestedDigits >
353 static_cast<unsigned long long>(std::numeric_limits<SQLSMALLINT>::max())) {
! 354 throw py::value_error("decimal digits are out of range");
! 355 }
356 }
357 }
358 }Lines 477-486 477 const std::string& charEncoding = "utf-8") {
478 PyTypeCache::initialize();
479
480 if (!PyList_Check(params)) {
! 481 throw py::type_error("params must be a list");
! 482 }
483 ValidateInputSizes(inputSizes);
484
485 const Py_ssize_t n = PyList_GET_SIZE(params);
486 const Py_ssize_t inputSizeCount = inputSizes == Py_None ? 0 : PyList_GET_SIZE(inputSizes);📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 65.6%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.connection.connection_pool.cpp: 82.3%
mssql_python.pybind.ddbc_bindings.cpp: 82.3%
mssql_python.pybind.connection.connection.cpp: 83.2%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%
mssql_python.pybind.param_detect.hpp: 90.2%🔗 Quick Links
|
…nto saumya/native-size-validation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical native buffer-copy and allocation risks, plus incomplete public batch-size validation, remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
github-actions
Bot
added
pr-size: large
Contributor
|
gargsaumya , Can we improve the code coverage? |
Reject boolean, zero, oversized chunk counts and invalid chunk sizes before the internal production test binding can enter its simulation loop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Wide-character binary validation and the direct <limits> include remain unresolved, and native DAE coverage is incomplete.
0 open findings
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Normalize time values before DAE sizing and scan automatic executemany text columns by UTF-16 code units. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Avoid formatting an indeterminate SQLParamData token when the driver reports completion without writing it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Continue SQLParamData after each streamed executemany parameter and reject oversized numeric override text before native copying. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The native binding and fetch changes span several high-risk paths, and the remaining issues need resolution and cross-platform runtime validation.
1 open finding
2 resolved since last review
Previously missed (2)
In code that hasn't changed since last review
mssql_python/cursor.py:2652
The executemany size scan only includes existing str values, but text-bound datetime.time values are converted with isoformat() later, in the processed_parameters loop (cursor.py:2844-2847). With setinputsizes([(SQL_WVARCHAR, 1, 0)]) and a time subclass whose isoformat() returns more than 4,000 UTF-16 units, isDAE stays false; the array binder rejects the normalized value against the declared size instead of streaming it. Measure the normalized text before deciding on array binding, as scalar execute already does, and add a batch regression test.
mssql_python/cursor.py:3040
Validating size here makes synchronous fetchmany(-1) raise ValueError, where it previously returned [] before reaching native code. AsyncCursor.fetchmany(-1) still returns [] (async_query/async_fetch.py:91-95), and its existing test asserts this (tests/AsyncTest/test_007_async_fetch.py:654-655). Preserve the synchronous non-positive short-circuit if that is the intended API contract, or update both cursor APIs and their tests together; rejecting a value that never crosses the native boundary need not create this inconsistency.
🧠 Review effort: Lite
Encode string subclasses from their immutable Unicode contents so overridden encode methods cannot bypass native sizing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Broad native binding and streaming changes need cross-platform runtime review, and automatic time normalization still has an unresolved size gap.
0 open findings
1 resolved since last review
Previously missed (1)
In code that hasn't changed since last review
mssql_python/pybind/param_detect.hpp:620
The new size-based streaming decision only runs for values that arrive as str. Without setinputsizes, DetectParamTypes handles a datetime.time separately: it calls isoformat() and returns without measuring the resulting text. A time subclass whose isoformat() returns a very large string therefore leaves isDAE false, and BindParameters copies that string into an unbounded native UTF-16 buffer. Check the normalized value in the automatic time path too, rejecting oversized text if SQL_TYPE_TIME cannot be streamed safely. The new large-time test covers only an explicit text override.
🧠 Review effort: Lite
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Two executemany sizing issues remain, and the broad native streaming changes need cross-platform runtime review.
1 open finding
Previously missed (1)
In code that hasn't changed since last review
mssql_python/cursor.py:2670
With setinputsizes([(SQL_VARCHAR, 1, 0)]), an executemany row containing a datetime.time subclass whose isoformat() returns 4,001 characters is not included in this str-only scan. The later conversion turns it into text, but array binding has already been selected and rejects its length against the declared size; the same value works through scalar execute, which normalizes before choosing streaming. Normalize and size time values before selecting array binding, or route such rows through scalar execution.
🧠 Review effort: Lite
Bypass string subclass protocol overrides during bounded narrow and UTF-16 executemany sizing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The broad native binding and fetch changes need cross-platform human validation, and a binary-subclass sizing issue remains unresolved.
0 open findings
1 resolved since last review
Previously missed (1)
In code that hasn't changed since last review
mssql_python/cursor.py:2679
Both the new explicit-size scan here and the automatic binary scan at cursor.py:2808-2815 use overridable len(value). A bytes or bytearray subclass can report length 1 while holding 8,001 bytes. executemany() then leaves DAE disabled; the native array binder reads the real buffer size and rejects the row as exceeding its allocated column size, although the scalar path can stream it. Measure the underlying buffer length in both scans so the actual payload selects streaming.
🧠 Review effort: Lite
Sumit Sarabhai (sumitmsft)
left a comment
Sumit Sarabhai (sumitmsft)
left a comment
Contributor
There was a problem hiding this comment.
Revalidated the four previously drafted comments against bf5e738. The row/tuple comparison assertion is fixed, so I have omitted that comment. This is a targeted revalidation of those comments, not a new full review.
Document the compatibility changes
Please document the new 1,000,000-row caps (including bulk-copy rows per commit), the native allocation budgets, and changed negative/boolean-size behavior in API documentation and the changelog. Explain how parameter-array limits depend on batch size and column widths, with guidance for reducing batch sizes. Distinguish native buffer limits from total process or result memory.
Reject oversized formatted numeric batch parameters before native binding. Cover the rejection boundary and the existing wide automatic text binding, and document row caps, allocation budgets, and compatibility changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
gargsaumya
commented
Oct 19, 2026
gargsaumya
commented
Oct 19, 2026
Use dedicated context-managed cursors so expected binding exceptions cannot leave input-size hints on the shared module cursor. Verified the reported binary-rejection then numeric-test ordering: eight cases pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use budget-aware amortized growth and lossless partial Arrow batches with statement-owned pending rows and resumable LOB reads. Preserve pending state across mixed fetch, raw fetch/GetData, scroll/skip, and metadata-only attribute changes; clear it on real result invalidation. Document short-batch semantics and add isolated ordinary/LOB and lifecycle regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
No tracking issue exists for this security hardening work.