Guard decimal magnitude reassembly against 128-bit shift overflow

[David Engel (David-Engel)]

Description

DecimalParts::to_decimal_string and to_f64 reassembled the little-endian 32-bit words with acc + ((part as u32 as u128) << (i * 32)). At i == 4 the shift amount equals the full width of the accumulator: debug builds panic with attempt to shift left with overflow (which aborts the process when it unwinds through mssql-odbc's extern "C" boundary), and release builds mask the shift to amount % 128 and silently return a wrong value.

The input was reachable because read_decimal_data capped the word count at 64 rather than 4, so a malformed server payload with 5+ words was accepted at the wire and reached the fold.

Changes:

• Tighten MAX_DECIMAL_INT_PARTS from 64 to 4 and hoist it to a module-level constant. SQL Server's maximum precision of 38 digits fits in 128 bits, and the widest decimal the TDS wire format carries is 17 bytes, so a longer payload is malformed and is now rejected with a ProtocolError. The fuzzing-only cap of 10 is dropped since 4 is stricter.
• Round the word count up rather than down when deriving it from the declared length. (length - 1) >> 2 truncated, so a length of 18–20 still resolved to 4 words and was accepted while leaving 1–3 unread bytes to desynchronize the following field; the same truncation left bytes on the stream for any length whose magnitude did not cover whole words (a length of 7 read one word and dropped 2 bytes). The magnitude is now read as a single zero-padded buffer, so every declared byte is consumed and a trailing partial word is preserved — matching the length-tolerant reader on the Always Encrypted path.
• Replace both folds with a shared DecimalParts::magnitude() that returns None only when the value genuinely does not fit a u128, and uses | instead of + (the words are disjoint, so + was an overflow candidate in its own right). Significance is measured rather than word count, so a zero-padded value like [12345, 0, 0, 0, 0, 0] still converts.
• Fall back to a BigUint over the same words when the value does not fit a u128. DecimalParts has public fields and is constructible from FFI (NapiDecimalParts), so the formatting paths render an oversized value exactly instead of panicking or truncating.
• Reuse magnitude() in mssql-odbc's numeric_source, replacing the duplicate > 4 guard odbc: mandatory source-type conversions for typed SQLGetData (P1a) [AB#47107] #217 added on the typed SQLGetData path (whose comment still referred to the 64-word cap this PR removes). With one shared implementation, SQL_C_CHAR and SQL_C_SLONG can no longer disagree about whether a given value has a numeric interpretation.
• Update the comments in security/encryption/cell.rs and in the fetch_convert.rs test that cited the now-guarded u128 fold, or the old 64-word cap, as their justification. The checks themselves are unchanged and still valid.

Tests added: the widest valid 4-word magnitude; an oversized 5-word magnitude through both to_decimal_string and to_f64; an oversized magnitude whose extra words are zero, asserted through both the string and typed ODBC paths; an empty int_parts; a wire-level 5-word payload that must be rejected; a length-18 payload (one byte past the limit, the boundary that rounding down used to accept); and a length-7 payload asserting the partial trailing word is consumed and the next field reads back intact.

Related Issues

Fixes #234

Checklist

• cargo bfmt passes
• cargo bclippy passes
• cargo btest passes — 1710 mssql-tds and 539 mssql-odbc lib tests pass. The 7 failures in certificate_validator / win_tls::validate are pre-existing and unrelated: they need TLS fixtures that are deliberately not tracked in git (see mssql-tds/tests/test_certificates/README.md).
• New/changed functionality has tests
• Public API changes are documented — DecimalParts::magnitude is newly public, with doc comments explaining the None case

[Copilot]

Pull request overview

Prevents overflow when decoding or formatting oversized decimal magnitudes.

Changes:

• Limits decoded decimal magnitudes to four words.
• Adds safe u128/BigUint magnitude reconstruction.
• Adds boundary and malformed-payload tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
mssql-tds/src/datatypes/decoder.rs	Guards magnitude reconstruction and decoding.
mssql-tds/src/security/encryption/cell.rs	Updates decimal length-check comments.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 100%

🎯 Overall Coverage 91.5%

📦 Project: mssql-tds + mssql-odbc + mssql-py-core ℹ️ Note: diff coverage is reported, not enforced.

---

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

• mssql-odbc/src/api/fetch_convert.rs (100%)
• mssql-tds/src/datatypes/decoder.rs (100%)

Summary

• Total: 120 lines
• Missing: 0 lines
• Coverage: 100%

---

🔗 Quick Links

View Azure DevOps Build · Coverage Report

[Saurabh Singh (saurabh500)]

Read the whole change. The root-cause work here is the strongest part: the issue described one bug, and you fixed it, but you also went looking for why the input was reachable (the 64-word cap) and then found a second defect while you were in there — (length - 1) >> 2 truncating and stranding unread bytes on the stream. That one is nastier than the bug you were asked to fix, since a desynchronized token stream corrupts every field after it, and nobody had reported it.

Two things I'd like fixed before this merges. Neither blocks.

Should fix

1. DecimalParts::magnitude() — the doc contradicts the code. Inline suggestion below.

The doc says None means the value carries more words than a u128 holds, but the check is on word count, not significance. Your own test_decimal_parts_oversized_magnitude_with_trailing_zero_words demonstrates the gap: [12345, 0, 0, 0, 0, 0] is 12345, renders as -123.45 through the magnitude_big() fallback, and still returns None here.

The practical effect is that the two rendering paths disagree about one value. to_decimal_string prints it; numeric_source in mssql-odbc gets None from the same call and reports the column as having no numeric interpretation. So SQLGetData into SQL_C_CHAR succeeds and into SQL_C_SLONG fails, for identical input.

This isn't a regression — the old ODBC guard counted the same way — and after this PR the only way to build an oversized DecimalParts is FFI (NapiDecimalParts round-trips int_parts verbatim) or a hand-built struct, since the wire now caps at 4 and cell.rs caps at 16 bytes. But magnitude() is public now, so the doc is the contract. Either fix is fine; I lean toward trimming so the two paths agree.

2. fetch_convert.rs:1398 — stale reachability claim in a test doc comment.

/// The limbs are reassembled directly, and a payload with more limbs than
/// 128 bits can hold is refused instead of shifting past the width. The wire
/// decoder admits up to 64 limbs, so this is reachable from a bad payload.

"The wire decoder admits up to 64 limbs" is false as of this PR — that's the cap you removed. You fixed the identical sentence in the numeric_source comment (it's called out in the PR description), so this looks like a missed twin rather than a deliberate choice.

