Skip to content

Display impl for PacketSize and Database EnvChange variants shows old/new values swapped #418

@johndauphine

Description

@johndauphine

The Display implementations for TokenEnvChange::PacketSize and
TokenEnvChange::Database format the two stored values in the wrong
order, producing log lines that report the opposite of what actually
happened during connection negotiation.

Where the bug is

Both pieces are in src/tds/codec/token/token_env_change.rs.

Parser stores (new, old) — wire order per MS-TDS 2.2.7.10 is
NewValue first, OldValue second:

// PacketSize — line 173
let new_value = String::from_utf16(...)?;   // wire field 1
let old_value = String::from_utf16(...)?;   // wire field 2
TokenEnvChange::PacketSize(new_value.parse()?, old_value.parse()?)
//                         ^^^ tuple.0 = NEW   ^^^ tuple.1 = OLD

// Database — line 152, same shape
TokenEnvChange::Database(new_value, old_value)

Display then pattern-matches the tuple as (old, new):

// line 87
Self::Database(ref old, ref new) => {
    write!(f, "Database change from '{}' to '{}'", old, new)
}

// line 90
Self::PacketSize(old, new) => {
    write!(f, "Packet size change from '{}' to '{}'", old, new)
}

Net effect: the log line Packet size change from 'X' to 'Y' actually
means the size went from Y to X — and the same for Database changes.

SqlCollation { old, new } and Routing { host, port } are
struct-shape variants and bind by name, so they are unaffected.

Real-world impact

This is more than a cosmetic logging issue — it actively misleads anyone
debugging packet-size negotiation, which is a common perf-tuning area
when bulk-loading data via tiberius.

In one debugging session the log emitted:

Packet size change from '32576' to '4096'

against an Azure SQL Edge server with network packet size (B) = 4096
(server default) where the client (tiberius via a fork that calls
Config::packet_size(32767)) was issuing LOGIN7 with PacketSize=32767.
Reading the log at face value suggested the server was down-negotiating
to 4096 and the large-packet request was being ignored — which led to
~30 minutes investigating SQL Server config, the fork, and sp_configure
settings that turned out to be irrelevant. Reading the parser source
revealed the log was simply lying: the actual transition was 4096 →
32576 (an up negotiation, exactly as intended).

Suggested fix

Either flip the binding order in Display or flip the construction
order in the parser. The least intrusive change is the Display:

Self::Database(ref new, ref old) => {
    write!(f, "Database change from '{}' to '{}'", old, new)
}
Self::PacketSize(new, old) => {
    write!(f, "Packet size change from '{}' to '{}'", old, new)
}

Happy to send a PR if you'd like.

Versions

  • Bug present on main as of today; the affected lines are unchanged
    for years.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions