FIX: Validate driver-provided fetch sizes by gargsaumya · Pull Request #803 · microsoft/mssql-python

gargsaumya · 2026-09-21T06:53:15Z

Work Item / Issue Reference

ADO Work Item: AB#47843

Summary

Validate ODBC row counts, indicators, and column-buffer layout before access.
Check streamed fetch growth and Arrow source/destination bounds.
Correct fixed-width buffer lengths and reject malformed wide-character data.

Stacked on #802 so this review contains only driver-provided fetch validation.

…-validation

…nto saumya/native-fetch-validation

…nto saumya/native-fetch-validation # Conflicts: # mssql_python/pybind/ddbc_bindings.cpp

Gaurav Sharma (bewithgaurav)

the driver-provided size validation is well scoped and the normal fetch and arrow paths hold up. I found one suggestion worth incorporating before merge, so requesting changes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🟡 Changes recommended

Successful zero-row fetches can loop indefinitely, and malformed indicators or the global test hook can cause resource and concurrency problems.

Review effort: Balanced
Findings: 2 · 2

Open (4)

Reject successful zero-row fetches to prevent infinite loops · New
Avoid unsynchronized mutation of the global SQLGetData hook · New
Validate data length before growing Arrow destination · New
Check source bounds before allocating from data length · New

What changed in this PR

Adds defensive validation for driver-provided fetch metadata in the native ODBC layer.

Changes:

Validates row counts, indicators, wide-character lengths, and Arrow bounds.
Hardens streamed variable-length fetching and buffer growth.
Adds native regression scenarios and subprocess coverage.

File	Description
`mssql_python/pybind/ddbc_bindings.cpp`	Implements fetch validation and test hooks.
`mssql_python/pybind/ddbc_bindings.h`	Rejects malformed wide-character lengths.
`tests/test_010_pybind_functions.py`	Tests malformed driver responses and streaming progress.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🔵 Needs a closer look

The test hook unsafely replaces a process-wide ODBC function pointer while concurrent fetches may access it without the GIL.

Review effort: Balanced
Findings: 2 · 2

Open (4)

Avoid unsynchronized mutation of the global SQLGetData hook
Reject successful zero-row fetches to prevent infinite loops
Check source bounds before allocating from data length
Validate data length before growing Arrow destination

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…nto copilot/pr803-rebased

Copilot

🟡 Changes recommended

Direct LOB streaming still rejects valid SQL_NO_DATA completion after streamed progress.

1 open finding

Handle SQL_NO_DATA after chunked LOB reads · New

2 resolved since last review

🧠 Review effort: Balanced

Treat SQL_NO_DATA as normal completion only after a direct LOB fetch has returned payload. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…crosoft/mssql-python into copilot/pr803-rebased

Copilot

🔵 Needs a closer look

The broad native ODBC changes require final cross-platform CI and human verification.

0 open findings

1 resolved since last review

Handle SQL_NO_DATA after chunked LOB reads

🧠 Review effort: Balanced

…crosoft/mssql-python into copilot/pr803-rebased

Copilot

🟡 Changes recommended

Reused or zero-initialized indicators can still accept omitted driver outputs and expose stale or incorrect results.

2 open findings

Initialize LOB length to an invalid sentinel · New
Reset indicator slots before each fetch · New

🧠 Review effort: Balanced

Seed direct and bound fetch indicators with invalid sentinels so omitted driver writes cannot expose empty or stale data. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…crosoft/mssql-python into copilot/pr803-rebased

Copilot

🟡 Changes recommended

Several direct-fetch indicators remain initialized unsafely, allowing malformed successful responses to trigger undefined behavior or evade validation.

1 open finding

Initialize dataLen before SQLGetData · New

2 resolved since last review

🧠 Review effort: Balanced

Seed every direct SQLGetData output with the invalid indicator sentinel before driver calls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…crosoft/mssql-python into copilot/pr803-rebased

Copilot

🟡 Changes recommended

Some malformed diagnostic and indicator paths still fail open or silently return None.

3 open findings

Fail closed when diagnostic retrieval fails · New
Reject missing sql_variant fetch indicator · New
Reject SQL_NO_TOTAL for fixed-width columns · New

1 resolved since last review

Initialize dataLen before SQLGetData

🧠 Review effort: Balanced

Reject unreadable truncation diagnostics, missing variant probe indicators, and SQL_NO_TOTAL for fixed-width columns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

