Repository navigation
FIX: Validate caller-controlled native buffer sizes - #802
Open
gargsaumya wants to merge 34 commits into
Open
gargsaumya wants to merge 34 commits into
gargsaumya wants to merge 34 commits into
Conversation
PR Performance Report
|
| Database task | Unix / SQL Server 2022 | Unix / SQL Server 2025 |
|---|---|---|
| 256 KiB VARCHAR(MAX) / fetchall() | 103.2% slower | 79.7% 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 | 256 KiB VARCHAR(MAX) / fetchall() | 1.228 ms | 2.461 ms | +103.2% |
| Unix / SQL Server 2025 | 256 KiB VARCHAR(MAX) / fetchall() | 1.525 ms | 2.753 ms | +79.7% |
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
256 KiB VARCHAR(MAX) / fetchall(): ddbc::FetchAll_wrap +1.231 ms; py::fetchall::cpp_call +1.231 ms; ddbc::SQLGetData_wrap +1.229 ms.
Unix / SQL Server 2025
256 KiB VARCHAR(MAX) / fetchall(): ddbc::FetchLobColumnData +1.245 ms; ddbc::SQLGetData_wrap +1.244 ms; ddbc::FetchAll_wrap +1.235 ms.
All database tasks and timings
Unix / SQL Server 2022
| Database task | Before | After | Paired change | Result |
|---|---|---|---|---|
| Connection opening | 10.382 ms | 10.247 ms | -1.3% | no signal |
| SELECT queries | 1.070 ms | 1.106 ms | +0.8% | no signal |
| Row insertion | 34.324 ms | 34.906 ms | -0.1% | no signal |
| Executemany inserts | 157.475 ms | 173.673 ms | +10.1% | no signal |
| Fetch-all queries | 121.321 ms | 120.789 ms | -0.0% | no signal |
| Row-by-row fetching | 14.311 ms | 14.218 ms | -1.6% | no signal |
| Batched row fetching | 115.740 ms | 117.942 ms | +1.7% | no signal |
| Transaction commit and rollback | 114.372 ms | 114.946 ms | +0.5% | no signal |
| Arrow row fetching | 94.264 ms | 96.084 ms | +1.9% | no signal |
| 100,000-row insertion | 463.502 ms | 454.789 ms | -2.3% | no signal |
| Row fetching in batches of 100 | 121.941 ms | 123.889 ms | +0.2% | no signal |
| Row fetching in batches of 10,000 | 131.885 ms | 140.823 ms | +9.8% | no signal |
| Repeated positional queries | 33.673 ms | 33.896 ms | +0.0% | no signal |
| Repeated named-parameter queries | 35.801 ms | 36.712 ms | +1.7% | no signal |
| Legacy 100,000-row insertion | 353.350 ms | 358.110 ms | +1.3% | no signal |
| Insertion with explicit input sizes | 502.255 ms | 592.667 ms | +17.9% | no signal |
| Joined aggregation queries | 181.330 ms | 184.856 ms | -0.3% | no signal |
| Large joined-result fetching | 189.726 ms | 197.129 ms | +2.5% | no signal |
| 1.2-million-row fetching | 3501.123 ms | 3475.212 ms | -1.0% | no signal |
| Common table expression queries | 5.313 ms | 5.331 ms | -0.4% | no signal |
| 256 KiB VARCHAR(MAX) / fetchall() | 1.228 ms | 2.461 ms | +103.2% | consistent slowdown |
| 10,000 scalar values / fetchval() (debug disabled) | 106.937 ms | 108.565 ms | +2.4% | no signal |
Unix / SQL Server 2025
| Database task | Before | After | Paired change | Result |
|---|---|---|---|---|
| Connection opening | 97.502 ms | 97.097 ms | -0.4% | no signal |
| SELECT queries | 1.141 ms | 1.132 ms | -2.6% | no signal |
| Row insertion | 35.550 ms | 35.638 ms | +3.1% | no signal |
| Executemany inserts | 160.061 ms | 165.116 ms | +9.2% | no signal |
| Fetch-all queries | 123.143 ms | 125.260 ms | +0.7% | no signal |
| Row-by-row fetching | 14.453 ms | 14.636 ms | -1.7% | no signal |
| Batched row fetching | 120.519 ms | 120.792 ms | -0.0% | no signal |
| Transaction commit and rollback | 116.607 ms | 117.150 ms | +0.9% | no signal |
| Arrow row fetching | 95.782 ms | 97.453 ms | +1.1% | no signal |
| 100,000-row insertion | 459.060 ms | 455.400 ms | +2.9% | no signal |
| Row fetching in batches of 100 | 123.715 ms | 125.341 ms | +0.6% | no signal |
| Row fetching in batches of 10,000 | 132.998 ms | 146.219 ms | +9.9% | no signal |
| Repeated positional queries | 33.983 ms | 34.256 ms | -0.5% | no signal |
| Repeated named-parameter queries | 36.478 ms | 36.639 ms | +0.6% | no signal |
| Legacy 100,000-row insertion | 399.570 ms | 374.319 ms | -6.3% | no signal |
| Insertion with explicit input sizes | 501.446 ms | 609.312 ms | +19.6% | no signal |
| Joined aggregation queries | 160.034 ms | 159.658 ms | -0.6% | no signal |
| Large joined-result fetching | 194.861 ms | 190.231 ms | -3.4% | no signal |
| 1.2-million-row fetching | 3559.067 ms | 3621.418 ms | +0.3% | no signal |
| Common table expression queries | 5.414 ms | 5.296 ms | +0.1% | no signal |
| 256 KiB VARCHAR(MAX) / fetchall() | 1.525 ms | 2.753 ms | +79.7% | consistent slowdown |
| 10,000 scalar values / fetchval() (debug disabled) | 108.381 ms | 109.237 ms | -0.3% | no signal |
Build and measurement details
PR head: f53323085fcf86a70bc68804b17e8b031a5818b5
Base: 666f3cb6d23981bb23cd182ec273df10a7b2c805
Measured merge: 5333a64288907cdafc76f05765a432b389c9999a
- Unix / SQL Server 2022: Python 3.12.3, x86_64, SQL 16.0.4295.3; 5 paired comparisons and 1 warmup.
- Unix / SQL Server 2025: Python 3.12.3, x86_64, SQL 17.0.5005.3; 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-76 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 for offset in range(0, len(value), chunk_size):
! 66 end = min(offset + chunk_size, len(value))
! 67 total += len(encoder.encode(value[offset:end], final=end == len(value)))
! 68 if total > limit:
! 69 return True
! 70 if not value:
! 71 total += len(encoder.encode("", final=True))
! 72 return total > limit
73
74
75 # SQL BIGINT is a signed 64-bit integer. Ints outside this range have no BIGINT
76 # encoding and must be rejected at detect time on both paths (see _map_sql_type).Lines 2653-2661 2653 binary_values = [
2654 value for value in column if isinstance(value, (bytes, bytearray))
2655 ]
2656 if c_type == ddbc_sql_const.SQL_CHAR.value:
! 2657 text_is_large = any(
2658 _encoded_length_exceeds(
2659 value,
2660 encoding_settings["encoding"],
2661 MAX_INLINE_CHAR,Lines 2674-2682 2674 requires_row_fallback = bool(
2675 narrow_text_type and text_values and binary_values
2676 )
2677 if narrow_text_type and binary_values and not text_values:
! 2678 c_type = ddbc_sql_const.SQL_CHAR.value
2679 is_dae = text_is_large or binary_is_large
2680
2681 # Sanitize precision/scale for numeric types
2682 if sql_type in (Lines 2702-2710 2702
2703 # For SQL Server VARBINARY(MAX), we need to use large object binding
2704 if max_binary_size > MAX_INLINE_BINARY:
2705 if sql_type != ddbc_sql_const.SQL_SS_UDT.value:
! 2706 sql_type = ddbc_sql_const.SQL_LONGVARBINARY.value
2707 is_dae = True
2708
2709 # Update column_size to actual maximum size if it's larger
2710 # 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 516-524 516 const char* errorMessage) {
517 ReserveNativeFetchBytes(reservedBytes, count, sizeof(ElementType));
518 return AllocateUniqueArray<ElementType>(count, errorMessage);
519 }
! 520
521 std::string DescribeChar(unsigned char ch) {
522 if (ch >= 32 && ch <= 126) {
523 return std::string("'") + static_cast<char>(ch) + "'";
524 } else {Lines 559-568 559 if (!chunk) throw py::error_already_set();
560 py::object encoded = encoder.attr("encode")(chunk, end == length);
561 char* data = nullptr;
562 Py_ssize_t size = 0;
! 563 if (PyBytes_AsStringAndSize(encoded.ptr(), &data, &size) != 0) {
! 564 throw py::error_already_set();
565 }
566 if (size > 0) {
567 SQLRETURN rc = put_data_fn(data, static_cast<SQLLEN>(size));
568 if (!SQL_SUCCEEDED(rc)) return rc;Lines 567-583 567 SQLRETURN rc = put_data_fn(data, static_cast<SQLLEN>(size));
568 if (!SQL_SUCCEEDED(rc)) return rc;
569 }
570 }
! 571 if (length == 0) {
! 572 py::object encoded = encoder.attr("encode")(py::str(), true);
! 573 char* data = nullptr;
! 574 Py_ssize_t size = 0;
! 575 if (PyBytes_AsStringAndSize(encoded.ptr(), &data, &size) != 0) {
! 576 throw py::error_already_set();
! 577 }
! 578 if (size > 0) return put_data_fn(data, static_cast<SQLLEN>(size));
! 579 return put_data_fn(nullptr, 0);
580 }
581 return SQL_SUCCESS;
582 }Lines 583-593 583
584 static SQLRETURN StreamDAEParameter(SQLHSTMT hStmt, const ParamInfo& info,
585 const std::string& charEncoding) {
586 PyObject* value = info.dataPtr.ptr();
! 587 if (!value || value == Py_None) {
! 588 py::gil_scoped_release release;
! 589 return SQLPutData_ptr(hStmt, nullptr, 0);
590 }
591
592 auto putImmutableData = [&](SQLPOINTER data, SQLLEN length) {
593 py::gil_scoped_release release;Lines 595-607 595 };
596 if (PyUnicode_Check(value)) {
597 if (info.paramCType == SQL_C_WCHAR) {
598 return stream_unicode_dae_chunks(value, "utf-16-le", putImmutableData);
! 599 }
! 600 if (info.paramCType == SQL_C_CHAR) {
! 601 return stream_unicode_dae_chunks(value, charEncoding, putImmutableData);
! 602 }
! 603 ThrowStdException("DAE only supports text C types for str values");
604 }
605 if (PyBytes_Check(value)) {
606 return stream_dae_chunks(PyBytes_AS_STRING(value),
607 static_cast<size_t>(PyBytes_GET_SIZE(value)),Lines 613-622 613 std::vector<char> chunk(std::min(static_cast<size_t>(DAE_CHUNK_SIZE), totalBytes));
614 for (size_t offset = 0; offset < totalBytes; offset += chunk.size()) {
615 const size_t currentSize = static_cast<size_t>(PyByteArray_GET_SIZE(value));
616 const size_t length = std::min(chunk.size(), totalBytes - offset);
! 617 if (currentSize < offset + length) {
! 618 ThrowStdException("bytearray changed size during DAE streaming");
619 }
620 std::copy_n(PyByteArray_AS_STRING(value) + offset, length, chunk.data());
621 SQLRETURN rc = putImmutableData(chunk.data(), static_cast<SQLLEN>(length));
622 if (!SQL_SUCCEEDED(rc)) return rc;Lines 621-630 621 SQLRETURN rc = putImmutableData(chunk.data(), static_cast<SQLLEN>(length));
622 if (!SQL_SUCCEEDED(rc)) return rc;
623 }
624 return SQL_SUCCESS;
! 625 }
! 626 ThrowStdException("DAE only supports str, bytes, or bytearray values");
627 return SQL_ERROR;
628 }
629
630 // GH-610: Resolve SQL type for a NULL parameter using per-handle cache.Lines 2522-2533 2522 SQLRETURN describeRc = PreResolveUdtTypes(hStmt, paramInfos);
2523 if (!SQL_SUCCEEDED(describeRc)) return describeRc;
2524 // GH-627: resolve unknown NULL array param SQL types before binding any param.
2525 PreResolveUnknownNullTypes(handle, hStmt, paramInfos);
! 2526 size_t reservedParameterBytes = 0;
! 2527 for (const ParamInfo& info : paramInfos) {
! 2528 ReserveNativeParameterBytes(reservedParameterBytes, paramSetSize,
! 2529 ParameterArrayElementSize(info));
2530 ReserveNativeParameterBytes(reservedParameterBytes, paramSetSize, sizeof(SQLLEN));
2531 }
2532 for (int paramIndex = 0; paramIndex < columnwise_params.size(); ++paramIndex) {
2533 const py::list& columnValues = columnwise_params[paramIndex].cast<py::list>();Lines 2596-2607 2596 elementWidth, sizeof(SQLWCHAR),
2597 "Wide-character parameter length is too large");
2598 if (bufferBytes > static_cast<size_t>(std::numeric_limits<SQLLEN>::max())) {
2599 ThrowStdException("Wide-character parameter length is too large");
! 2600 }
2601 SQLWCHAR* wcharArray = AllocateParamBufferArray<SQLWCHAR>(
! 2602 tempBuffers,
! 2603 CheckedMultiplySize(paramSetSize, elementWidth,
2604 "Wide-character parameter buffer is too large"));
2605 strLenOrIndArray = AllocateParamBufferArray<SQLLEN>(tempBuffers, paramSetSize);
2606 for (size_t i = 0; i < paramSetSize; ++i) {
2607 if (columnValues[i].is_none()) {Lines 2650-2658 2650 LOG("BindParameterArray: SQL_C_WCHAR bound - "
2651 "param_index=%d",
2652 paramIndex);
2653 dataPtr = wcharArray;
! 2654 bufferLength = static_cast<SQLLEN>(bufferBytes);
2655 break;
2656 }
2657 case SQL_C_TINYINT:
2658 case SQL_C_UTINYINT: {Lines 2721-2733 2721 case SQL_C_BINARY: {
2722 LOG("BindParameterArray: Binding SQL_C_CHAR/BINARY array - "
2723 "param_index=%d, count=%zu, column_size=%zu, encoding='%s'",
2724 paramIndex, paramSetSize, info.columnSize, charEncoding.c_str());
! 2725 const size_t elementWidth = CheckedAddSize(
! 2726 info.columnSize, 1, "Character parameter size is too large");
! 2727 if (elementWidth > static_cast<size_t>(std::numeric_limits<SQLLEN>::max())) {
! 2728 ThrowStdException("Character parameter length is too large");
! 2729 }
2730 char* charArray = AllocateParamBufferArray<char>(
2731 tempBuffers,
2732 CheckedMultiplySize(paramSetSize, elementWidth,
2733 "Character parameter buffer is too large"));Lines 2737-2745 2737 strLenOrIndArray[i] = SQL_NULL_DATA;
2738 std::memset(charArray + i * (info.columnSize + 1), 0,
2739 info.columnSize + 1);
2740 } else {
! 2741 if (info.paramCType == SQL_C_BINARY &&
2742 !py::isinstance<py::bytes>(columnValues[i]) &&
2743 !py::isinstance<py::bytearray>(columnValues[i])) {
2744 ThrowStdException(MakeParamMismatchErrorStr(info.paramCType,
2745 paramIndex));Lines 2754-2763 2754 encoded =
2755 columnValues[i].attr("encode")(charEncoding, "strict");
2756 if (PyBytes_AsStringAndSize(encoded.ptr(), &encodedData,
2757 &encodedSize) != 0) {
! 2758 throw py::error_already_set();
! 2759 }
2760 LOG("BindParameterArray: param[%d] row[%zu] SQL_C_CHAR - "
2761 "Encoded with '%s', "
2762 "size=%zu bytes",
2763 paramIndex, i, charEncoding.c_str(),Lines 2788-2806 2788 ThrowStdException(
2789 MakeParamMismatchErrorStr(info.paramCType, paramIndex));
2790 }
2791
! 2792 const size_t dataSize = static_cast<size_t>(encodedSize);
! 2793 if (dataSize > info.columnSize) {
2794 LOG("BindParameterArray: String/binary too "
2795 "long - param_index=%d, row=%zu, size=%zu, "
2796 "max=%zu",
! 2797 paramIndex, i, dataSize, info.columnSize);
2798 ThrowStdException("Input exceeds column size at index " +
2799 std::to_string(i));
2800 }
! 2801 std::copy_n(encodedData, dataSize, charArray + i * elementWidth);
! 2802 strLenOrIndArray[i] = static_cast<SQLLEN>(dataSize);
2803 }
2804 }
2805 LOG("BindParameterArray: SQL_C_CHAR/BINARY bound - "
2806 "param_index=%d",Lines 2805-2813 2805 LOG("BindParameterArray: SQL_C_CHAR/BINARY bound - "
2806 "param_index=%d",
2807 paramIndex);
2808 dataPtr = charArray;
! 2809 bufferLength = static_cast<SQLLEN>(elementWidth);
2810 break;
2811 }
2812 case SQL_C_BIT: {
2813 LOG("BindParameterArray: Binding SQL_C_BIT array - "Lines 3308-3317 3308
3309 std::vector<ParamInfo> rowParamInfos = paramInfos;
3310 for (size_t paramIndex = 0; paramIndex < rowParamInfos.size(); ++paramIndex) {
3311 if (rowParams[paramIndex].is_none()) {
! 3312 rowParamInfos[paramIndex].paramCType = SQL_C_DEFAULT;
! 3313 rowParamInfos[paramIndex].isDAE = false;
3314 rowParamInfos[paramIndex].dataPtr = py::none();
3315 }
3316 }
3317 std::vector<std::shared_ptr<void>> paramBuffers;Lines 3367-3380 3367 LOG("SQLExecuteMany: DAE row %zu failed - rc=%d", rowIndex, rc);
3368 return rc;
3369 }
3370
! 3371 const SQLRETURN rowRc = rc;
! 3372 rc = SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS);
! 3373 if (!SQL_SUCCEEDED(rc)) {
! 3374 LOG("SQLExecuteMany: SQL_RESET_PARAMS failed for row %zu - "
! 3375 "rc=%d",
! 3376 rowIndex, rc);
3377 return rc;
3378 }
3379 rc = rowRc;
3380 }Lines 4679-4687 4679 ret = SQLBindCol_ptr(hStmt, col, SQL_C_TINYINT, buffers.charBuffers[col - 1].data(),
4680 sizeof(SQLCHAR), buffers.indicators[col - 1].data());
4681 break;
4682 case SQL_BIT:
! 4683 ResizeNativeFetchBuffer(buffers.charBuffers[col - 1], fetchSize, reservedBytes);
4684 ret = SQLBindCol_ptr(hStmt, col, SQL_C_BIT, buffers.charBuffers[col - 1].data(),
4685 sizeof(SQLCHAR), buffers.indicators[col - 1].data());
4686 break;
4687 case SQL_REAL:Lines 4701-4709 4701 buffers.indicators[col - 1].data());
4702 break;
4703 case SQL_DOUBLE:
4704 case SQL_FLOAT:
! 4705 ResizeNativeFetchBuffer(buffers.doubleBuffers[col - 1], fetchSize, reservedBytes);
4706 ret =
4707 SQLBindCol_ptr(hStmt, col, SQL_C_DOUBLE, buffers.doubleBuffers[col - 1].data(),
4708 sizeof(SQLDOUBLE), buffers.indicators[col - 1].data());
4709 break;Lines 4722-4730 4722 SQLBindCol_ptr(hStmt, col, SQL_C_SBIGINT, buffers.bigIntBuffers[col - 1].data(),
4723 sizeof(SQLBIGINT), buffers.indicators[col - 1].data());
4724 break;
4725 case SQL_TYPE_DATE:
! 4726 ResizeNativeFetchBuffer(buffers.dateBuffers[col - 1], fetchSize, reservedBytes);
4727 ret =
4728 SQLBindCol_ptr(hStmt, col, SQL_C_TYPE_DATE, buffers.dateBuffers[col - 1].data(),
4729 sizeof(SQL_DATE_STRUCT), buffers.indicators[col - 1].data());
4730 break;Lines 4728-4736 4728 SQLBindCol_ptr(hStmt, col, SQL_C_TYPE_DATE, buffers.dateBuffers[col - 1].data(),
4729 sizeof(SQL_DATE_STRUCT), buffers.indicators[col - 1].data());
4730 break;
4731 case SQL_SS_TIME2:
! 4732 ResizeNativeFetchBuffer(buffers.timeBuffers[col - 1], fetchSize, reservedBytes);
4733 ret =
4734 SQLBindCol_ptr(hStmt, col, SQL_C_SS_TIME2, buffers.timeBuffers[col - 1].data(),
4735 sizeof(SQL_SS_TIME2_STRUCT), buffers.indicators[col - 1].data());
4736 break;Lines 4754-4764 4754 ret = SQLBindCol_ptr(hStmt, col, SQL_C_BINARY, buffers.charBuffers[col - 1].data(),
4755 CheckedFetchBufferLength(fetchBufferSize, 1),
4756 buffers.indicators[col - 1].data());
4757 break;
! 4758 }
4759 case SQL_SS_TIMESTAMPOFFSET:
! 4760 ResizeNativeFetchBuffer(buffers.datetimeoffsetBuffers[col - 1], fetchSize,
4761 reservedBytes);
4762 ret = SQLBindCol_ptr(hStmt, col, SQL_C_SS_TIMESTAMPOFFSET,
4763 buffers.datetimeoffsetBuffers[col - 1].data(),
4764 sizeof(DateTimeOffset),Lines 4799-4807 4799 PERF_TIMER("FetchBatchData");
4800 LOG("FetchBatchData: Fetching data in batches");
4801 SQLRETURN ret;
4802 {
! 4803 numRowsFetched = 0;
4804 // Release the GIL during the blocking ODBC fetch
4805 py::gil_scoped_release release;
4806 PERF_TIMER("FetchBatchData::SQLFetchScroll_call");
4807 ret = SQLFetchScroll_ptr(hStmt, SQL_FETCH_NEXT, 0);Lines 5436-5444 5436 FetchStateGuard fetchStateGuard(StatementHandle, messages);
5437
5438 // Bind columns
5439 ret = SQLBindColums(hStmt, buffers, columnNames, numCols, fetchSize, reservedBytes, charCtype,
! 5440 messages);
5441 if (!SQL_SUCCEEDED(ret)) {
5442 LOG("FetchMany_wrap: Error when binding columns - SQLRETURN=%d", ret);
5443 return ret;
5444 }Lines 5576-5584 5576 const size_t offsetCount = CheckedAddSize(batchSize, 1, "Arrow batch size is too large");
5577 const size_t initialVarDataSize =
5578 CheckedMultiplySize(batchSize, 42, "Arrow batch size is too large");
5579 const size_t bitmapSize =
! 5580 CheckedAddSize(batchSize, 7, "Arrow batch size is too large") / 8;
5581 // Fetch narrow char data as SQL_C_CHAR if on Linux/macOS and configured by the user
5582 charCtype = EffectiveCharCtypeForFetch(charCtype, "utf-8");
5583
5584 // An overly large fetch size doesn't seem to help performance.Lines 5606-5614 5606 std::vector<SQLSMALLINT> dataTypes(numCols);
5607 std::vector<bool> columnNullable(numCols);
5608 std::vector<bool> columnVarLen(numCols, false);
5609 std::vector<int64_t> nullCounts(numCols, 0);
! 5610 size_t reservedBytes = 0;
5611
5612 std::vector<std::unique_ptr<ArrowArrayPrivateData>> arrowArrayPrivateData(numCols);
5613 std::vector<std::unique_ptr<ArrowSchemaPrivateData>> arrowSchemaPrivateData(numCols);
5614 for (SQLSMALLINT i = 0; i < numCols; i++) {Lines 5668-5676 5668 arrowColumnProducer->varVal =
5669 AllocateArrowArray<uint64_t>(offsetCount, reservedBytes,
5670 "Arrow offset buffer is too large");
5671 ResizeNativeFetchBuffer(arrowColumnProducer->varData, initialVarDataSize,
! 5672 reservedBytes);
5673 columnVarLen[i] = true;
5674 // start at offset 0
5675 arrowColumnProducer->varVal[0] = 0;
5676 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->varVal.get();Lines 5683-5692 5683 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->uint8Val.get();
5684 break;
5685 case SQL_SMALLINT:
5686 format = "s";
! 5687 arrowColumnProducer->int16Val =
! 5688 AllocateArrowArray<int16_t>(batchSize, reservedBytes,
5689 "Arrow value buffer is too large");
5690 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->int16Val.get();
5691 break;
5692 case SQL_INTEGER:Lines 5697-5706 5697 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->int32Val.get();
5698 break;
5699 case SQL_BIGINT:
5700 format = "l";
! 5701 arrowColumnProducer->int64Val =
! 5702 AllocateArrowArray<int64_t>(batchSize, reservedBytes,
5703 "Arrow value buffer is too large");
5704 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->int64Val.get();
5705 break;
5706 case SQL_REAL:Lines 5712-5721 5712 break;
5713 case SQL_FLOAT:
5714 case SQL_DOUBLE:
5715 format = "g";
! 5716 arrowColumnProducer->float64Val =
! 5717 AllocateArrowArray<double>(batchSize, reservedBytes,
5718 "Arrow value buffer is too large");
5719 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->float64Val.get();
5720 break;
5721 case SQL_DECIMAL:Lines 5737-5747 5737 case SQL_TIMESTAMP:
5738 case SQL_TYPE_TIMESTAMP:
5739 case SQL_DATETIME:
5740 format = "tsu:";
! 5741 arrowColumnProducer->tsMicroVal =
! 5742 AllocateArrowArray<int64_t>(batchSize, reservedBytes,
! 5743 "Arrow value buffer is too large");
5744 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->tsMicroVal.get();
5745 break;
5746 case SQL_SS_TIMESTAMPOFFSET:
5747 format = "tsu:+00:00";Lines 5751-5760 5751 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->tsMicroVal.get();
5752 break;
5753 case SQL_TYPE_DATE:
5754 format = "tdD";
! 5755 arrowColumnProducer->dateVal =
! 5756 AllocateArrowArray<int32_t>(batchSize, reservedBytes,
5757 "Arrow value buffer is too large");
5758 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->dateVal.get();
5759 break;
5760 case SQL_SS_TIME2:Lines 6147-6155 6147 ? buffers.charBuffers[idxCol].size()
6148 : buffers.charBuffers[idxCol].size() /
6149 static_cast<size_t>(fetchSize);
6150 const size_t sourceOffset = CheckedArrowSourceOffset(
! 6151 buffers.charBuffers[idxCol], idxRowSql, sourceStride, dataLen);
6152
6153 std::memcpy(&(*target_vec)[start],
6154 &buffers.charBuffers[idxCol][sourceOffset], dataLen);
6155 }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) {📋 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: 80.9%
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: 91.9%🔗 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
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Bounded UTF-8 sizing can under-report payloads above 4,001 bytes and must be corrected.
Review effort: Lite
Findings: None
Previously missed (1)
In code that hasn't changed since last review
mssql_python/pybind/param_detect.hpp:241
EncodedUnicodeLength stops counting at 4,001 bytes, but DAEColumnSize uses that result to describe SQL_C_CHAR parameters. Any encoded string longer than 4,001 bytes is therefore advertised with a SQL_VARCHAR column size of 4,001; values between 4,002 and 8,000 are under-described, and values over 8,000 do not get the required MAX sentinel. For example, "\u0800" * 3000 encodes to 9,000 UTF-8 bytes while this probe returns 4,001, so the DAE payload metadata can reject or truncate the value. Continue the bounded measurement through the 8,000-byte boundary and return a value above that boundary when it is exceeded.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use the Unicode-safe MAX boundary for VARCHAR data-at-execution metadata and compare the wide fetch regression by row values rather than generated Row identity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Prevent a too-small setinputsizes declaration from truncating later non-DAE text or binary rows after executemany falls back to row execution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reject boolean row counts before conversion, guard empty Arrow character copies, validate bytearray inputs before C API access, and make the DAE regressions compare row values reliably. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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>
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.