FIX: Validate caller-controlled native buffer sizes - #802
Open
gargsaumya wants to merge 5 commits into
Open
gargsaumya wants to merge 5 commits into
gargsaumya wants to merge 5 commits into
Conversation
PR Performance ReportNo consistent slowdowns detected across all 2 environments. Coverage: 2 of 2 environments completed. Advisory result; does not block merging.
Affected phases and call countsPhase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed. No affected phases or call-count changes were recorded. All database tasks and timingsUnix / SQL Server 2022
Unix / SQL Server 2025
Build, commits 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/pybind/ddbc_bindings.cppLines 322-331 322
323 template <typename ParamType>
324 ParamType* AllocateParamBufferArray(std::vector<std::shared_ptr<void>>& paramBuffers,
325 size_t count) {
! 326 if (count > std::numeric_limits<size_t>::max() / sizeof(ParamType)) {
! 327 ThrowStdException("Parameter buffer size is too large");
328 }
329 std::shared_ptr<ParamType> buffer(new ParamType[count], std::default_delete<ParamType[]>());
330 ParamType* raw = buffer.get();
331 paramBuffers.push_back(buffer);Lines 332-341 332 return raw;
333 }
334
335 size_t CheckedAddSize(size_t left, size_t right, const char* errorMessage) {
! 336 if (left > std::numeric_limits<size_t>::max() - right) {
! 337 ThrowStdException(errorMessage);
338 }
339 return left + right;
340 }Lines 339-348 339 return left + right;
340 }
341
342 size_t CheckedMultiplySize(size_t left, size_t right, const char* errorMessage) {
! 343 if (left != 0 && right > std::numeric_limits<size_t>::max() / left) {
! 344 ThrowStdException(errorMessage);
345 }
346 return left * right;
347 }Lines 358-367 358 }
359
360 template <typename ElementType>
361 std::unique_ptr<ElementType[]> AllocateUniqueArray(size_t count, const char* errorMessage) {
! 362 if (count > std::numeric_limits<size_t>::max() / sizeof(ElementType)) {
! 363 ThrowStdException(errorMessage);
364 }
365 return std::make_unique<ElementType[]>(count);
366 }Lines 402-410 402 const char* errorMessage) {
403 ReserveNativeFetchBytes(reservedBytes, count, sizeof(ElementType));
404 return AllocateUniqueArray<ElementType>(count, errorMessage);
405 }
! 406
407 std::string DescribeChar(unsigned char ch) {
408 if (ch >= 32 && ch <= 126) {
409 return std::string("'") + static_cast<char>(ch) + "'";
410 } else {Lines 2273-2282 2273 LOG("BindParameterArray: Starting column-wise array binding - "
2274 "param_count=%zu, param_set_size=%zu",
2275 columnwise_params.size(), paramSetSize);
2276 if (columnwise_params.size() != paramInfos.size()) {
! 2277 ThrowStdException("Parameter count does not match parameter metadata count");
! 2278 }
2279
2280 std::vector<std::shared_ptr<void>> tempBuffers;
2281
2282 try {Lines 2344-2353 2344 "param_index=%d, count=%zu, column_size=%zu",
2345 paramIndex, paramSetSize, info.columnSize);
2346 const size_t elementWidth = CheckedAddSize(
2347 info.columnSize, 1, "Wide-character parameter size is too large");
! 2348 const size_t bufferBytes = CheckedMultiplySize(
! 2349 elementWidth, sizeof(SQLWCHAR),
2350 "Wide-character parameter length is too large");
2351 if (bufferBytes > static_cast<size_t>(std::numeric_limits<SQLLEN>::max())) {
2352 ThrowStdException("Wide-character parameter length is too large");
2353 }Lines 2378-2386 2378 LOG("BindParameterArray: SQL_C_WCHAR bound - "
2379 "param_index=%d",
2380 paramIndex);
2381 dataPtr = wcharArray;
! 2382 bufferLength = static_cast<SQLLEN>(bufferBytes);
2383 break;
2384 }
2385 case SQL_C_TINYINT:
2386 case SQL_C_UTINYINT: {Lines 2449-2458 2449 case SQL_C_BINARY: {
2450 LOG("BindParameterArray: Binding SQL_C_CHAR/BINARY array - "
2451 "param_index=%d, count=%zu, column_size=%zu, encoding='%s'",
2452 paramIndex, paramSetSize, info.columnSize, charEncoding.c_str());
! 2453 const size_t elementWidth = CheckedAddSize(
! 2454 info.columnSize, 1, "Character parameter size is too large");
2455 if (elementWidth > static_cast<size_t>(std::numeric_limits<SQLLEN>::max())) {
2456 ThrowStdException("Character parameter length is too large");
2457 }
2458 char* charArray = AllocateParamBufferArray<char>(Lines 2467-2476 2467 info.columnSize + 1);
2468 } else {
2469 if (info.paramCType == SQL_C_BINARY &&
2470 !py::isinstance<py::bytes>(columnValues[i]) &&
! 2471 !py::isinstance<py::bytearray>(columnValues[i])) {
! 2472 ThrowStdException(MakeParamMismatchErrorStr(info.paramCType,
2473 paramIndex));
2474 }
2475 std::string encodedStr;Lines 2516-2524 2516 LOG("BindParameterArray: SQL_C_CHAR/BINARY bound - "
2517 "param_index=%d",
2518 paramIndex);
2519 dataPtr = charArray;
! 2520 bufferLength = static_cast<SQLLEN>(elementWidth);
2521 break;
2522 }
2523 case SQL_C_BIT: {
2524 LOG("BindParameterArray: Binding SQL_C_BIT array - "Lines 4155-4163 4155 uint64_t fetchBufferSize = columnSize + 1 /*null-terminator*/;
4156 ResizeNativeFetchBuffer(buffers.wcharBuffers[col - 1],
4157 CheckedMultiplySize(fetchSize, fetchBufferSize,
4158 "Native fetch buffer is too large"),
! 4159 reservedBytes);
4160 ret = SQLBindCol_ptr(hStmt, col, SQL_C_WCHAR, buffers.wcharBuffers[col - 1].data(),
4161 fetchBufferSize * sizeof(SQLWCHAR),
4162 buffers.indicators[col - 1].data());
4163 break;Lines 4167-4175 4167 ret = SQLBindCol_ptr(hStmt, col, SQL_C_SLONG, buffers.intBuffers[col - 1].data(),
4168 sizeof(SQLINTEGER), buffers.indicators[col - 1].data());
4169 break;
4170 case SQL_SMALLINT:
! 4171 ResizeNativeFetchBuffer(buffers.smallIntBuffers[col - 1], fetchSize, reservedBytes);
4172 ret = SQLBindCol_ptr(hStmt, col, SQL_C_SSHORT,
4173 buffers.smallIntBuffers[col - 1].data(), sizeof(SQLSMALLINT),
4174 buffers.indicators[col - 1].data());
4175 break;Lines 4189-4197 4189 sizeof(SQLREAL), buffers.indicators[col - 1].data());
4190 break;
4191 case SQL_DECIMAL:
4192 case SQL_NUMERIC:
! 4193 ResizeNativeFetchBuffer(
4194 buffers.charBuffers[col - 1],
4195 CheckedMultiplySize(fetchSize, MAX_DIGITS_IN_NUMERIC,
4196 "Native fetch buffer is too large"),
4197 reservedBytes);Lines 4209-4217 4209 case SQL_TIMESTAMP:
4210 case SQL_TYPE_TIMESTAMP:
4211 case SQL_DATETIME:
4212 ResizeNativeFetchBuffer(buffers.timestampBuffers[col - 1], fetchSize,
! 4213 reservedBytes);
4214 ret = SQLBindCol_ptr(
4215 hStmt, col, SQL_C_TYPE_TIMESTAMP, buffers.timestampBuffers[col - 1].data(),
4216 sizeof(SQL_TIMESTAMP_STRUCT), buffers.indicators[col - 1].data());
4217 break;Lines 4244-4255 4244 case SQL_LONGVARBINARY:
4245 // TODO: handle variable length data correctly. This logic wont
4246 // suffice
4247 HandleZeroColumnSizeAtFetch(columnSize);
! 4248 ResizeNativeFetchBuffer(buffers.charBuffers[col - 1],
4249 CheckedMultiplySize(fetchSize, columnSize,
4250 "Native fetch buffer is too large"),
! 4251 reservedBytes);
4252 ret = SQLBindCol_ptr(hStmt, col, SQL_C_BINARY, buffers.charBuffers[col - 1].data(),
4253 columnSize, buffers.indicators[col - 1].data());
4254 break;
4255 case SQL_SS_TIMESTAMPOFFSET:Lines 4887-4895 4887 }
4888
4889 // Ensure initial buffer has space for at least the null terminator
4890 if (dataVec.size() < sizeNullTerminator) {
! 4891 ResizeNativeFetchBuffer(dataVec, sizeNullTerminator, reservedBytes);
4892 }
4893
4894 while (true) {
4895 SQLLEN localInd = 0;Lines 4923-4932 4923 if (ret == SQL_SUCCESS_WITH_INFO) {
4924 // Determine how much more space we need
4925 if (localInd < 0) {
4926 // SQL_NO_TOTAL: driver doesn't know total size, double the buffer
! 4927 end = CheckedMultiplySize(dataVec.size(), 2,
! 4928 "Native fetch buffer size is too large");
4929 } else {
4930 // Driver returned total size: allocate exactly what we need
4931 assert(localInd % sizeof(T) == 0);
4932 end = CheckedAddSize(Lines 4985-4995 4985 int arrowBatchSize,
4986 int charCtype) {
4987 PERF_TIMER("FetchArrowBatch_wrap");
4988 ValidateNativeRowCount(arrowBatchSize, "Arrow batch size", true);
! 4989 const size_t batchSize = static_cast<size_t>(arrowBatchSize);
! 4990 const size_t offsetCount = CheckedAddSize(batchSize, 1, "Arrow batch size is too large");
! 4991 const size_t initialVarDataSize =
4992 CheckedMultiplySize(batchSize, 42, "Arrow batch size is too large");
4993 const size_t bitmapSize =
4994 CheckedAddSize(batchSize, 7, "Arrow batch size is too large") / 8;
4995 // Fetch narrow char data as SQL_C_CHAR if on Linux/macOS and configured by the userLines 5083-5091 5083 arrowColumnProducer->varVal =
5084 AllocateArrowArray<uint64_t>(offsetCount, reservedBytes,
5085 "Arrow offset buffer is too large");
5086 ResizeNativeFetchBuffer(arrowColumnProducer->varData, initialVarDataSize,
! 5087 reservedBytes);
5088 columnVarLen[i] = true;
5089 // start at offset 0
5090 arrowColumnProducer->varVal[0] = 0;
5091 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->varVal.get();Lines 5098-5107 5098 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->uint8Val.get();
5099 break;
5100 case SQL_SMALLINT:
5101 format = "s";
! 5102 arrowColumnProducer->int16Val =
! 5103 AllocateArrowArray<int16_t>(batchSize, reservedBytes,
5104 "Arrow value buffer is too large");
5105 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->int16Val.get();
5106 break;
5107 case SQL_INTEGER:Lines 5121-5129 5121 case SQL_REAL:
5122 format = "f";
5123 arrowColumnProducer->float32Val =
5124 AllocateArrowArray<float>(batchSize, reservedBytes,
! 5125 "Arrow value buffer is too large");
5126 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->float32Val.get();
5127 break;
5128 case SQL_FLOAT:
5129 case SQL_DOUBLE:Lines 5144-5152 5144 std::memcpy(arrowSchemaPrivateData[i]->format.get(), formatStr.c_str(), formatLen);
5145 format = arrowSchemaPrivateData[i]->format.get();
5146 arrowColumnProducer->decimalVal =
5147 AllocateArrowArray<Int128_t>(batchSize, reservedBytes,
! 5148 "Arrow value buffer is too large");
5149 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->decimalVal.get();
5150 break;
5151 }
5152 case SQL_TIMESTAMP:Lines 5166-5175 5166 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->tsMicroVal.get();
5167 break;
5168 case SQL_TYPE_DATE:
5169 format = "tdD";
! 5170 arrowColumnProducer->dateVal =
! 5171 AllocateArrowArray<int32_t>(batchSize, reservedBytes,
5172 "Arrow value buffer is too large");
5173 arrowColumnProducer->ptrValueBuffer = arrowColumnProducer->dateVal.get();
5174 break;
5175 case SQL_SS_TIME2:Lines 5207-5215 5207
5208 arrowColumnProducer->valid =
5209 AllocateArrowArray<uint8_t>(bitmapSize, reservedBytes, "Arrow bitmap is too large");
5210 // Initialize validity bitmap to all valid
! 5211 std::memset(arrowColumnProducer->valid.get(), 0xFF, bitmapSize);
5212 }
5213
5214 // Initialize column buffers
5215 ReserveNativeFetchBytes(📋 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: 64.1%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 79.2%
mssql_python.pybind.connection.connection_pool.cpp: 82.3%
mssql_python.pybind.connection.connection.cpp: 82.5%
mssql_python.row.py: 83.4%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%🔗 Quick Links
|
…nto saumya/native-size-validation
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.