Repository navigation
FIX: Validate caller-controlled native buffer sizes - #802
Open
gargsaumya wants to merge 36 commits into
Open
gargsaumya wants to merge 36 commits into
gargsaumya wants to merge 36 commits into
Conversation
PR Performance Report🔍 Performance needs review1 database task produced inconsistent slowdown signals across 1 measured environment. 0 IMPROVEMENTS 0 SLOWDOWNS 2/2 ENVIRONMENTS 1 INCONSISTENT SLOWDOWN Signal fingerprint
Coverage: 2 of 2 environments completed. Advisory result; does not block merging. Measured timings
Performance diagnosticsPhase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed. Unix / SQL Server 2025Insertion with explicit input sizes: ddbc::BindParameters +2.387 ms; py::execute::cpp_call +1.154 ms; ddbc::BindParameters::SQLBindParameter_call +1.112 ms. All database tasks and timingsUnix / SQL Server 2022
Unix / SQL Server 2025
Build and measurement detailsPR head:
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 |
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 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 615-625 615
616 static SQLRETURN StreamDAEParameter(SQLHSTMT hStmt, const ParamInfo& info,
617 const std::string& charEncoding) {
618 PyObject* value = info.dataPtr.ptr();
! 619 if (!value || value == Py_None) {
! 620 py::gil_scoped_release release;
! 621 return SQLPutData_ptr(hStmt, nullptr, 0);
622 }
623
624 auto putImmutableData = [&](SQLPOINTER data, SQLLEN length) {
625 py::gil_scoped_release release;Lines 627-639 627 };
628 if (PyUnicode_Check(value)) {
629 if (info.paramCType == SQL_C_WCHAR) {
630 return stream_unicode_dae_chunks(value, "utf-16-le", putImmutableData);
! 631 }
! 632 if (info.paramCType == SQL_C_CHAR) {
! 633 return stream_unicode_dae_chunks(value, charEncoding, putImmutableData);
! 634 }
! 635 ThrowStdException("DAE only supports text C types for str values");
636 }
637 if (PyBytes_Check(value)) {
638 return stream_dae_chunks(PyBytes_AS_STRING(value),
639 static_cast<size_t>(PyBytes_GET_SIZE(value)),Lines 645-654 645 std::vector<char> chunk(std::min(static_cast<size_t>(DAE_CHUNK_SIZE), totalBytes));
646 for (size_t offset = 0; offset < totalBytes; offset += chunk.size()) {
647 const size_t currentSize = static_cast<size_t>(PyByteArray_GET_SIZE(value));
648 const size_t length = std::min(chunk.size(), totalBytes - offset);
! 649 if (currentSize < offset + length) {
! 650 ThrowStdException("bytearray changed size during DAE streaming");
651 }
652 std::copy_n(PyByteArray_AS_STRING(value) + offset, length, chunk.data());
653 SQLRETURN rc = putImmutableData(chunk.data(), static_cast<SQLLEN>(length));
654 if (!SQL_SUCCEEDED(rc)) return rc;Lines 653-662 653 SQLRETURN rc = putImmutableData(chunk.data(), static_cast<SQLLEN>(length));
654 if (!SQL_SUCCEEDED(rc)) return rc;
655 }
656 return SQL_SUCCESS;
! 657 }
! 658 ThrowStdException("DAE only supports str, bytes, or bytearray values");
659 return SQL_ERROR;
660 }
661
662 // GH-610: Resolve SQL type for a NULL parameter using per-handle cache.Lines 2555-2566 2555 SQLRETURN describeRc = PreResolveUdtTypes(hStmt, paramInfos);
2556 if (!SQL_SUCCEEDED(describeRc)) return describeRc;
2557 // GH-627: resolve unknown NULL array param SQL types before binding any param.
2558 PreResolveUnknownNullTypes(handle, hStmt, paramInfos);
! 2559 size_t reservedParameterBytes = 0;
! 2560 for (const ParamInfo& info : paramInfos) {
! 2561 ReserveNativeParameterBytes(reservedParameterBytes, paramSetSize,
! 2562 ParameterArrayElementSize(info));
2563 ReserveNativeParameterBytes(reservedParameterBytes, paramSetSize, sizeof(SQLLEN));
2564 }
2565 for (int paramIndex = 0; paramIndex < columnwise_params.size(); ++paramIndex) {
2566 const py::list& columnValues = columnwise_params[paramIndex].cast<py::list>();Lines 2629-2640 2629 elementWidth, sizeof(SQLWCHAR),
2630 "Wide-character parameter length is too large");
2631 if (bufferBytes > static_cast<size_t>(std::numeric_limits<SQLLEN>::max())) {
2632 ThrowStdException("Wide-character parameter length is too large");
! 2633 }
2634 SQLWCHAR* wcharArray = AllocateParamBufferArray<SQLWCHAR>(
! 2635 tempBuffers,
! 2636 CheckedMultiplySize(paramSetSize, elementWidth,
2637 "Wide-character parameter buffer is too large"));
2638 strLenOrIndArray = AllocateParamBufferArray<SQLLEN>(tempBuffers, paramSetSize);
2639 for (size_t i = 0; i < paramSetSize; ++i) {
2640 if (columnValues[i].is_none()) {Lines 2683-2691 2683 LOG("BindParameterArray: SQL_C_WCHAR bound - "
2684 "param_index=%d",
2685 paramIndex);
2686 dataPtr = wcharArray;
! 2687 bufferLength = static_cast<SQLLEN>(bufferBytes);
2688 break;
2689 }
2690 case SQL_C_TINYINT:
2691 case SQL_C_UTINYINT: {Lines 2754-2766 2754 case SQL_C_BINARY: {
2755 LOG("BindParameterArray: Binding SQL_C_CHAR/BINARY array - "
2756 "param_index=%d, count=%zu, column_size=%zu, encoding='%s'",
2757 paramIndex, paramSetSize, info.columnSize, charEncoding.c_str());
! 2758 const size_t elementWidth = CheckedAddSize(
! 2759 info.columnSize, 1, "Character parameter size is too large");
! 2760 if (elementWidth > static_cast<size_t>(std::numeric_limits<SQLLEN>::max())) {
! 2761 ThrowStdException("Character parameter length is too large");
! 2762 }
2763 char* charArray = AllocateParamBufferArray<char>(
2764 tempBuffers,
2765 CheckedMultiplySize(paramSetSize, elementWidth,
2766 "Character parameter buffer is too large"));Lines 2770-2778 2770 strLenOrIndArray[i] = SQL_NULL_DATA;
2771 std::memset(charArray + i * (info.columnSize + 1), 0,
2772 info.columnSize + 1);
2773 } else {
! 2774 if (info.paramCType == SQL_C_BINARY &&
2775 !py::isinstance<py::bytes>(columnValues[i]) &&
2776 !py::isinstance<py::bytearray>(columnValues[i])) {
2777 ThrowStdException(MakeParamMismatchErrorStr(info.paramCType,
2778 paramIndex));Lines 2787-2796 2787 encoded =
2788 columnValues[i].attr("encode")(charEncoding, "strict");
2789 if (PyBytes_AsStringAndSize(encoded.ptr(), &encodedData,
2790 &encodedSize) != 0) {
! 2791 throw py::error_already_set();
! 2792 }
2793 LOG("BindParameterArray: param[%d] row[%zu] SQL_C_CHAR - "
2794 "Encoded with '%s', "
2795 "size=%zu bytes",
2796 paramIndex, i, charEncoding.c_str(),Lines 2821-2839 2821 ThrowStdException(
2822 MakeParamMismatchErrorStr(info.paramCType, paramIndex));
2823 }
2824
! 2825 const size_t dataSize = static_cast<size_t>(encodedSize);
! 2826 if (dataSize > info.columnSize) {
2827 LOG("BindParameterArray: String/binary too "
2828 "long - param_index=%d, row=%zu, size=%zu, "
2829 "max=%zu",
! 2830 paramIndex, i, dataSize, info.columnSize);
2831 ThrowStdException("Input exceeds column size at index " +
2832 std::to_string(i));
2833 }
! 2834 std::copy_n(encodedData, dataSize, charArray + i * elementWidth);
! 2835 strLenOrIndArray[i] = static_cast<SQLLEN>(dataSize);
2836 }
2837 }
2838 LOG("BindParameterArray: SQL_C_CHAR/BINARY bound - "
2839 "param_index=%d",Lines 2838-2846 2838 LOG("BindParameterArray: SQL_C_CHAR/BINARY bound - "
2839 "param_index=%d",
2840 paramIndex);
2841 dataPtr = charArray;
! 2842 bufferLength = static_cast<SQLLEN>(elementWidth);
2843 break;
2844 }
2845 case SQL_C_BIT: {
2846 LOG("BindParameterArray: Binding SQL_C_BIT array - "Lines 3346-3355 3346
3347 std::vector<ParamInfo> rowParamInfos = paramInfos;
3348 for (size_t paramIndex = 0; paramIndex < rowParamInfos.size(); ++paramIndex) {
3349 if (rowParams[paramIndex].is_none()) {
! 3350 rowParamInfos[paramIndex].paramCType = SQL_C_DEFAULT;
! 3351 rowParamInfos[paramIndex].isDAE = false;
3352 rowParamInfos[paramIndex].dataPtr = py::none();
3353 }
3354 }
3355 std::vector<std::shared_ptr<void>> paramBuffers;Lines 3406-3419 3406 return rc;
3407 }
3408
3409 const SQLRETURN rowRc = rc;
! 3410 rc = SQLFreeStmt_ptr(hStmt, SQL_RESET_PARAMS);
! 3411 if (!SQL_SUCCEEDED(rc)) {
! 3412 LOG("SQLExecuteMany: SQL_RESET_PARAMS failed for row %zu - "
! 3413 "rc=%d",
! 3414 rowIndex, rc);
! 3415 return rc;
3416 }
3417 rc = rowRc;
3418 }
3419 LOG("SQLExecuteMany: All DAE rows processed successfully - "Lines 3671-3681 3671 LOG("FetchLobColumnData: SQL_SUCCESS - no more data at loop %d", loopCount);
3672 break;
3673 }
3674 }
! 3675 LOG("FetchLobColumnData: Total bytes collected=%zu for column %d", dataSize, colIndex);
3676
! 3677 if (dataSize == 0) {
3678 if (isBinary) {
3679 return py::bytes("");
3680 }
3681 return py::str("");Lines 4716-4724 4716 ret = SQLBindCol_ptr(hStmt, col, SQL_C_TINYINT, buffers.charBuffers[col - 1].data(),
4717 sizeof(SQLCHAR), buffers.indicators[col - 1].data());
4718 break;
4719 case SQL_BIT:
! 4720 ResizeNativeFetchBuffer(buffers.charBuffers[col - 1], fetchSize, reservedBytes);
4721 ret = SQLBindCol_ptr(hStmt, col, SQL_C_BIT, buffers.charBuffers[col - 1].data(),
4722 sizeof(SQLCHAR), buffers.indicators[col - 1].data());
4723 break;
4724 case SQL_REAL:Lines 4738-4746 4738 buffers.indicators[col - 1].data());
4739 break;
4740 case SQL_DOUBLE:
4741 case SQL_FLOAT:
! 4742 ResizeNativeFetchBuffer(buffers.doubleBuffers[col - 1], fetchSize, reservedBytes);
4743 ret =
4744 SQLBindCol_ptr(hStmt, col, SQL_C_DOUBLE, buffers.doubleBuffers[col - 1].data(),
4745 sizeof(SQLDOUBLE), buffers.indicators[col - 1].data());
4746 break;Lines 4759-4767 4759 SQLBindCol_ptr(hStmt, col, SQL_C_SBIGINT, buffers.bigIntBuffers[col - 1].data(),
4760 sizeof(SQLBIGINT), buffers.indicators[col - 1].data());
4761 break;
4762 case SQL_TYPE_DATE:
! 4763 ResizeNativeFetchBuffer(buffers.dateBuffers[col - 1], fetchSize, reservedBytes);
4764 ret =
4765 SQLBindCol_ptr(hStmt, col, SQL_C_TYPE_DATE, buffers.dateBuffers[col - 1].data(),
4766 sizeof(SQL_DATE_STRUCT), buffers.indicators[col - 1].data());
4767 break;Lines 4765-4773 4765 SQLBindCol_ptr(hStmt, col, SQL_C_TYPE_DATE, buffers.dateBuffers[col - 1].data(),
4766 sizeof(SQL_DATE_STRUCT), buffers.indicators[col - 1].data());
4767 break;
4768 case SQL_SS_TIME2:
! 4769 ResizeNativeFetchBuffer(buffers.timeBuffers[col - 1], fetchSize, reservedBytes);
4770 ret =
4771 SQLBindCol_ptr(hStmt, col, SQL_C_SS_TIME2, buffers.timeBuffers[col - 1].data(),
4772 sizeof(SQL_SS_TIME2_STRUCT), buffers.indicators[col - 1].data());
4773 break;Lines 4791-4801 4791 ret = SQLBindCol_ptr(hStmt, col, SQL_C_BINARY, buffers.charBuffers[col - 1].data(),
4792 CheckedFetchBufferLength(fetchBufferSize, 1),
4793 buffers.indicators[col - 1].data());
4794 break;
! 4795 }
4796 case SQL_SS_TIMESTAMPOFFSET:
! 4797 ResizeNativeFetchBuffer(buffers.datetimeoffsetBuffers[col - 1], fetchSize,
4798 reservedBytes);
4799 ret = SQLBindCol_ptr(hStmt, col, SQL_C_SS_TIMESTAMPOFFSET,
4800 buffers.datetimeoffsetBuffers[col - 1].data(),
4801 sizeof(DateTimeOffset),Lines 4836-4844 4836 PERF_TIMER("FetchBatchData");
4837 LOG("FetchBatchData: Fetching data in batches");
4838 SQLRETURN ret;
4839 {
! 4840 numRowsFetched = 0;
4841 // Release the GIL during the blocking ODBC fetch
4842 py::gil_scoped_release release;
4843 PERF_TIMER("FetchBatchData::SQLFetchScroll_call");
4844 ret = SQLFetchScroll_ptr(hStmt, SQL_FETCH_NEXT, 0);Lines 5473-5481 5473 FetchStateGuard fetchStateGuard(StatementHandle, messages);
5474
5475 // Bind columns
5476 ret = SQLBindColums(hStmt, buffers, columnNames, numCols, fetchSize, reservedBytes, charCtype,
! 5477 messages);
5478 if (!SQL_SUCCEEDED(ret)) {
5479 LOG("FetchMany_wrap: Error when binding columns - SQLRETURN=%d", ret);
5480 return ret;
5481 }Lines 5613-5621 5613 const size_t offsetCount = CheckedAddSize(batchSize, 1, "Arrow batch size is too large");
5614 const size_t initialVarDataSize =
5615 CheckedMultiplySize(batchSize, 42, "Arrow batch size is too large");
5616 const size_t bitmapSize =
! 5617 CheckedAddSize(batchSize, 7, "Arrow batch size is too large") / 8;
5618 // Fetch narrow char data as SQL_C_CHAR if on Linux/macOS and configured by the user
5619 charCtype = EffectiveCharCtypeForFetch(charCtype, "utf-8");
5620
5621 // An overly large fetch size doesn't seem to help performance.Lines 5643-5651 5643 std::vector<SQLSMALLINT> dataTypes(numCols);
5644 std::vector<bool> columnNullable(numCols);
5645 std::vector<bool> columnVarLen(numCols, false);
5646 std::vector<int64_t> nullCounts(numCols, 0);
! 5647 size_t reservedBytes = 0;
5648
5649 std::vector<std::unique_ptr<ArrowArrayPrivateData>> arrowArrayPrivateData(numCols);
5650 std::vector<std::unique_ptr<ArrowSchemaPrivateData>> arrowSchemaPrivateData(numCols);
5651 for (SQLSMALLINT i = 0; i < numCols; i++) {Lines 5705-5713 5705 arrowColumnProducer->varVal =
5706 AllocateArrowArray<uint64_t>(offsetCount, reservedBytes,
5707 "Arrow offset buffer is too large");
5708 ResizeNativeFetchBuffer(arrowColumnProducer->varData, initialVarDataSize,
! 5709 reservedBytes);
5710 columnVarLen[i] = true;
5711 // start at offset 0
5712 arrowColumnProducer->varVal[0] = 0;
5713 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->varVal.get();Lines 5720-5729 5720 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->uint8Val.get();
5721 break;
5722 case SQL_SMALLINT:
5723 format = "s";
! 5724 arrowColumnProducer->int16Val =
! 5725 AllocateArrowArray<int16_t>(batchSize, reservedBytes,
5726 "Arrow value buffer is too large");
5727 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->int16Val.get();
5728 break;
5729 case SQL_INTEGER:Lines 5734-5743 5734 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->int32Val.get();
5735 break;
5736 case SQL_BIGINT:
5737 format = "l";
! 5738 arrowColumnProducer->int64Val =
! 5739 AllocateArrowArray<int64_t>(batchSize, reservedBytes,
5740 "Arrow value buffer is too large");
5741 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->int64Val.get();
5742 break;
5743 case SQL_REAL:Lines 5749-5758 5749 break;
5750 case SQL_FLOAT:
5751 case SQL_DOUBLE:
5752 format = "g";
! 5753 arrowColumnProducer->float64Val =
! 5754 AllocateArrowArray<double>(batchSize, reservedBytes,
5755 "Arrow value buffer is too large");
5756 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->float64Val.get();
5757 break;
5758 case SQL_DECIMAL:Lines 5774-5784 5774 case SQL_TIMESTAMP:
5775 case SQL_TYPE_TIMESTAMP:
5776 case SQL_DATETIME:
5777 format = "tsu:";
! 5778 arrowColumnProducer->tsMicroVal =
! 5779 AllocateArrowArray<int64_t>(batchSize, reservedBytes,
! 5780 "Arrow value buffer is too large");
5781 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->tsMicroVal.get();
5782 break;
5783 case SQL_SS_TIMESTAMPOFFSET:
5784 format = "tsu:+00:00";Lines 5788-5797 5788 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->tsMicroVal.get();
5789 break;
5790 case SQL_TYPE_DATE:
5791 format = "tdD";
! 5792 arrowColumnProducer->dateVal =
! 5793 AllocateArrowArray<int32_t>(batchSize, reservedBytes,
5794 "Arrow value buffer is too large");
5795 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->dateVal.get();
5796 break;
5797 case SQL_SS_TIME2:Lines 6184-6192 6184 ? buffers.charBuffers[idxCol].size()
6185 : buffers.charBuffers[idxCol].size() /
6186 static_cast<size_t>(fetchSize);
6187 const size_t sourceOffset = CheckedArrowSourceOffset(
! 6188 buffers.charBuffers[idxCol], idxRowSql, sourceStride, dataLen);
6189
6190 std::memcpy(&(*target_vec)[start],
6191 &buffers.charBuffers[idxCol][sourceOffset], dataLen);
6192 }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 474-483 474 const std::string& charEncoding = "utf-8") {
475 PyTypeCache::initialize();
476
477 if (!PyList_Check(params)) {
! 478 throw py::type_error("params must be a list");
! 479 }
480 ValidateInputSizes(inputSizes);
481
482 const Py_ssize_t n = PyList_GET_SIZE(params);
483 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.1%🔗 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
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>
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.
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.