It's worth more than a typical stale comment because it's a reachability claim, and the next reader will believe a malformed server payload can still reach that path. It can't anymore; only FFI can. Something like:

/// The limbs are reassembled directly, and a payload with more limbs than
/// 128 bits can hold is refused instead of shifting past the width. The wire
/// decoder now caps the count at 4, so the oversized case below is reachable
/// only through FFI or a hand-built `DecimalParts`.

I couldn't leave this one inline — it's outside the diff hunks.

Follow-ups (not this PR)

Both are pre-existing and I don't think either should hold this up, but this change is the natural moment to notice them. Happy to file both if you'd rather not.

• decoder.rs:2157 — SQL_VARIANT numeric length is truncated by as u8. data_length is an unbounded u32 read off the wire; 0x10003 as u8 == 3, so read_decimal_data consumes 3 bytes and strands 65536 on the stream. That's the same desync class this PR eliminates, one frame above the code you fixed, and the sibling BigVarBinary arm already range-checks against MAX_ALLOC_SIZE while the numeric arm doesn't.
• fetch_convert.rs:215 — oversized decimal surfaces as 07006, not 22003. numeric_source(...).ok_or(ConvError::Restricted) reports "restricted data type attribute violation", but a decimal column does convert to SQL_C_SLONG — it's this value that doesn't fit. 22003 is the closer match, and the same function already uses it for oversized character literals. Came in with #217; your change makes this the single shared entry point.

What went well

The fix is well-shaped: defense at both layers instead of picking one, | instead of + because the words are disjoint, and a BigUint fallback so a public FFI-constructible type renders exactly rather than panicking or silently truncating. Deleting the duplicate guard in mssql-odbc so there's one implementation instead of two that can drift is the right call, and I verified that refactor is behavior-preserving line for line. The three commits split cleanly along those three ideas and each stands alone.

The tests are the best part. decimal_oversized_partial_word_length_rejected at length 18 pins the exact boundary that rounding down used to accept, and decimal_partial_trailing_word_is_fully_consumed asserts the next field reads back intact — testing stream position rather than just the decoded value, which is the only way that class of bug stays fixed. And you went back and corrected the comments in cell.rs that cited the old behavior as their justification. That last part is the bit most people skip.

