Skip to content

harden: malformed-input hardening, non-panicking encoders, unit coverage - #436

Closed
MattJackson wants to merge 1 commit into
tiberius-rs:sync/s4from
MattJackson:sync/s5
Closed

harden: malformed-input hardening, non-panicking encoders, unit coverage#436
MattJackson wants to merge 1 commit into
tiberius-rs:sync/s4from
MattJackson:sync/s5

Conversation

@MattJackson

Copy link
Copy Markdown
Contributor

Robustness hardening and broad unit-test coverage across the codec/token layer.

  • Bounds/robustness hardening across codec and token decoders against malformed server input.
  • Non-panicking var-len encoders; DONE-status leniency.
  • AAD-token redaction/zeroize; numeric rescale to column scale instead of panicking.
  • Broad in-crate unit-test coverage across decoders, time, tokens, config.

Squashed into a single commit (all the submitter's own work — no external authorship to preserve here).

Supersedes: none.

Sequential series — merge after #432#435. Based on main; diff reduces to its own delta as the earlier PRs land.
Reviewer note: rebase-merge or merge-commit, not squash (keeps the series consistent).

@victorchutw

Copy link
Copy Markdown

Thanks for lifting the scale < 38 assertion in Numeric::new_with_scaleDECIMAL(38,38) is a valid SQL Server type, and the released 0.12.3 panics on it (assertion failed: scale < 38, src/tds/numeric.rs:35).

One half of the invariant is still missing on this branch: Numeric::precision() counts a phantom integral digit when the integral part is zero (1 + scale), so a fraction-only scale-38 value is described as numeric(39,38), which SQL Server rejects as a type. Reproduction on this PR's head (394574729ea48776a127b280eab623ac718779d4):

let n = Numeric::new_with_scale(-1, 38);
assert_eq!(n.precision(), 39); // passes today; the only valid answer is 38

precision() feeds ColumnData::type_name() (the numeric(p,s) in the RPC parameter declaration) and the NUMERICNTYPE TYPE_INFO header written when no destination type info is available, so SELECT @P1 with such a value declares @P1 numeric(39,38), above SQL Server's maximum precision of 38, even with the assertion lifted. The precision_with_zero_int_part test in this PR pins the 1 + scale behaviour, so this is a request to change it rather than a regression report.

The complete invariant, as far as I can tell: with no integral digits, precision = max(scale, 1)0.05 is numeric(2,2), zero at scale 0 stays numeric(1,0), and a scale-38 fraction is numeric(38,38); with integral digits, keep integral_digits + scale. The rust_decimal and bigdecimal conversions route through the same constructor and precision(), so they inherit the fix. A side effect worth knowing: fraction-only values at scales 9, 19, and 28 move down one wire-length bucket (5/9/13 bytes instead of 9/13/17), which SQL Server 2022 accepts on both the RPC and bulk paths.

I am carrying exactly that change as a focused patch on top of the 0.12.3 sources: victorchutw@65c1a32 — the constructor bound, the precision correction, unit tests (construction, precision, length buckets, encode/decode round trips, TYPE_INFO bytes, scale-39 rejection), and live SELECT @P1 plus bulk-insert round trips of DECIMAL(38,38) boundary values and the bucket-edge fractions, all green against SQL Server 2022. Happy to open it as its own issue and PR against main if you would rather keep this series as is.

victorchutw added a commit to victorchutw/data-spark that referenced this pull request Sep 2, 2026
* Pin Tiberius to a fork carrying the decimal(38,38) Numeric fix

tiberius 0.12.3 from crates.io asserts `scale < 38` in
`Numeric::new_with_scale` and describes a fraction-only scale-38 value as
the invalid `numeric(39,38)`, so a valid `decimal(38,38)` declaration
(ADR-0044, mapped verbatim by ADR-0062) could never reach SQL Server.
Upstream main still carries the assertion and the open hardening PR
tiberius-rs/tiberius#436 fixes only the assertion.

Activate ADR-0069's fork rung: pin the maintainer-controlled fork
victorchutw/tiberius by full commit SHA 65c1a32f (upstream "Bump v0.12.3"
plus one Numeric patch that accepts scale <= 38 and computes the
precision of a value without integral digits as max(scale, 1)). The lock
records the same revision and nothing else moves. Add a direct offline
smoke test of the pinned revision, record the activation, verification
posture, upstream trace, and crates.io exit in ADR-0069, and note in
ADR-0062 that decimal(38,38) is not a third representability sliver.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Tighten the dependency smoke test and ADR wording after review

Name the smoke test without the glossary-avoided "numeric", prove scale-39
rejection with should_panic(expected) like the rest of the repo, keep one
determinism assertion, trim the Cargo.toml comment to the sibling style,
and make ADR-0069 name the fork's test-module edits and the branch/revision
relationship precisely.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* Narrate ADR-0069's crates.io source historically

The opening paragraph described the crates.io source and caret requirement
in the present tense, contradicting the fork pin now in Cargo.toml. State
both as the original decision, point at the fork activation that replaced
only the source, and note that the lockfile discipline carries over.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: 朱余晟 <victorchu@gamasys.com.tw>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
@MattJackson

Copy link
Copy Markdown
Contributor Author

Superseded by #443. Re-opened as a native GitHub stacked PR (stack #445) per @aqrln's request in #440 — the fork origin of these PRs made a native stack impossible (GitHub can't add fork-head PRs to a stack), so the branches now live in tiberius-rs/tiberius directly. Same commits, same authorship. Please review #443 instead.

@MattJackson MattJackson closed this Sep 2, 2026
@MattJackson
MattJackson deleted the sync/s5 branch September 2, 2026 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants