fix: land pending community fixes (columns, numeric, IN-lists, packet_size, zeroize, bulk, …) by MattJackson · Pull Request #441 · tiberius-rs/tiberius

MattJackson · 2026-09-02T15:07:24Z

A batch of community-contributed fixes, each kept as a standalone commit with its original author preserved:

occured typo (@DucMinhNe) · empty into_results QueryStream into_result doesn't return correct number of results #380 (ingrese1nombre) · column names like End / with spaces (@cjordan) · negative-Numeric sign/padding (@zuckschwerdt) · IN-list & 2100-param helpers (@joelparkerhenderson) · row-by-index accessors (LazyDope) · SSPI response header (@staticlibs) · packet_size config (@johndauphine) · zeroize SQL password buffers (@lstkz) · IntoSql for rust_decimal (@esheppa) · bulk-insert column list (@NTmatter) · test-server.sh helper (@joelparkerhenderson).

Supersedes #296, #304, #351, #359, #376, #385, #388, #390, #400, #411, #423, #429.
Complementary — not superseded, please keep open: #387 (@polina-alekseeva-rogii) adds ]→]] escaping beyond #388.

Sequential series — merge in order after #432, #433. Based on main, so its diff reduces to its own 16 commits once the earlier PRs land.
Reviewer note: please rebase-merge or merge-commit, not squash — these carry each contributor's authorship; squashing collapses that credit.

MattJackson · 2026-09-04T20:50:39Z

@aqrln rebased onto the merged main, all green and MERGEABLE — ready for review whenever you get a chance. Thanks!

SimSmith · 2026-09-06T10:56:38Z

        // retrieve column metadata from server
-        let query = format!("SELECT TOP 0 * FROM {}", table);
+        let columns = columns.join(", ");
+        let query = format!("SELECT TOP 0 {columns} FROM {table}");


Possible SQL injection here. I mean it's unlikely that this function gets untrusted input, but it could happen. Maybe add a comment to the doc or make the inputs identifiers.

Good point. Added a # Security note that the table/column identifiers are interpolated into the batch (T-SQL can't parameterize identifiers) so they must be trusted, plus a guard that rejects a table name with control chars or an unbalanced ]. Didn't auto-quote, since qualified/bracketed names must keep working.

SimSmith · 2026-09-06T11:01:27Z

 impl<'a> Display for MetaDataColumn<'a> {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{} ", self.col_name)?;
+        write!(f, "[{}] ", self.col_name)?;


Good change! An even better one would be to escape ] too.

Agreed — now escaping ] as ]] when bracket-quoting the column name. This is what #387 proposed; credit to @polina-alekseeva-rogii. Added a test with a ] in the name.

SimSmith · 2026-09-06T11:13:56Z

        cursor.write_u32::<LittleEndian>(cursor.get_ref().len() as u32)?;

-        dst.extend(cursor.into_inner());
+        Ok(Zeroizing::new(cursor.into_inner()))


This hardening is incomplete, because of a potential reallocation after the password is written to the buffer. But the change is good, so not a blocker for this PR.

Right — the buffer could reallocate after the password was written, abandoning an un-zeroized copy in freed heap. Now reserving the full capacity up front (via an exact encoded_len()) so the Vec never reallocates after the password write, keeping the single zeroize complete. Added an always-on capacity assert so a future layout change can't silently reintroduce it.

SimSmith · 2026-09-06T11:17:19Z

