Skip to content

Fix: Add buffer accumulation to TDSTLSFramer for Azure SQL Database #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mklahn wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix: Add buffer accumulation to TDSTLSFramer for Azure SQL Database #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Sources/CosmoMSSQL/TDS/TDSPreLogin.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ enum PreLoginEncryption: UInt8 {
}

struct TDSPreLoginRequest {
var clientVersion: (UInt8, UInt8, UInt8, UInt8) = (8, 0, 0, 0)
// Changed from TDS 8.0 to TDS 7.4 for better Azure SQL compatibility
var clientVersion: (UInt8, UInt8, UInt8, UInt8) = (7, 4, 0, 0)
var encryption: PreLoginEncryption

init(encryption: PreLoginEncryption = .on) {
Expand Down
29 changes: 27 additions & 2 deletions Sources/CosmoMSSQL/TDS/TDSTLSFramer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,27 +29,52 @@ final class TDSTLSFramer: ChannelDuplexHandler, @unchecked Sendable {
/// the TLS handshake completes.
var active: Bool = false

// Accumulator buffer for incomplete TDS packets
private var accumulatedBuffer: ByteBuffer?

// MARK: - Inbound

func channelRead(context: ChannelHandlerContext, data: NIOAny) {
guard active else {
context.fireChannelRead(data)
return
}
var buf = unwrapInboundIn(data)
var incoming = unwrapInboundIn(data)

// Accumulate incoming data with any previously buffered data
if var accumulated = accumulatedBuffer {
accumulated.writeBuffer(&incoming)
accumulatedBuffer = accumulated
} else {
accumulatedBuffer = incoming
}

guard var buf = accumulatedBuffer else { return }

// The buffer may contain one or more TDS Pre-Login packets.
// Each packet: [type 1B][status 1B][length 2B big-endian][spid 2B][pid 1B][win 1B] + payload
while buf.readableBytes >= 8 {
guard
let lengthBE: UInt16 = buf.getInteger(at: buf.readerIndex + 2, endianness: .big),
Int(lengthBE) <= buf.readableBytes,
Int(lengthBE) >= 8
else { break }
else {
// Save remaining data for next read
accumulatedBuffer = buf
return
}
Comment on lines 57 to +65
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle invalid TDS header lengths as protocol errors, not partial frames.

Int(lengthBE) >= 8 is currently grouped with partial-read handling. For malformed headers (lengthBE < 8), this path keeps buffering forever and never advances, which can stall the connection and grow memory.

Suggested fix 
         while buf.readableBytes >= 8 {
-            guard
-                let lengthBE: UInt16 = buf.getInteger(at: buf.readerIndex + 2, endianness: .big),
-                Int(lengthBE) <= buf.readableBytes,
-                Int(lengthBE) >= 8
-            else {
-                // Save remaining data for next read
-                accumulatedBuffer = buf
-                return
-            }
-            let packetLen = Int(lengthBE)
+            guard let lengthBE: UInt16 = buf.getInteger(at: buf.readerIndex + 2, endianness: .big) else {
+                accumulatedBuffer = buf
+                return
+            }
+            let packetLen = Int(lengthBE)
+
+            // Invalid TDS frame length: fail fast instead of buffering forever.
+            guard packetLen >= 8 else {
+                accumulatedBuffer = nil
+                context.close(promise: nil)
+                return
+            }
+
+            // Incomplete frame: keep buffering until full packet arrives.
+            guard packetLen <= buf.readableBytes else {
+                accumulatedBuffer = buf
+                return
+            }
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Sources/CosmoMSSQL/TDS/TDSTLSFramer.swift` around lines 57 - 65, The guard
that checks the TDS packet length (reading lengthBE in TDSTLSFramer.swift)
currently treats all failures as a partial read by saving accumulatedBuffer;
change it so that a lengthBE < 8 is treated as a protocol error rather than
buffering: if lengthBE < 8, do not assign accumulatedBuffer, instead fail the
pipeline/connection (emit/throw a protocol error indicating "invalid TDS header
length") and close the connection; only keep the existing buffering logic for
cases where Int(lengthBE) > buf.readableBytes (genuine partial frames). Ensure
you reference the same variables (lengthBE, accumulatedBuffer, buf) and perform
a clear error emission/connection teardown in the TDSTLSFramer code path.

let packetLen = Int(lengthBE)
var packet = buf.readSlice(length: packetLen)!
packet.moveReaderIndex(forwardBy: 8) // skip TDS header
context.fireChannelRead(wrapInboundOut(packet))
}

// If all data was processed, clear the accumulator
if buf.readableBytes == 0 {
accumulatedBuffer = nil
} else {
accumulatedBuffer = buf
}
}

// MARK: - Outbound
Expand Down