The direct decimal path still accepts a missing driver indicator and silently returns None.

0 open findings

3 resolved since last review

🧠 Review effort: Balanced

Sumit Sarabhai (sumitmsft)

Reviewed the exact current head for correctness, native safety, data integrity, diagnostics, cleanup, Arrow/LOB bounds, test coverage, and the applicable ODBC length/indicator contract.

One Medium-severity issue remains: the directly changed DECIMAL/NUMERIC fetch path can accept an unwritten driver indicator and silently expose malformed non-NULL output as Python None. The remaining reviewed fetch paths had no actionable findings.

Recommendation: Request changes.

Sumit Sarabhai (sumitmsft) · 2026-10-19T10:54:28Z

+                    if (indicator < 0) {
+                        ThrowStdException("Unexpected negative data length");
+                    }
+                    ValidateDecimalDataLength(static_cast<uint64_t>(indicator));


This new decimal length check still accepts an unwritten indicator because indicator starts at 0. A successful driver call that leaves it untouched bypasses both guards; the empty zero-initialized buffer then fails Decimal parsing and is silently converted to None. Initialize it with INVALID_FETCH_INDICATOR, reject the sentinel (and zero for a non-NULL decimal) before parsing, and add missing/zero-indicator tests so malformed driver output cannot become SQL NULL.

gargsaumya · 2026-10-20T10:32:24Z

Copilot resolve the merge conflicts in this pull request

gargsaumya added 3 commits September 21, 2026 12:17

FIX: Validate driver-provided fetch sizes

6be230a

Merge branch 'saumya/native-size-validation' into saumya/native-fetch…

c58b9c2

…-validation

FIX: Unify checked size helpers

d085633

github-actions Bot added the pr-size: medium Moderate update size label Sep 21, 2026

gargsaumya added 5 commits September 21, 2026 12:58

Merge remote-tracking branch 'origin/saumya/native-size-validation' i…

8f69db5

…nto saumya/native-fetch-validation

Merge remote-tracking branch 'origin/saumya/native-size-validation' i…

f178c60

…nto saumya/native-fetch-validation # Conflicts: # mssql_python/pybind/ddbc_bindings.cpp

FIX: Validate driver-provided fetch sizes

b2c89b8

FIX: Preserve stacked fetch validation

d40dc89

Merge remote-tracking branch 'origin/saumya/native-size-validation' i…

522aebd

…nto saumya/native-fetch-validation # Conflicts: # mssql_python/pybind/ddbc_bindings.cpp

Sumit Sarabhai (sumitmsft) reviewed Sep 23, 2026

View reviewed changes

Comment thread mssql_python/pybind/ddbc_bindings.cpp Outdated

Gaurav Sharma (bewithgaurav) requested changes Sep 24, 2026

View reviewed changes

Comment thread mssql_python/pybind/ddbc_bindings.cpp Outdated

Comment thread mssql_python/pybind/ddbc_bindings.cpp Outdated

gargsaumya and others added 2 commits October 7, 2026 11:41

FIX: Validate driver-provided fetch sizes

d9ebebb

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Merge prior native fetch validation history

a4de9e3

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions Bot added pr-size: large Substantial code update and removed pr-size: medium Moderate update size labels Oct 7, 2026

Merge updated native size validation base

95d53ef

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions Bot added pr-size: medium Moderate update size and removed pr-size: large Substantial code update labels Oct 7, 2026

gargsaumya and others added 2 commits October 7, 2026 11:46

Merge main-updated native size validation base

319bcfe

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Merge peak-safe native size validation base

12d688f

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

gargsaumya requested a balanced review from Copilot October 7, 2026 06:25

Copilot started reviewing on behalf of gargsaumya October 7, 2026 06:26

View session

Copilot AI reviewed Oct 7, 2026

View reviewed changes

Comment thread mssql_python/pybind/ddbc_bindings.cpp

Comment thread mssql_python/pybind/ddbc_bindings.cpp Outdated

Comment thread mssql_python/pybind/ddbc_bindings.cpp Outdated

Comment thread mssql_python/pybind/ddbc_bindings.cpp

Merge bounded LOB native size base

6e1efed

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

gargsaumya requested a balanced review from Copilot October 7, 2026 06:31

Copilot started reviewing on behalf of gargsaumya October 7, 2026 06:31

View session

Copilot AI reviewed Oct 7, 2026

View reviewed changes

