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 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

Subrata (subrata-ms) reviewed Oct 8, 2026

View reviewed changes

Comment thread mssql_python/pybind/ddbc_bindings.cpp Outdated

Subrata (subrata-ms) reviewed Oct 8, 2026

View reviewed changes

Comment thread mssql_python/pybind/ddbc_bindings.cpp Outdated

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

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!

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

🔵 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gargsaumya commented Sep 21, 2026 •

edited

Loading