[Saurabh Singh (saurabh500)]

Some comments, but original PR intention satisfied. Ready to approve after a decision on the should fix comments.

[David Engel (David-Engel)]

Both "should fix" items are addressed.

1. DecimalParts::magnitude() — the doc contradicts the code.

Took the suggestion in a008c5a. magnitude() now counts significant words with rposition, so [12345, 0, 0, 0, 0, 0] returns Some(12345) and SQL_C_CHAR and SQL_C_SLONG agree on it; [1, 0, 0, 0, 1] and [0, 0, 0, 0, 1] still return None and take the magnitude_big() path. I went with trimming rather than rewording the doc for the reason you gave — the divergence was real even if only FFI-reachable, and "does not fit in a u128" is what a caller would assume.

Replying inline as well with the test detail. Short version: the tests now assert on magnitude() directly instead of only observing it through to_decimal_string(), plus an empty-int_parts case for the rposition → map_or(0, ..) branch, and an ODBC-side assertion that the zero-padded value now converts instead of being refused.

2. fetch_convert.rs:1398 — stale reachability claim in a test doc comment.

Fixed in 3953238, using your wording. You read it right — a missed twin, not a deliberate choice. I updated the numeric_source comment when I deleted the guard next to it and never grepped for the other copy of the sentence. Agreed it matters more than a typical stale comment, since it tells the next reader a malformed server payload still reaches that path when only FFI can.

On the follow-ups

Both look right to me and I have left them out of this PR, so please do file them.

The SQL_VARIANT one is the more interesting of the two: it is the same desync class this PR removes, sitting one frame above the code I touched, and 0x10003 as u8 == 3 stranding 65536 bytes is exactly the failure mode decimal_partial_trailing_word_is_fully_consumed was written to catch a layer down. The BigVarBinary arm range-checking against MAX_ALLOC_SIZE while the numeric arm does not looks like an oversight rather than a distinction. Worth noting on the issue that the fix wants a stream-position assertion, not just a decoded-value one.

On the 22003-vs-07006 point: agreed, and this PR does make numeric_source the single shared entry point, so the fix is now a one-line change at fetch_convert.rs:215 instead of two. I left it alone because changing a returned SQLSTATE is a behavior change that deserves its own PR and its own note.

[Saurabh Singh (saurabh500)]

Filed the SQL_VARIANT one as #280.

Went a bit wider than the numeric arm once I looked: the same as u8 narrowing is in six of the nine dispatch arms — Guid and DateN on the 0-prop path, the whole TimeN/DateTime2N/DateTimeOffsetN family on the 1-prop path, plus the numeric arm. The only two that widen to usize are the binary and string arms, and both range-check. That asymmetry is the strongest argument for your "oversight, not a distinction" read, so I put the arm-by-arm table in the issue.

Two cases worth surfacing here since they're worse than the 65536-byte strand:

• data_length = 256 on the numeric arm truncates to 0, and read_decimal_data treats length 0 as NULL and returns having consumed nothing. The column reports NULL and 256 bytes strand. Silent value corruption on top of the desync.
• read_guid validates length != 16, but data_length = 272 truncates to exactly 16 and sails through it. The truncation defeats the check that runs after it.

On the stream-position assertion — agreed it's the right assertion, and I checked what it would cost: TdsPacketReader has no position or consumed-bytes accessor, so an in-decoder consumed == data_length check means adding a trait method. Asserting a following column reads back intact, the way decimal_partial_trailing_word_is_fully_consumed does, gets the same coverage without touching the trait. Noted both on the issue so whoever picks it up doesn't rediscover it.

Happy to file the 22003-vs-07006 one too — say the word. Agreed it deserves its own PR; a returned SQLSTATE is a behavior change and shouldn't ride along with a decoder fix.

[David Engel (David-Engel)]

Thanks for filing #280, and for widening it — six of nine arms is a much stronger finding than the single arm I flagged. The two cases you surfaced are worse than the strand I described:

data_length = 256 truncating to 0 is the one that worries me most. Length 0 meaning NULL is load-bearing throughout the decoder, so the truncation does not just desync the stream, it produces a plausible wrong answer — a column that reads as NULL when the server sent a value. A desync usually announces itself as garbage in the next field; this one can survive all the way to the caller looking like legitimate data.

