Repository navigation
FIX: Validate caller-controlled native buffer sizes - #802
Open
gargsaumya wants to merge 44 commits into
Open
gargsaumya wants to merge 44 commits into
gargsaumya wants to merge 44 commits into
Conversation
PR Performance Report
|
| Database task | Unix / SQL Server 2025 |
|---|---|
| Insertion with explicit input sizes | 22.2% 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 2025 | Insertion with explicit input sizes | 485.794 ms | 593.439 ms | +22.2% |
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 2025
Insertion with explicit input sizes: ddbc::SQLExecute_wrap +12.706 ms; py::execute::cpp_call +12.697 ms; ddbc::BindParameters +1.983 ms.
All database tasks and timings
Unix / SQL Server 2022
| Database task | Before | After | Paired change | Result |
|---|---|---|---|---|
| Connection opening | 10.950 ms | 10.264 ms | -5.7% | no signal |
| SELECT queries | 1.144 ms | 1.096 ms | -5.1% | no signal |
| Row insertion | 35.518 ms | 34.470 ms | -3.4% | no signal |
| Executemany inserts | 162.442 ms | 182.830 ms | +11.1% | no signal |
| Fetch-all queries | 121.760 ms | 120.956 ms | +0.4% | no signal |
| Row-by-row fetching | 14.444 ms | 14.241 ms | +0.2% | no signal |
| Batched row fetching | 116.887 ms | 116.834 ms | +0.4% | no signal |
| Transaction commit and rollback | 116.010 ms | 115.541 ms | -1.7% | no signal |
| Arrow row fetching | 94.893 ms | 95.692 ms | -1.2% | no signal |
| 100,000-row insertion | 463.577 ms | 450.661 ms | -2.6% | no signal |
| Row fetching in batches of 100 | 121.558 ms | 122.287 ms | +0.5% | no signal |
| Row fetching in batches of 10,000 | 137.655 ms | 128.614 ms | -8.1% | no signal |
| Repeated positional queries | 34.264 ms | 34.134 ms | -0.4% | no signal |
| Repeated named-parameter queries | 36.627 ms | 36.332 ms | -0.8% | no signal |
| Legacy 100,000-row insertion | 361.770 ms | 354.937 ms | -3.1% | no signal |
| Insertion with explicit input sizes | 529.656 ms | 591.596 ms | +17.0% | no signal |
| Joined aggregation queries | 177.269 ms | 180.680 ms | +1.3% | no signal |
| Large joined-result fetching | 195.039 ms | 185.596 ms | -3.9% | no signal |
| 1.2-million-row fetching | 3517.614 ms | 3456.285 ms | -0.8% | no signal |
| Common table expression queries | 5.536 ms | 5.451 ms | -1.7% | no signal |
| 256 KiB VARCHAR(MAX) / fetchall() | 1.292 ms | 1.245 ms | -8.2% | no signal |
| 10,000 scalar values / fetchval() (debug disabled) | 107.167 ms | 108.197 ms | +1.0% | no signal |
Unix / SQL Server 2025
| Database task | Before | After | Paired change | Result |
|---|---|---|---|---|
| Connection opening | 97.331 ms | 98.060 ms | +1.1% | no signal |
| SELECT queries | 1.022 ms | 1.079 ms | +6.8% | no signal |
| Row insertion | 33.979 ms | 33.911 ms | -1.3% | no signal |
| Executemany inserts | 151.605 ms | 174.502 ms | +13.6% | no signal |
| Fetch-all queries | 118.695 ms | 120.279 ms | +0.6% | no signal |
| Row-by-row fetching | 14.345 ms | 14.649 ms | +1.2% | no signal |
| Batched row fetching | 115.052 ms | 117.608 ms | +2.2% | no signal |
| Transaction commit and rollback | 112.193 ms | 110.584 ms | -1.6% | no signal |
| Arrow row fetching | 92.735 ms | 95.391 ms | +2.2% | no signal |
| 100,000-row insertion | 422.711 ms | 430.124 ms | +2.7% | no signal |
| Row fetching in batches of 100 | 119.008 ms | 122.533 ms | +3.3% | no signal |
| Row fetching in batches of 10,000 | 131.243 ms | 133.205 ms | +0.1% | no signal |
| Repeated positional queries | 32.659 ms | 32.732 ms | -0.1% | no signal |
| Repeated named-parameter queries | 35.077 ms | 34.948 ms | +0.4% | no signal |
| Legacy 100,000-row insertion | 340.625 ms | 343.307 ms | +1.3% | no signal |
| Insertion with explicit input sizes | 485.794 ms | 593.439 ms | +22.2% | consistent slowdown |
| Joined aggregation queries | 160.996 ms | 159.136 ms | -0.6% | no signal |
| Large joined-result fetching | 174.110 ms | 174.356 ms | +0.7% | no signal |
| 1.2-million-row fetching | 3427.982 ms | 3508.108 ms | +0.5% | no signal |
| Common table expression queries | 5.100 ms | 5.114 ms | -0.3% | no signal |
| 256 KiB VARCHAR(MAX) / fetchall() | 1.492 ms | 1.360 ms | -7.7% | no signal |
| 10,000 scalar values / fetchval() (debug disabled) | 108.410 ms | 107.101 ms | -1.2% | no signal |
Build and measurement details
PR head: bf5e7384fd39ebe07055bf968f054765640f2800
Base: 9424705251baf8ded41a8a4968216b1fd9b00929
Measured merge: fcdbea3be420916f499281994e28de4ebffa6791
- 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 2660-2668 2660 binary_values = [
2661 value for value in column if isinstance(value, (bytes, bytearray))
2662 ]
2663 if c_type == ddbc_sql_const.SQL_CHAR.value:
! 2664 text_is_large = any(
2665 _encoded_length_exceeds(
2666 value,
2667 encoding_settings["encoding"],
2668 MAX_INLINE_CHAR,Lines 2680-2688 2680 requires_row_fallback = bool(
2681 narrow_text_type and text_values and binary_values
2682 )
2683 if narrow_text_type and binary_values and not text_values:
! 2684 c_type = ddbc_sql_const.SQL_CHAR.value
2685 is_dae = text_is_large or binary_is_large
2686
2687 # Sanitize precision/scale for numeric types
2688 if sql_type in (Lines 2708-2716 2708
2709 # For SQL Server VARBINARY(MAX), we need to use large object binding
2710 if max_binary_size > MAX_INLINE_BINARY:
2711 if sql_type != ddbc_sql_const.SQL_SS_UDT.value:
! 2712 sql_type = ddbc_sql_const.SQL_LONGVARBINARY.value
2713 is_dae = True
2714
2715 # Update column_size to actual maximum size if it's larger
2716 # 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 415-425 415 case SQL_C_CHAR:
416 case SQL_C_BINARY:
417 return CheckedAddSize(info.columnSize, 1, "Character parameter size is too large");
418 case SQL_C_BIT:
! 419 return sizeof(char);
! 420 case SQL_C_STINYINT:
! 421 case SQL_C_USHORT:
422 return sizeof(unsigned short);
423 case SQL_C_SBIGINT:
424 case SQL_C_SLONG:
425 case SQL_C_UBIGINT:Lines 427-450 427 return sizeof(int64_t);
428 case SQL_C_FLOAT:
429 return sizeof(float);
430 case SQL_C_TYPE_DATE:
! 431 return sizeof(SQL_DATE_STRUCT);
! 432 case SQL_C_TYPE_TIME:
433 return sizeof(SQL_TIME_STRUCT);
434 case SQL_C_TYPE_TIMESTAMP:
435 return sizeof(SQL_TIMESTAMP_STRUCT);
436 case SQL_C_SS_TIMESTAMPOFFSET:
! 437 return sizeof(DateTimeOffset);
! 438 case SQL_C_NUMERIC:
439 return sizeof(SQL_NUMERIC_STRUCT);
440 case SQL_C_GUID:
441 return sizeof(SQLGUID);
442 case SQL_C_DEFAULT:
! 443 return sizeof(char);
! 444 default:
445 ThrowStdException("Unsupported C type for parameter array allocation");
! 446 }
447 return 0;
448 }
449
450 template <typename ElementType>Lines 454-463 454 CheckedMultiplySize(rowIndex, rowStride, "Arrow source offset is too large");
455 const size_t rowCapacity = CheckedMultiplySize(
456 rowStride, sizeof(ElementType), "Arrow source capacity is too large");
457 if (offset > buffer.size() || rowStride > buffer.size() - offset ||
! 458 dataBytes > rowCapacity) {
! 459 ThrowStdException("Driver data length exceeds the allocated fetch buffer");
460 }
461 return offset;
462 }Lines 462-471 462 }
463
464 template <typename ElementType>
465 std::unique_ptr<ElementType[]> AllocateUniqueArray(size_t count, const char* errorMessage) {
! 466 if (count > std::numeric_limits<size_t>::max() / sizeof(ElementType)) {
! 467 ThrowStdException(errorMessage);
468 }
469 return std::make_unique<ElementType[]>(count);
470 }Lines 548-556 548 const char* errorMessage) {
549 ReserveNativeFetchBytes(reservedBytes, count, sizeof(ElementType));
550 return AllocateUniqueArray<ElementType>(count, errorMessage);
551 }
! 552
553 std::string DescribeChar(unsigned char ch) {
554 if (ch >= 32 && ch <= 126) {
555 return std::string("'") + static_cast<char>(ch) + "'";
556 } else {Lines 591-600 591 if (!chunk) throw py::error_already_set();
592 py::object encoded = encoder.attr("encode")(chunk, end == length);
593 char* data = nullptr;
594 Py_ssize_t size = 0;
! 595 if (PyBytes_AsStringAndSize(encoded.ptr(), &data, &size) != 0) {
! 596 throw py::error_already_set();
597 }
598 if (size > 0) {
599 SQLRETURN rc = put_data_fn(data, static_cast<SQLLEN>(size));
600 if (!SQL_SUCCEEDED(rc)) return rc;Lines 599-615 599 SQLRETURN rc = put_data_fn(data, static_cast<SQLLEN>(size));
600 if (!SQL_SUCCEEDED(rc)) return rc;
601 }
602 }
! 603 if (length == 0) {
! 604 py::object encoded = encoder.attr("encode")(py::str(), true);
! 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) return put_data_fn(data, static_cast<SQLLEN>(size));
! 611 return put_data_fn(nullptr, 0);
612 }
613 return SQL_SUCCESS;
614 }Lines 621-631 621
622 static SQLRETURN StreamDAEParameter(SQLHSTMT hStmt, const ParamInfo& info,
623 const std::string& charEncoding) {
624 PyObject* value = info.dataPtr.ptr();
! 625 if (!value || value == Py_None) {
! 626 py::gil_scoped_release release;
! 627 return SQLPutData_ptr(hStmt, nullptr, 0);
628 }
629
630 auto putImmutableData = [&](SQLPOINTER data, SQLLEN length) {
631 py::gil_scoped_release release;Lines 633-645 633 };
634 if (PyUnicode_Check(value)) {
635 if (info.paramCType == SQL_C_WCHAR) {
636 return stream_unicode_dae_chunks(value, "utf-16-le", putImmutableData);
! 637 }
! 638 if (info.paramCType == SQL_C_CHAR) {
! 639 return stream_unicode_dae_chunks(value, charEncoding, putImmutableData);
! 640 }
! 641 ThrowStdException("DAE only supports text C types for str values");
642 }
643 if (PyBytes_Check(value)) {
644 return stream_dae_chunks(PyBytes_AS_STRING(value),
645 static_cast<size_t>(PyBytes_GET_SIZE(value)),Lines 651-660 651 std::vector<char> chunk(std::min(static_cast<size_t>(DAE_CHUNK_SIZE), totalBytes));
652 for (size_t offset = 0; offset < totalBytes; offset += chunk.size()) {
653 const size_t currentSize = static_cast<size_t>(PyByteArray_GET_SIZE(value));
654 const size_t length = std::min(chunk.size(), totalBytes - offset);
! 655 if (currentSize < offset + length) {
! 656 ThrowStdException("bytearray changed size during DAE streaming");
657 }
658 std::copy_n(PyByteArray_AS_STRING(value) + offset, length, chunk.data());
659 SQLRETURN rc = putImmutableData(chunk.data(), static_cast<SQLLEN>(length));
660 if (!SQL_SUCCEEDED(rc)) return rc;Lines 659-668 659 SQLRETURN rc = putImmutableData(chunk.data(), static_cast<SQLLEN>(length));
660 if (!SQL_SUCCEEDED(rc)) return rc;
661 }
662 return SQL_SUCCESS;
! 663 }
! 664 ThrowStdException("DAE only supports str, bytes, or bytearray values");
665 return SQL_ERROR;
666 }
667
668 // GH-610: Resolve SQL type for a NULL parameter using per-handle cache.Lines 856-864 856 if (py::isinstance<py::str>(param)) {
857 // Encode Unicode string using the specified encoding
858 try {
859 py::object encoded =
! 860 EncodeUnicodeValue(param.ptr(), charEncoding);
861 encodedStr = encoded.cast<std::string>();
862 LOG("BindParameters: param[%d] SQL_C_CHAR - Encoded with '%s', "
863 "size=%zu bytes",
864 paramIndex, charEncoding.c_str(), encodedStr.size());Lines 2562-2573 2562 SQLRETURN describeRc = PreResolveUdtTypes(hStmt, paramInfos);
2563 if (!SQL_SUCCEEDED(describeRc)) return describeRc;
2564 // GH-627: resolve unknown NULL array param SQL types before binding any param.
2565 PreResolveUnknownNullTypes(handle, hStmt, paramInfos);
! 2566 size_t reservedParameterBytes = 0;
! 2567 for (const ParamInfo& info : paramInfos) {
! 2568 ReserveNativeParameterBytes(reservedParameterBytes, paramSetSize,
! 2569 ParameterArrayElementSize(info));
2570 ReserveNativeParameterBytes(reservedParameterBytes, paramSetSize, sizeof(SQLLEN));
2571 }
2572 for (int paramIndex = 0; paramIndex < columnwise_params.size(); ++paramIndex) {
2573 const py::list& columnValues = columnwise_params[paramIndex].cast<py::list>();Lines 2636-2647 2636 elementWidth, sizeof(SQLWCHAR),
2637 "Wide-character parameter length is too large");
2638 if (bufferBytes > static_cast<size_t>(std::numeric_limits<SQLLEN>::max())) {
2639 ThrowStdException("Wide-character parameter length is too large");
! 2640 }
2641 SQLWCHAR* wcharArray = AllocateParamBufferArray<SQLWCHAR>(
! 2642 tempBuffers,
! 2643 CheckedMultiplySize(paramSetSize, elementWidth,
2644 "Wide-character parameter buffer is too large"));
2645 strLenOrIndArray = AllocateParamBufferArray<SQLLEN>(tempBuffers, paramSetSize);
2646 for (size_t i = 0; i < paramSetSize; ++i) {
2647 if (columnValues[i].is_none()) {Lines 2690-2698 2690 LOG("BindParameterArray: SQL_C_WCHAR bound - "
2691 "param_index=%d",
2692 paramIndex);
2693 dataPtr = wcharArray;
! 2694 bufferLength = static_cast<SQLLEN>(bufferBytes);
2695 break;
2696 }
2697 case SQL_C_TINYINT:
2698 case SQL_C_UTINYINT: {Lines 2761-2773 2761 case SQL_C_BINARY: {
2762 LOG("BindParameterArray: Binding SQL_C_CHAR/BINARY array - "
2763 "param_index=%d, count=%zu, column_size=%zu, encoding='%s'",
2764 paramIndex, paramSetSize, info.columnSize, charEncoding.c_str());
! 2765 const size_t elementWidth = CheckedAddSize(
! 2766 info.columnSize, 1, "Character parameter size is too large");
! 2767 if (elementWidth > static_cast<size_t>(std::numeric_limits<SQLLEN>::max())) {
! 2768 ThrowStdException("Character parameter length is too large");
! 2769 }
2770 char* charArray = AllocateParamBufferArray<char>(
2771 tempBuffers,
2772 CheckedMultiplySize(paramSetSize, elementWidth,
2773 "Character parameter buffer is too large"));Lines 2777-2785 2777 strLenOrIndArray[i] = SQL_NULL_DATA;
2778 std::memset(charArray + i * (info.columnSize + 1), 0,
2779 info.columnSize + 1);
2780 } else {
! 2781 if (info.paramCType == SQL_C_BINARY &&
2782 !py::isinstance<py::bytes>(columnValues[i]) &&
2783 !py::isinstance<py::bytearray>(columnValues[i])) {
2784 ThrowStdException(MakeParamMismatchErrorStr(info.paramCType,
2785 paramIndex));Lines 2794-2803 2794 encoded =
2795 EncodeUnicodeValue(columnValues[i].ptr(), charEncoding);
2796 if (PyBytes_AsStringAndSize(encoded.ptr(), &encodedData,
2797 &encodedSize) != 0) {
! 2798 throw py::error_already_set();
! 2799 }
2800 LOG("BindParameterArray: param[%d] row[%zu] SQL_C_CHAR - "
2801 "Encoded with '%s', "
2802 "size=%zu bytes",
2803 paramIndex, i, charEncoding.c_str(),Lines 2828-2846 2828 ThrowStdException(
2829 MakeParamMismatchErrorStr(info.paramCType, paramIndex));
2830 }
2831
! 2832 const size_t dataSize = static_cast<size_t>(encodedSize);
! 2833 if (dataSize > info.columnSize) {
2834 LOG("BindParameterArray: String/binary too "
2835 "long - param_index=%d, row=%zu, size=%zu, "
2836 "max=%zu",
! 2837 paramIndex, i, dataSize, info.columnSize);
2838 ThrowStdException("Input exceeds column size at index " +
2839 std::to_string(i));
2840 }
! 2841 std::copy_n(encodedData, dataSize, charArray + i * elementWidth);
! 2842 strLenOrIndArray[i] = static_cast<SQLLEN>(dataSize);
2843 }
2844 }
2845 LOG("BindParameterArray: SQL_C_CHAR/BINARY bound - "
2846 "param_index=%d",Lines 2845-2853 2845 LOG("BindParameterArray: SQL_C_CHAR/BINARY bound - "
2846 "param_index=%d",
2847 paramIndex);
2848 dataPtr = charArray;
! 2849 bufferLength = static_cast<SQLLEN>(elementWidth);
2850 break;
2851 }
2852 case SQL_C_BIT: {
2853 LOG("BindParameterArray: Binding SQL_C_BIT array - "Lines 3354-3363 3354 std::vector<ParamInfo> rowParamInfos = paramInfos;
3355 for (size_t paramIndex = 0; paramIndex < rowParamInfos.size(); ++paramIndex) {
3356 if (rowParams[paramIndex].is_none()) {
3357 rowParamInfos[paramIndex].paramCType = SQL_C_DEFAULT;
! 3358 rowParamInfos[paramIndex].isDAE = false;
! 3359 rowParamInfos[paramIndex].dataPtr = py::none();
3360 }
3361 }
3362 std::vector<std::shared_ptr<void>> paramBuffers;
3363 rc = BindParameters(*statementHandle, hStmt, rowParams, rowParamInfos,Lines 3375-3383 3375 }
3376 LOG("SQLExecuteMany: SQLExecute for row %zu - initial_rc=%d", rowIndex, rc);
3377 size_t dae_chunk_count = 0;
3378 while (rc == SQL_NEED_DATA) {
! 3379 SQLPOINTER token = nullptr;
3380 {
3381 // Release the GIL around the blocking SQLParamData call.
3382 py::gil_scoped_release release;
3383 rc = SQLParamData_ptr(hStmt, &token);Lines 3414-3427 3414 return rc;
3415 }
3416
3417 const SQLRETURN rowRc = rc;
! 3418 rc = SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS);
! 3419 if (!SQL_SUCCEEDED(rc)) {
! 3420 LOG("SQLExecuteMany: SQL_RESET_PARAMS failed for row %zu - "
! 3421 "rc=%d",
! 3422 rowIndex, rc);
! 3423 return rc;
3424 }
3425 rc = rowRc;
3426 }
3427 LOG("SQLExecuteMany: All DAE rows processed successfully - "Lines 3679-3689 3679 LOG("FetchLobColumnData: SQL_SUCCESS - no more data at loop %d", loopCount);
3680 break;
3681 }
3682 }
! 3683 LOG("FetchLobColumnData: Total bytes collected=%zu for column %d", dataSize, colIndex);
3684
! 3685 if (dataSize == 0) {
3686 if (isBinary) {
3687 return py::bytes("");
3688 }
3689 return py::str("");Lines 4724-4732 4724 ret = SQLBindCol_ptr(hStmt, col, SQL_C_TINYINT, buffers.charBuffers[col - 1].data(),
4725 sizeof(SQLCHAR), buffers.indicators[col - 1].data());
4726 break;
4727 case SQL_BIT:
! 4728 ResizeNativeFetchBuffer(buffers.charBuffers[col - 1], fetchSize, reservedBytes);
4729 ret = SQLBindCol_ptr(hStmt, col, SQL_C_BIT, buffers.charBuffers[col - 1].data(),
4730 sizeof(SQLCHAR), buffers.indicators[col - 1].data());
4731 break;
4732 case SQL_REAL:Lines 4746-4754 4746 buffers.indicators[col - 1].data());
4747 break;
4748 case SQL_DOUBLE:
4749 case SQL_FLOAT:
! 4750 ResizeNativeFetchBuffer(buffers.doubleBuffers[col - 1], fetchSize, reservedBytes);
4751 ret =
4752 SQLBindCol_ptr(hStmt, col, SQL_C_DOUBLE, buffers.doubleBuffers[col - 1].data(),
4753 sizeof(SQLDOUBLE), buffers.indicators[col - 1].data());
4754 break;Lines 4767-4775 4767 SQLBindCol_ptr(hStmt, col, SQL_C_SBIGINT, buffers.bigIntBuffers[col - 1].data(),
4768 sizeof(SQLBIGINT), buffers.indicators[col - 1].data());
4769 break;
4770 case SQL_TYPE_DATE:
! 4771 ResizeNativeFetchBuffer(buffers.dateBuffers[col - 1], fetchSize, reservedBytes);
4772 ret =
4773 SQLBindCol_ptr(hStmt, col, SQL_C_TYPE_DATE, buffers.dateBuffers[col - 1].data(),
4774 sizeof(SQL_DATE_STRUCT), buffers.indicators[col - 1].data());
4775 break;Lines 4773-4781 4773 SQLBindCol_ptr(hStmt, col, SQL_C_TYPE_DATE, buffers.dateBuffers[col - 1].data(),
4774 sizeof(SQL_DATE_STRUCT), buffers.indicators[col - 1].data());
4775 break;
4776 case SQL_SS_TIME2:
! 4777 ResizeNativeFetchBuffer(buffers.timeBuffers[col - 1], fetchSize, reservedBytes);
4778 ret =
4779 SQLBindCol_ptr(hStmt, col, SQL_C_SS_TIME2, buffers.timeBuffers[col - 1].data(),
4780 sizeof(SQL_SS_TIME2_STRUCT), buffers.indicators[col - 1].data());
4781 break;Lines 4799-4809 4799 ret = SQLBindCol_ptr(hStmt, col, SQL_C_BINARY, buffers.charBuffers[col - 1].data(),
4800 CheckedFetchBufferLength(fetchBufferSize, 1),
4801 buffers.indicators[col - 1].data());
4802 break;
! 4803 }
4804 case SQL_SS_TIMESTAMPOFFSET:
! 4805 ResizeNativeFetchBuffer(buffers.datetimeoffsetBuffers[col - 1], fetchSize,
4806 reservedBytes);
4807 ret = SQLBindCol_ptr(hStmt, col, SQL_C_SS_TIMESTAMPOFFSET,
4808 buffers.datetimeoffsetBuffers[col - 1].data(),
4809 sizeof(DateTimeOffset),Lines 4844-4852 4844 PERF_TIMER("FetchBatchData");
4845 LOG("FetchBatchData: Fetching data in batches");
4846 SQLRETURN ret;
4847 {
! 4848 numRowsFetched = 0;
4849 // Release the GIL during the blocking ODBC fetch
4850 py::gil_scoped_release release;
4851 PERF_TIMER("FetchBatchData::SQLFetchScroll_call");
4852 ret = SQLFetchScroll_ptr(hStmt, SQL_FETCH_NEXT, 0);Lines 5481-5489 5481 FetchStateGuard fetchStateGuard(StatementHandle, messages);
5482
5483 // Bind columns
5484 ret = SQLBindColums(hStmt, buffers, columnNames, numCols, fetchSize, reservedBytes, charCtype,
! 5485 messages);
5486 if (!SQL_SUCCEEDED(ret)) {
5487 LOG("FetchMany_wrap: Error when binding columns - SQLRETURN=%d", ret);
5488 return ret;
5489 }Lines 5621-5629 5621 const size_t offsetCount = CheckedAddSize(batchSize, 1, "Arrow batch size is too large");
5622 const size_t initialVarDataSize =
5623 CheckedMultiplySize(batchSize, 42, "Arrow batch size is too large");
5624 const size_t bitmapSize =
! 5625 CheckedAddSize(batchSize, 7, "Arrow batch size is too large") / 8;
5626 // Fetch narrow char data as SQL_C_CHAR if on Linux/macOS and configured by the user
5627 charCtype = EffectiveCharCtypeForFetch(charCtype, "utf-8");
5628
5629 // An overly large fetch size doesn't seem to help performance.Lines 5651-5659 5651 std::vector<SQLSMALLINT> dataTypes(numCols);
5652 std::vector<bool> columnNullable(numCols);
5653 std::vector<bool> columnVarLen(numCols, false);
5654 std::vector<int64_t> nullCounts(numCols, 0);
! 5655 size_t reservedBytes = 0;
5656
5657 std::vector<std::unique_ptr<ArrowArrayPrivateData>> arrowArrayPrivateData(numCols);
5658 std::vector<std::unique_ptr<ArrowSchemaPrivateData>> arrowSchemaPrivateData(numCols);
5659 for (SQLSMALLINT i = 0; i < numCols; i++) {Lines 5713-5721 5713 arrowColumnProducer->varVal =
5714 AllocateArrowArray<uint64_t>(offsetCount, reservedBytes,
5715 "Arrow offset buffer is too large");
5716 ResizeNativeFetchBuffer(arrowColumnProducer->varData, initialVarDataSize,
! 5717 reservedBytes);
5718 columnVarLen[i] = true;
5719 // start at offset 0
5720 arrowColumnProducer->varVal[0] = 0;
5721 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->varVal.get();Lines 5728-5737 5728 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->uint8Val.get();
5729 break;
5730 case SQL_SMALLINT:
5731 format = "s";
! 5732 arrowColumnProducer->int16Val =
! 5733 AllocateArrowArray<int16_t>(batchSize, reservedBytes,
5734 "Arrow value buffer is too large");
5735 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->int16Val.get();
5736 break;
5737 case SQL_INTEGER:Lines 5742-5751 5742 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->int32Val.get();
5743 break;
5744 case SQL_BIGINT:
5745 format = "l";
! 5746 arrowColumnProducer->int64Val =
! 5747 AllocateArrowArray<int64_t>(batchSize, reservedBytes,
5748 "Arrow value buffer is too large");
5749 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->int64Val.get();
5750 break;
5751 case SQL_REAL:Lines 5757-5766 5757 break;
5758 case SQL_FLOAT:
5759 case SQL_DOUBLE:
5760 format = "g";
! 5761 arrowColumnProducer->float64Val =
! 5762 AllocateArrowArray<double>(batchSize, reservedBytes,
5763 "Arrow value buffer is too large");
5764 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->float64Val.get();
5765 break;
5766 case SQL_DECIMAL:Lines 5782-5792 5782 case SQL_TIMESTAMP:
5783 case SQL_TYPE_TIMESTAMP:
5784 case SQL_DATETIME:
5785 format = "tsu:";
! 5786 arrowColumnProducer->tsMicroVal =
! 5787 AllocateArrowArray<int64_t>(batchSize, reservedBytes,
! 5788 "Arrow value buffer is too large");
5789 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->tsMicroVal.get();
5790 break;
5791 case SQL_SS_TIMESTAMPOFFSET:
5792 format = "tsu:+00:00";Lines 5796-5805 5796 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->tsMicroVal.get();
5797 break;
5798 case SQL_TYPE_DATE:
5799 format = "tdD";
! 5800 arrowColumnProducer->dateVal =
! 5801 AllocateArrowArray<int32_t>(batchSize, reservedBytes,
5802 "Arrow value buffer is too large");
5803 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->dateVal.get();
5804 break;
5805 case SQL_SS_TIME2:Lines 6192-6200 6192 ? buffers.charBuffers[idxCol].size()
6193 : buffers.charBuffers[idxCol].size() /
6194 static_cast<size_t>(fetchSize);
6195 const size_t sourceOffset = CheckedArrowSourceOffset(
! 6196 buffers.charBuffers[idxCol], idxRowSql, sourceStride, dataLen);
6197
6198 std::memcpy(&(*target_vec)[start],
6199 &buffers.charBuffers[idxCol][sourceOffset], dataLen);
6200 }mssql_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: 62.6%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 81.2%
mssql_python.pybind.connection.connection_pool.cpp: 82.3%
mssql_python.pybind.connection.connection.cpp: 83.1%
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
Release ODBC parameter bindings before row-local buffers and SQLParamData tokens leave scope, and surface reset failures without disturbing execution diagnostics on error paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Track LOB payload length separately from geometric allocation size, validate native executemany counts and input-size metadata before handle mutation, and add direct boundary and growth-count regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Declared-size DAE behavior, numeric conversion limits, and peak wide-buffer accounting remain unresolved.
0 open findings
Previously missed (1)
In code that hasn't changed since last review
mssql_python/pybind/param_detect.hpp:423
The numeric override path is explicitly excluded from the actual-size/DAE guard. A caller can set SQL_DECIMAL/SQL_NUMERIC and pass a very large string; ApplyInputSizeOverride leaves it non-DAE, and BindParameters then materializes a full std::u16string before SQL Server rejects the value, bypassing the new native allocation limit. Reject or precision-validate oversized numeric text before this conversion.
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
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
Comment on lines
+2677
to
+2685
| binary_is_large = narrow_text_type and any( | ||
| len(value) > MAX_INLINE_BINARY for value in binary_values | ||
| ) | ||
| requires_row_fallback = bool( | ||
| narrow_text_type and text_values and binary_values | ||
| ) | ||
| if narrow_text_type and binary_values and not text_values: | ||
| c_type = ddbc_sql_const.SQL_CHAR.value | ||
| is_dae = text_is_large or binary_is_large |
Comment on lines
+2793
to
+2797
| ) and any( | ||
| isinstance(value, str) and _utf16_length_exceeds(value, MAX_INLINE_CHAR) | ||
| for value in column | ||
| ): | ||
| paraminfo.isDAE = True |
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.