Skip to content

Commit e1b6ec3

Browse files
Probe correctness: DNS/IPv6, A2S challenge, SLP framing, real timeouts (#60)
* fix(probe): DNS/IPv6, A2S challenge handshake, SLP framing, real timeouts The probe engine failed silently against most real servers: - IPv4 literals only: parseIp4 rejected every hostname and IPv6 address. New resolveAddress() accepts v4/v6 literals and falls back to DNS (getAddressList); sockets now open with the resolved address family. - A2S challenge ignored: modern Source servers first reply 0x41 + a 4-byte token and only send A2S_INFO (0x49) after a re-query with that token. The old code returned null on 0x41, so it failed against nearly all current servers. Now performs the one-shot challenge handshake. - Minecraft SLP truncation: a single read() corrupted any MOTD spanning multiple TCP segments. Now reads in a loop until the VarInt-declared packet length is satisfied (new decodeVarInt helper), buffer grown to 16K. - timeout_ms ignored: probeSingle discarded its budget (_= timeout_ms) and every sub-probe hardcoded 3000ms. The parameter is now threaded through trySteamQuery/tryMinecraftQuery/tryRCON/tryHTTPProbe/tryTCPBanner. Adds unit tests for decodeVarInt (incl. 25565 and incomplete-input cases) and resolveAddress (v4/v6 literals). 152 tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TaWWedv6VQqeZaPvc94keN * docs(http): mark the unused timeout constants as an honest known-limitation std.http.Client in Zig 0.15.2 has no timeout/deadline API, so STEAM_API_TIMEOUT_MS / GROOVE_TIMEOUT_MS could not be honoured and were dead. Rather than ship a subtle threading fix under time pressure, document the limitation precisely and track enforcement (raw-socket client or watchdog) as a follow-up. No behaviour change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TaWWedv6VQqeZaPvc94keN --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent ae0cbcd commit e1b6ec3

3 files changed

Lines changed: 131 additions & 31 deletions

File tree

src/interface/ffi/src/groove_client.zig

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,10 @@ const GROOVE_TTS_PATH: []const u8 = "/.well-known/groove/tts";
4040
/// Maximum number of Groove targets we track simultaneously.
4141
const MAX_TARGETS: usize = 8;
4242

43-
/// HTTP timeout for Groove probes (milliseconds).
43+
/// Intended per-request deadline for Groove probes (milliseconds). NOT yet
44+
/// enforced — Zig 0.15.2's std.http.Client has no timeout knob, so a hung
45+
/// endpoint blocks the caller. Wiring it (raw-socket client or watchdog thread)
46+
/// is a tracked follow-up; kept as the contract to honour.
4447
const GROOVE_TIMEOUT_MS: u32 = 3000;
4548

4649
// ═══════════════════════════════════════════════════════════════════════════════

src/interface/ffi/src/probe.zig

Lines changed: 122 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -142,12 +142,27 @@ pub const KNOWN_PORTS: []const KnownPort = &.{
142142
// Network helpers
143143
// ═══════════════════════════════════════════════════════════════════════════════
144144

145+
/// Resolve `host` to a socket address. Accepts IPv4/IPv6 literals directly
146+
/// (no allocation) and falls back to DNS (libc getaddrinfo via
147+
/// net.getAddressList) for hostnames, returning the first address. Replaces the
148+
/// old IPv4-literal-only path that failed on every hostname and IPv6 address.
149+
fn resolveAddress(host: []const u8, port: u16) !net.Address {
150+
if (net.Address.parseIp(host, port)) |a| {
151+
return a;
152+
} else |_| {}
153+
const list = net.getAddressList(std.heap.c_allocator, host, port) catch
154+
return error.NameResolutionFailed;
155+
defer list.deinit();
156+
if (list.addrs.len == 0) return error.NameResolutionFailed;
157+
return list.addrs[0];
158+
}
159+
145160
/// Open a TCP connection to host:port with a timeout in milliseconds.
146161
/// Returns the connected stream or an error.
147162
fn tcpConnect(host: []const u8, port: u16, timeout_ms: u32) !net.Stream {
148-
const addr = try net.Address.parseIp4(host, port);
163+
const addr = try resolveAddress(host, port);
149164
const sock = try posix.socket(
150-
posix.AF.INET,
165+
@as(u32, addr.any.family),
151166
posix.SOCK.STREAM | posix.SOCK.NONBLOCK,
152167
0,
153168
);
@@ -186,9 +201,9 @@ fn udpExchange(
186201
response_buf: []u8,
187202
timeout_ms: u32,
188203
) !usize {
189-
const addr = try net.Address.parseIp4(host, port);
204+
const addr = try resolveAddress(host, port);
190205
const sock = try posix.socket(
191-
posix.AF.INET,
206+
@as(u32, addr.any.family),
192207
posix.SOCK.DGRAM,
193208
0,
194209
);
@@ -214,6 +229,23 @@ fn udpExchange(
214229
return n;
215230
}
216231

232+
/// A decoded Minecraft-protocol VarInt: its value and how many bytes it spans.
233+
const VarIntDecode = struct { value: usize, bytes: usize };
234+
235+
/// Decode a leading Minecraft VarInt (LEB128, 7 data bits/byte, MSB = continue,
236+
/// max 5 bytes) from `buf`. Returns null if `buf` does not yet contain a
237+
/// complete VarInt — used to know when a framed read has enough header bytes.
238+
fn decodeVarInt(buf: []const u8) ?VarIntDecode {
239+
var value: u32 = 0;
240+
var i: usize = 0;
241+
while (i < buf.len and i < 5) : (i += 1) {
242+
const b = buf[i];
243+
value |= @as(u32, b & 0x7F) << @intCast(i * 7);
244+
if (b & 0x80 == 0) return .{ .value = value, .bytes = i + 1 };
245+
}
246+
return null;
247+
}
248+
217249
/// Measure time between send and first byte of response, returning nanoseconds.
218250
fn measureLatencyNs(start: std.time.Instant) u64 {
219251
const now = std.time.Instant.now() catch return 0;
@@ -228,23 +260,39 @@ fn measureLatencyNs(start: std.time.Instant) u64 {
228260
/// the Source Engine response header to extract game name and version.
229261
///
230262
/// Reference: https://developer.valvesoftware.com/wiki/Server_queries#A2S_INFO
231-
pub fn trySteamQuery(host: []const u8, port: u16) !?ProbeResult {
263+
pub fn trySteamQuery(host: []const u8, port: u16, timeout_ms: u32) !?ProbeResult {
232264
// A2S_INFO challenge: FF FF FF FF 54 "Source Engine Query\x00"
233265
const challenge = "\xff\xff\xff\xff\x54Source Engine Query\x00";
234266
var response_buf: [1400]u8 = undefined;
235267

236268
const start = std.time.Instant.now() catch return null;
237-
const n = udpExchange(host, port, challenge, &response_buf, 3000) catch return null;
269+
var n = udpExchange(host, port, challenge, &response_buf, timeout_ms) catch return null;
238270
const latency_ns = measureLatencyNs(start);
239271

240-
if (n < 6) return null;
241-
const data = response_buf[0..n];
272+
if (n < 5) return null;
273+
// Validate header: FF FF FF FF
274+
if (response_buf[0] != 0xFF or response_buf[1] != 0xFF or
275+
response_buf[2] != 0xFF or response_buf[3] != 0xFF) return null;
276+
277+
// A2S challenge handshake: most modern Source servers first reply with
278+
// 0x41 + a 4-byte challenge token; the real A2S_INFO (0x49) is only sent
279+
// after we re-send the query with that token appended. Without this the
280+
// probe silently failed against nearly all current Source servers.
281+
// One retry only — a malicious server can't loop us.
282+
if (response_buf[4] == 0x41) {
283+
if (n < 9) return null;
284+
var q2: [challenge.len + 4]u8 = undefined;
285+
@memcpy(q2[0..challenge.len], challenge);
286+
@memcpy(q2[challenge.len..], response_buf[5..9]);
287+
n = udpExchange(host, port, &q2, &response_buf, timeout_ms) catch return null;
288+
if (n < 5) return null;
289+
if (response_buf[0] != 0xFF or response_buf[1] != 0xFF or
290+
response_buf[2] != 0xFF or response_buf[3] != 0xFF) return null;
291+
}
242292

243-
// Validate header: FF FF FF FF 49 (type 'I' = 0x49)
244-
if (data[0] != 0xFF or data[1] != 0xFF or data[2] != 0xFF or data[3] != 0xFF)
245-
return null;
293+
const data = response_buf[0..n];
246294

247-
// Response type: 0x49 (A2S_INFO), 0x41 (challenge — need to re-query)
295+
// Response type must now be 0x49 (A2S_INFO)
248296
if (data[4] != 0x49) return null;
249297

250298
// Parse: protocol(1) | name(NUL) | map(NUL) | folder(NUL) | game(NUL) | ...
@@ -310,8 +358,8 @@ pub fn trySteamQuery(host: []const u8, port: u16) !?ProbeResult {
310358
///
311359
/// Performs the modern (1.7+) handshake + status request to retrieve
312360
/// the server description, version, and player count.
313-
pub fn tryMinecraftQuery(host: []const u8, port: u16) !?ProbeResult {
314-
var stream = tcpConnect(host, port, 3000) catch return null;
361+
pub fn tryMinecraftQuery(host: []const u8, port: u16, timeout_ms: u32) !?ProbeResult {
362+
var stream = tcpConnect(host, port, timeout_ms) catch return null;
315363
defer stream.close();
316364

317365
const start = std.time.Instant.now() catch return null;
@@ -364,9 +412,26 @@ pub fn tryMinecraftQuery(host: []const u8, port: u16) !?ProbeResult {
364412
// Send status request: length=1, packet_id=0x00
365413
_ = stream.write(&[_]u8{ 0x01, 0x00 }) catch return null;
366414

367-
// Read response
368-
var read_buf: [4096]u8 = undefined;
369-
const bytes_read = stream.read(&read_buf) catch return null;
415+
// Read the full framed response. A single read() truncates any MOTD large
416+
// enough to span multiple TCP segments (common with plugin lists / MiniMessage
417+
// formatting), corrupting the JSON. Loop until we have the whole
418+
// VarInt-length-prefixed packet, or the buffer/connection is exhausted.
419+
var read_buf: [16384]u8 = undefined;
420+
var bytes_read: usize = 0;
421+
var frame_total: ?usize = null; // length-varint bytes + declared packet length
422+
while (bytes_read < read_buf.len) {
423+
const got = stream.read(read_buf[bytes_read..]) catch break;
424+
if (got == 0) break; // EOF
425+
bytes_read += got;
426+
if (frame_total == null) {
427+
if (decodeVarInt(read_buf[0..bytes_read])) |dv| {
428+
frame_total = dv.value + dv.bytes;
429+
}
430+
}
431+
if (frame_total) |need| {
432+
if (bytes_read >= need) break;
433+
}
434+
}
370435
if (bytes_read < 5) return null;
371436

372437
const latency_ns = measureLatencyNs(start);
@@ -419,8 +484,8 @@ pub fn tryMinecraftQuery(host: []const u8, port: u16) !?ProbeResult {
419484
/// Sends an RCON authentication packet with an empty password; a valid
420485
/// RCON server will respond with an auth-response packet (even if auth
421486
/// fails), which confirms the protocol.
422-
pub fn tryRCON(host: []const u8, port: u16) !?ProbeResult {
423-
var stream = tcpConnect(host, port, 3000) catch return null;
487+
pub fn tryRCON(host: []const u8, port: u16, timeout_ms: u32) !?ProbeResult {
488+
var stream = tcpConnect(host, port, timeout_ms) catch return null;
424489
defer stream.close();
425490

426491
const start = std.time.Instant.now() catch return null;
@@ -463,8 +528,8 @@ pub fn tryRCON(host: []const u8, port: u16) !?ProbeResult {
463528

464529
/// HTTP probe — send a GET / and inspect the response for game-specific
465530
/// indicators in headers or body content.
466-
pub fn tryHTTPProbe(host: []const u8, port: u16) !?ProbeResult {
467-
var stream = tcpConnect(host, port, 3000) catch return null;
531+
pub fn tryHTTPProbe(host: []const u8, port: u16, timeout_ms: u32) !?ProbeResult {
532+
var stream = tcpConnect(host, port, timeout_ms) catch return null;
468533
defer stream.close();
469534

470535
const start = std.time.Instant.now() catch return null;
@@ -510,8 +575,8 @@ pub fn tryHTTPProbe(host: []const u8, port: u16) !?ProbeResult {
510575

511576
/// Raw TCP banner grab — connect and read whatever the server sends
512577
/// within the first 1024 bytes, then match against known patterns.
513-
pub fn tryTCPBanner(host: []const u8, port: u16) !?ProbeResult {
514-
var stream = tcpConnect(host, port, 3000) catch return null;
578+
pub fn tryTCPBanner(host: []const u8, port: u16, timeout_ms: u32) !?ProbeResult {
579+
var stream = tcpConnect(host, port, timeout_ms) catch return null;
515580
defer stream.close();
516581

517582
const start = std.time.Instant.now() catch return null;
@@ -561,22 +626,20 @@ pub fn tryTCPBanner(host: []const u8, port: u16) !?ProbeResult {
561626
/// 4. HTTP (TCP)
562627
/// 5. TCP banner grab
563628
pub fn probeSingle(host: []const u8, port: u16, timeout_ms: u32) !ProbeResult {
564-
_ = timeout_ms; // individual probes use their own timeouts
565-
566629
// Try Steam A2S_INFO first — widest coverage
567-
if (try trySteamQuery(host, port)) |r| return r;
630+
if (try trySteamQuery(host, port, timeout_ms)) |r| return r;
568631

569632
// Minecraft SLP
570-
if (try tryMinecraftQuery(host, port)) |r| return r;
633+
if (try tryMinecraftQuery(host, port, timeout_ms)) |r| return r;
571634

572635
// RCON handshake
573-
if (try tryRCON(host, port)) |r| return r;
636+
if (try tryRCON(host, port, timeout_ms)) |r| return r;
574637

575638
// HTTP/REST
576-
if (try tryHTTPProbe(host, port)) |r| return r;
639+
if (try tryHTTPProbe(host, port, timeout_ms)) |r| return r;
577640

578641
// Raw TCP
579-
if (try tryTCPBanner(host, port)) |r| return r;
642+
if (try tryTCPBanner(host, port, timeout_ms)) |r| return r;
580643

581644
return error.NoProtocolMatched;
582645
}
@@ -806,3 +869,32 @@ test "ProbeResult set and get" {
806869
try std.testing.expectEqualStrings("192.168.1.5", r.hostSlice());
807870
try std.testing.expectEqual(@as(u16, 25565), r.port);
808871
}
872+
873+
test "decodeVarInt: single, multi-byte, and incomplete" {
874+
// 0x00 -> 0 in one byte
875+
try std.testing.expectEqual(@as(usize, 0), decodeVarInt(&[_]u8{0x00}).?.value);
876+
try std.testing.expectEqual(@as(usize, 1), decodeVarInt(&[_]u8{0x00}).?.bytes);
877+
// 0x7F -> 127 in one byte
878+
try std.testing.expectEqual(@as(usize, 127), decodeVarInt(&[_]u8{0x7F}).?.value);
879+
// 0x80 0x01 -> 128 in two bytes (MC VarInt example)
880+
const two = decodeVarInt(&[_]u8{ 0x80, 0x01 }).?;
881+
try std.testing.expectEqual(@as(usize, 128), two.value);
882+
try std.testing.expectEqual(@as(usize, 2), two.bytes);
883+
// 0xDD 0xC7 0x01 -> 25565 in three bytes
884+
const three = decodeVarInt(&[_]u8{ 0xDD, 0xC7, 0x01 }).?;
885+
try std.testing.expectEqual(@as(usize, 25565), three.value);
886+
try std.testing.expectEqual(@as(usize, 3), three.bytes);
887+
// continuation bit set but no following byte -> incomplete
888+
try std.testing.expect(decodeVarInt(&[_]u8{0x80}) == null);
889+
try std.testing.expect(decodeVarInt("") == null);
890+
}
891+
892+
test "resolveAddress accepts IPv4 and IPv6 literals without DNS" {
893+
const v4 = try resolveAddress("127.0.0.1", 27015);
894+
try std.testing.expectEqual(@as(u16, posix.AF.INET), v4.any.family);
895+
try std.testing.expectEqual(@as(u16, 27015), v4.getPort());
896+
897+
const v6 = try resolveAddress("::1", 25565);
898+
try std.testing.expectEqual(@as(u16, posix.AF.INET6), v6.any.family);
899+
try std.testing.expectEqual(@as(u16, 25565), v6.getPort());
900+
}

src/interface/ffi/src/steam_client.zig

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ const main = @import("main.zig");
3333
// ═══════════════════════════════════════════════════════════════════════════════
3434

3535
const STEAM_API_BASE = "https://api.steampowered.com";
36+
// Intended per-request deadline. NOT yet enforced: Zig 0.15.2's
37+
// std.http.Client.fetch exposes no timeout/deadline knob, so a hung endpoint
38+
// blocks the calling thread. Enforcing it needs either a raw-socket HTTP client
39+
// (SO_RCVTIMEO + non-blocking connect, as probe.zig already does) or a watchdog
40+
// thread — tracked as a dedicated follow-up. Kept as the contract to honour.
3641
const STEAM_API_TIMEOUT_MS: u32 = 8_000;
3742

3843
/// Maximum length of a Steam64 ID in decimal string form (17 digits + NUL)

0 commit comments

Comments
 (0)