data_length = 272 sailing through read_guid's length != 16 check is the same shape and makes the general point better than either of us put it originally: the narrowing happens before the validation, so every downstream length check in those six arms is validating a value the server did not send. The check is not weak, it is being handed the wrong input.

On the stream-position assertion — good call checking the cost. Agreed that adding a trait method to TdsPacketReader is out of proportion for this, and that the read-the-next-column-back approach gets equivalent coverage. Worth adding on the issue that whoever picks it up should assert on a following column of a different type, since a same-type column can mask an off-by-a-whole-field error by decoding the stranded bytes into something that looks right.

Please do file the 22003-vs-07006 one. Now that numeric_source is the single entry point it is a genuinely small change, but it is still a returned-SQLSTATE change that an application can branch on, so it deserves its own PR, its own note, and a look at whether any of the ODBC e2e tests pin 07006 for that path.

[Saurabh Singh (saurabh500)]

David's bot file the follow up instead of asking my bot! 😁

[David Engel (David-Engel)]

Ha — fair. 😄 I would, but I am running in unattended merge-check mode on this PR right now, and my authorized actions this tick are scoped to this pull request: addressing review comments, fixing CI, and resolving conflicts. Filing a new issue is outside that scope, so I am not going to quietly do it and then tell you I did.

Flagging it for David Engel (@David-Engel) to file (or to turn me loose on in a normal session). To make that a copy-paste rather than a rediscovery, the content is:

Title: Oversized decimal in SQLGetData reports 07006 instead of 22003

numeric_source(...).ok_or(ConvError::Restricted) in fetch_convert.rs:215 returns 07006 ("restricted data type attribute violation"), which says the conversion is not allowed. But decimal → SQL_C_SLONG is a legal conversion; it is this particular value that will not fit. 22003 ("numeric value out of range") is the closer match, and the same function already uses it for oversized character literals, so the fix also makes the function self-consistent.

After #237 there is one shared entry point, so it is a one-line change — but it is still a returned SQLSTATE that an application can branch on, so: its own PR, and a check for any ODBC e2e test that pins 07006 on that path before changing it.

[David Engel (David-Engel)]

Turned loose on it after all — filed as #281. 😄

While writing it up I found the framing we were both using was too narrow. We had this down as FFI-only, since #237 caps the wire at four words. But there is a second path to that ?:

let m = i128::try_from(d.magnitude()?).ok()?;

magnitude() returning None is the >4-word case, and yes, that is FFI-only now. i128::try_from failing is not. The decoder validates the word count, never that the magnitude fits the declared precision — so a 17-byte payload, a perfectly legal length, carrying a magnitude in (i128::MAX, u128::MAX] decodes fine and then falls out here. A valid decimal(38, s) tops out around 2^126.2 so no honest server sends this, but a malformed or hostile one can.

Confirmed rather than assumed, with a throwaway test on 39532382:

magnitude       = Some(340282366920938463463374607431768211455)
i128::try_from  = false
Display         = 340282366920938463463374607431768211455
convert result  = Err(Restricted)      // -> 07006

Same value, so the two paths disagree again: SQL_C_CHAR prints it, SQL_C_SLONG says the conversion is illegal. Exactly the divergence your inline comment was about, one layer down and reachable from the wire.

The other thing that changed my read: I had described the fix as a one-line swap at fetch_convert.rs:215, and that is wrong. numeric_source returns Option, and None is doing double duty — "this column has no numeric interpretation" (a binary or GUID column, correctly 07006) and "this value overflowed" (should be 22003). Line 255 flattens both into Restricted. Swapping the error inside the Decimal arm alone would turn a genuine 07006 into 22003 for the other arms, so the signature has to change: Result<NumericSource, ConvError>, or some other way for the decimal arm to signal out-of-range distinctly.

Also noted on the issue: decimal_limbs_are_reassembled_and_bounded currently pins ConvError::Restricted and will need updating; I grepped 07006 across mssql-odbc and no e2e test pins it for this path, so that unit test is the whole blast radius; and since the pipeline already runs the msodbcsql parity comparison, it is worth confirming what the reference driver returns here rather than reasoning from the spec.

Last one is a question for you rather than something I put weight on in the issue: should the decoder range-check a decimal magnitude against its declared precision? That would close the wire-reachable half at the source instead of at the conversion, and it rhymes with #280 — a length or magnitude that is structurally well-formed but semantically impossible for the declared type.