gargsaumya and others added 2 commits October 7, 2026 12:08

FIX: Harden malformed fetch handling

b441a0d

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Merge remote-tracking branch 'origin/saumya/native-size-validation' i…

6936472

…nto copilot/pr803-rebased

gargsaumya requested a balanced review from Copilot October 7, 2026 06:41

gargsaumya requested a balanced review from Copilot October 12, 2026 05:02

Copilot started reviewing on behalf of gargsaumya October 12, 2026 05:03

View session

Copilot AI reviewed Oct 12, 2026

View reviewed changes

Comment thread mssql_python/pybind/ddbc_bindings.cpp

gargsaumya and others added 2 commits October 12, 2026 12:25

FIX: accept SQL_NO_DATA after LOB progress

9e54e19

Treat SQL_NO_DATA as normal completion only after a direct LOB fetch has returned payload. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Merge branch 'saumya/native-size-validation' of https://github.com/mi…

f8c8ce5

…crosoft/mssql-python into copilot/pr803-rebased

gargsaumya requested a balanced review from Copilot October 12, 2026 06:58

Copilot started reviewing on behalf of gargsaumya October 12, 2026 06:59

View session

Merge branch 'saumya/native-size-validation' of https://github.com/mi…

db79491

…crosoft/mssql-python into copilot/pr803-rebased

Copilot AI reviewed Oct 12, 2026

View reviewed changes

Merge branch 'saumya/native-size-validation' of https://github.com/mi…

4bceee4

…crosoft/mssql-python into copilot/pr803-rebased

gargsaumya requested a balanced review from Copilot October 12, 2026 07:10

Copilot started reviewing on behalf of gargsaumya October 12, 2026 07:11

View session

Copilot AI reviewed Oct 12, 2026

View reviewed changes

Comment thread mssql_python/pybind/ddbc_bindings.cpp

Comment thread mssql_python/pybind/ddbc_bindings.cpp

gargsaumya and others added 2 commits October 12, 2026 12:48

FIX: invalidate driver-written indicators

153016a

Seed direct and bound fetch indicators with invalid sentinels so omitted driver writes cannot expose empty or stale data. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Merge branch 'saumya/native-size-validation' of https://github.com/mi…

cef1fe0

…crosoft/mssql-python into copilot/pr803-rebased

gargsaumya requested a balanced review from Copilot October 12, 2026 07:21

Copilot started reviewing on behalf of gargsaumya October 12, 2026 07:21

View session

Copilot AI reviewed Oct 12, 2026

View reviewed changes

Comment thread mssql_python/pybind/ddbc_bindings.cpp

gargsaumya and others added 2 commits October 12, 2026 15:03

FIX: initialize direct fetch indicators

b4ceb6b

Seed every direct SQLGetData output with the invalid indicator sentinel before driver calls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Merge branch 'saumya/native-size-validation' of https://github.com/mi…

200193a

…crosoft/mssql-python into copilot/pr803-rebased

gargsaumya requested a balanced review from Copilot October 12, 2026 09:35

Copilot started reviewing on behalf of gargsaumya October 12, 2026 09:36

View session

Copilot AI reviewed Oct 12, 2026

View reviewed changes

Comment thread mssql_python/pybind/ddbc_bindings.cpp Outdated

Comment thread mssql_python/pybind/ddbc_bindings.cpp

Comment thread mssql_python/pybind/ddbc_bindings.cpp Outdated

FIX: fail closed on malformed fetch metadata

58ae087

Reject unreadable truncation diagnostics, missing variant probe indicators, and SQL_NO_TOTAL for fixed-width columns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

gargsaumya requested a balanced review from Copilot October 12, 2026 09:45

Copilot started reviewing on behalf of gargsaumya October 12, 2026 09:46

View session

Copilot AI reviewed Oct 12, 2026

View reviewed changes

Sumit Sarabhai (sumitmsft) requested changes Oct 19, 2026

View reviewed changes

Copilot started work on behalf of gargsaumya October 20, 2026 10:32

View session

Conversation

gargsaumya commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Work Item / Issue Reference

Summary

Uh oh!

Uh oh!

Gaurav Sharma (bewithgaurav) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Sumit Sarabhai (sumitmsft) left a comment

Choose a reason for hiding this comment

Uh oh!

Sumit Sarabhai (sumitmsft) Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

gargsaumya commented Oct 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gargsaumya commented Sep 21, 2026 •

edited

Loading