+        let mut result: Vec<Row> = if self.try_next().await?.is_some() {
+            Vec::new()


What is thrown away here? Is it assumed to be the metadata?

Yes — it assumed the first stream item is the leading COLMETADATA and discarded it. Reworked into_results (now a testable collect_results) to start a new result set on each metadata token and push rows into the current one, so nothing is dropped on an assumption. Added a leading_row_is_not_discarded test.

MattJackson · 2026-09-07T01:49:26Z

Updated + green. Fixed a duplicate #[cfg(test)] mod tests that broke the test binary's compile, hardened Numeric's Debug impl against an i128::MIN panic (unsigned_abs), extended the bulk identifier validation to column names (not just the table name), and added a server-free test for the sensitive-login packet chunking.

aqrln · 2026-09-15T20:48:48Z

 pub struct SqlServerAuth {
    user: String,
-    password: String,
+    password: Zeroizing<String>,


(non-blocking suggestion, doesn't need to happen in this PR even if you take it)

perhaps we should even use SecretString from secrecy?

Good suggestion — worth capturing that this is an enhancement rather than a fix, since the current approach is already sound and tested:

It's the consistent pattern here. The whole crate protects secrets with zeroize::Zeroizing — the SQL Server and Windows passwords, the AAD token, the connection-string password, and the framed login payload/packets all use it. secrecy isn't currently a dependency or used anywhere, so adopting SecretString would introduce a second convention rather than align with the existing one.

It's covered by tests. Our zeroize + redaction guarantees are asserted, not assumed:

auth::tests::sql_server_password_can_be_consumed_and_zeroized — round-trips the password and asserts it's empty after .zeroize().

auth::tests::debug_redacts_credentials — asserts {:?} of the SQL password and the AAD token never contain the secret and render <HIDDEN>.

auth::tests::windows_auth_parses_domain_and_debug_redacts — same for WindowsAuth.

the login-packet Debug test in login.rs and the connection-string Debug test in config.rs assert the secret is redacted (<HIDDEN> / <redacted>) on the wire-encoding and config paths too.

What SecretString would add on top is expose_secret()-gated access (so plaintext can't be reached by accident) with redaction/zeroize baked into the type. That's a real ergonomic improvement, but it pulls in the secrecy dependency and touches every credential path — so I'd rather do it as a focused follow-up that can be reviewed on its own, rather than fold it into this PR.

aqrln · 2026-09-15T20:51:46Z

+            event!(Level::TRACE, "Sending a packet ({} bytes)", frame.len(),);
+
+            self.transport.write_all(frame.as_slice()).await?;
+            frame.zeroize();


I think Zeroizing automatically does that on Drop anyway

Good catch — you're right. frames is Vec<Zeroizing<Vec<u8>>>, so each frame is dropped at the end of its loop iteration and Zeroizing's Drop wipes it there — the explicit frame.zeroize() is redundant (and makes the mut binding unnecessary). Will remove both.

I'll keep the explicit payload.zeroize(): payload otherwise lives until the end of the function, so wiping it right after framing drops that plaintext copy before the network-write loop's .awaits — leaving only the in-flight frame bytes in memory during the I/O. Happy to drop that one too and lean entirely on Drop if you'd prefer.

aqrln · 2026-09-15T20:57:43Z

+    // Used only on winauth / integrated-auth-gssapi builds.
+    #[allow(dead_code)]


can we #[cfg(...-gate it instead?

Agreed — will replace the blanket #[allow(dead_code)] with #[cfg(all(windows, feature = "winauth"))]. That is the exact (and only) combo that calls PacketHeader::sspi(): the sole caller is the AuthMethod::Integrated arm on Windows + winauth, which sends a standalone SSPI packet. Every other auth path — including unix integrated-auth-gssapi and the Windows AuthMethod::Windows arm — wraps its token in a PacketHeader::login(), so gating any wider would reintroduce a dead-code warning on those builds. Will also add tests around the PacketHeader constructors and header encode/decode round-trips (the sspi test carrying the same gate).

aqrln · 2026-09-15T21:00:01Z

+        // To make that impossible we reserve the exact final size up front,
+        // before writing any variable-length data, so no reallocation can occur


sounds like we might want to convert it to a boxed slice and use (and return) that instead of a vec

Good call — will do, and take it through the whole sensitive-login path rather than just this one return, so the "no reallocation of a secret buffer" guarantee is end-to-end:

encode_to_vec will return Zeroizing<Box<[u8]>>. The existing exact Vec::with_capacity(encoded_len()) + the assert_eq! on capacity stay — that is what keeps the final into_boxed_slice() a no-op rather than a shrink-realloc (if len != capacity it would reallocate and leak the very copy we are guarding against, so the assert is load-bearing).

send_sensitive_login will take Zeroizing<Box<[u8]>> — it only reads the payload as &[u8] and zeroizes it, so a boxed slice is a strict tightening (cannot be grown by accident downstream).

frame_sensitive_login will return Vec<Zeroizing<Box<[u8]>>> — each frame built into an exact-capacity buffer and boxed.

I also found the fed-auth token temp buffer in encode_to_vec has the same class of issue: a Vec::new() grown one u16 at a time (so it can reallocate mid-encode, leaving un-zeroized token fragments in freed heap), with only the final allocation zeroized. Will reserve its exact capacity up front so it cannot reallocate, matching the password buffer.

Will cover it with tests (capacity==len invariants, boxed-slice round-trips, and a fed-auth-token encode path that exercises that buffer).

…columns, test login chunking STEP 1: Merge the two duplicate #[cfg(test)] mod tests in token_col_metadata.rs (E0428) and add the required table_name field to the test-helper BaseMetaDataColumn literal (E0063) so the lib test target compiles again. STEP 2: Numeric Debug used i128::abs(), which panics on i128::MIN (an adversarial 17-byte NUMERIC magnitude decodes to it). Use unsigned_abs() so Debug/Display are total for all i128 inputs; output unchanged for in-range values. Add red-before-green tests. STEP 3: bulk_insert_columns now applies the same control-char/unbalanced-] identifier guard to each columns entry (not just table), matching the documented behavior. Refactor validate_bulk_table_identifier over a shared validate_bulk_identifier core with per-kind messages; add tests. STEP 4: Extract the login packetization into a pure frame_sensitive_login helper and unit-test that an oversized login splits into >=2 correctly framed packets (header/length/status matching Packet::encode).

Each login frame is a `Zeroizing<Vec<u8>>` dropped at the end of its loop iteration, so `Zeroizing`'s `Drop` already wipes it there -- the explicit `frame.zeroize()` (and the now-unnecessary `mut` binding) were redundant. The explicit `payload.zeroize()` is kept intentionally: it drops that plaintext copy before the network-write loop's awaits.

Replace the blanket `#[allow(dead_code)]` on `PacketHeader::sspi` with a `#[cfg(...)]` matching exactly the auth feature/platform combos that call it, so it compiles only where used. Add unit coverage for the `PacketHeader` constructors and header encode/decode round-trips.

…oxed The finished login buffer never needs to grow again, and a shrink-realloc of a Vec holding the (obfuscated) password would free the old allocation without zeroizing it, leaking a recoverable copy. Return the LOGIN7 buffer as Zeroizing<Box<[u8]>> via into_boxed_slice() only after the capacity==len assert (so no shrink-realloc happens), and likewise box each login frame in frame_sensitive_login at exact capacity. Drop the now-redundant explicit zeroize in the Encode impl (the Zeroizing value is wiped on drop at the same point). Add boxed round-trip and no-slack framing tests.

The fed-auth (AAD) security token is a bearer credential encoded into a temporary buffer one UTF-16 code unit at a time. Growing it from an empty Vec could reallocate mid-encode, freeing an un-zeroized copy of the token into freed heap. Reserve its exact final capacity (code-unit count * 2) so it never reallocates, wrap it in Zeroizing so the finished buffer is wiped on drop, and assert the no-realloc invariant. Add a fed-auth encode-path test that exercises the token buffer and round-trips the token/echo/nonce.

MattJackson · 2026-09-16T02:42:20Z

Thanks for the thorough review, @aqrln — this update addresses all four comments. Quick rundown:

1. SecretString (auth.rs). Kept Zeroizing here since it's the crate-wide convention and the redaction/zeroization is already covered by tests — and I'm doing the secrecy migration as a dedicated follow-up PR so it can be reviewed on its own (it touches every credential path, not just this struct).

2. Redundant frame.zeroize(). Removed — each frame is a Zeroizing dropped at the end of its loop iteration, so Drop already wipes it (also dropped the now-needless mut). Kept the explicit payload.zeroize() so the plaintext copy is gone before the write loop's .awaits.

3. #[allow(dead_code)] → #[cfg] on sspi(). Gated it to #[cfg(all(windows, feature = "winauth"))] — that's the sole caller (the Integrated arm on Windows+winauth); every other auth path, including unix integrated-auth-gssapi, wraps its token in PacketHeader::login(), so gating wider would just reintroduce dead-code there.

4. Vec → boxed slice. Took the guarantee through the whole sensitive-login path: encode_to_vec now returns Zeroizing<Box<[u8]>> (the exact with_capacity + capacity assert_eq! stay, so into_boxed_slice() can't shrink-realloc), and send_sensitive_login/frame_sensitive_login carry Box<[u8]> through. While there I noticed the fed-auth token temp buffer had the same realloc-leak risk in miniature (a Vec::new() grown a u16 at a time, only the final allocation zeroized), so it now reserves its exact capacity up front.

While in this area I also went back through the surrounding code for the same patterns: removed one other redundant .zeroize() (in Encode::encode), and confirmed the remaining #[allow(dead_code)] items are genuinely unconditional dead code (partial enums / reserved variants / debug-only) rather than feature-gated, so a #[cfg] doesn't apply to them.

Testing: added 23 tests — the PacketHeader constructors and header encode/decode round-trips, the boxed-buffer capacity == len (no-realloc) invariants, a fed-auth-token encode path that exercises that buffer, and a framed-login no-slack check. The unit suite is at 207 passing, green on both default features and --no-default-features --features rustls,tds73,chrono.

aqrln · 2026-09-16T19:34:13Z

+        // `encoded` is `Zeroizing<Box<[u8]>>`; it is wiped on drop at the end of
+        // this function, immediately after the copy into `dst`, so no explicit
+        // `zeroize()` is needed here.
+        let encoded = self.encode_to_vec()?;


should we rename the method?

Good call — renamed it to encode_to_boxed_slice, since it now returns Zeroizing<Box<[u8]>> rather than a Vec. Updated the method, its callers, the doc references, and the round-trip test.

The method now returns Zeroizing<Box<[u8]>> (since the boxed-slice hardening), so the old '_to_vec' name was misleading. Renames the method, its callers, doc references, and the round-trip test. Addresses review feedback on #441.

The 'Install dependencies' step ran `apt-get install` without a preceding `apt-get update`, so when Ubuntu rotated the krb5 point release the runner's stale package index requested a .deb that had been removed from the mirror, failing every linux job with a 404. Refresh the index first so the current version is fetched.

The method now returns Zeroizing<Box<[u8]>> (since the boxed-slice hardening), so the old '_to_vec' name was misleading. Renames the method, its callers, doc references, and the round-trip test. Addresses review feedback on #441.

MattJackson changed the title ~~stack/s3~~ fix: land pending community fixes (columns, numeric, IN-lists, packet_size, zeroize, bulk, …) Sep 2, 2026

This was referenced Sep 2, 2026

fix: land pending community fixes (columns, numeric, IN-lists, packet_size, zeroize, bulk, …) #434

Closed

Proposal: volunteer as a long-term maintainer #427

Closed

MattJackson force-pushed the stack/s3 branch from b42bdfe to 3e29cb8 Compare September 2, 2026 19:34

MattJackson force-pushed the stack/s3 branch 3 times, most recently from 6ece6d2 to 6954578 Compare September 2, 2026 21:17

MattJackson force-pushed the stack/s3 branch from 1b5327d to 9afcb52 Compare September 2, 2026 23:54

MattJackson force-pushed the stack/s3 branch from 9afcb52 to 35065b6 Compare September 3, 2026 00:01

MattJackson closed this Sep 3, 2026

MattJackson reopened this Sep 3, 2026

MattJackson force-pushed the stack/s3 branch from 59acede to 6de8902 Compare September 3, 2026 15:05

MattJackson force-pushed the stack/s3 branch from 6de8902 to c1f9851 Compare September 4, 2026 20:42

SimSmith reviewed Sep 6, 2026

View reviewed changes

MattJackson force-pushed the stack/s3 branch 2 times, most recently from 7b79df1 to e480769 Compare September 7, 2026 00:01

MattJackson force-pushed the stack/s3 branch from e480769 to 956caf9 Compare September 8, 2026 18:07

aqrln reviewed Sep 15, 2026

View reviewed changes

Base automatically changed from stack/s2 to main September 15, 2026 23:02

MattJackson added 2 commits September 15, 2026 16:02

fix(query): rewrite into_results to not discard the first stream item

38896ba

MattJackson force-pushed the stack/s3 branch from 956caf9 to 7906d55 Compare September 15, 2026 23:02

MattJackson added 4 commits September 15, 2026 16:20

aqrln approved these changes Sep 16, 2026

View reviewed changes

MattJackson added 2 commits September 16, 2026 14:06

MattJackson merged commit feb8df2 into main Sep 16, 2026
33 checks passed

MattJackson deleted the stack/s3 branch September 16, 2026 21:40

		let mut result: Vec<Row> = if self.try_next().await?.is_some() {
		Vec::new()

		// Used only on winauth / integrated-auth-gssapi builds.
		#[allow(dead_code)]

		// To make that impossible we reserve the exact final size up front,
		// before writing any variable-length data, so no reallocation can occur

Conversation

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

Uh oh!

Uh oh!

MattJackson commented Sep 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattJackson commented Sep 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattJackson commented Sep 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

MattJackson commented Sep 2, 2026 •

edited

Loading

MattJackson Sep 15, 2026 •

edited

Loading

MattJackson Sep 15, 2026 •

edited

Loading