From 11364b10e7d964a9bef96b26ca931053d52dcd59 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Wed, 5 Aug 2026 12:36:14 -0700 Subject: [PATCH] fix ipv6, @domain, mmdb byte swaps, geocache location --- build.zig | 12 ++++++ src/Config.zig | 12 +++++- src/location/GeoIp.zig | 29 ++++++------- src/location/resolver.zig | 86 ++++++++++++++++++++++++++++++++++----- 4 files changed, 113 insertions(+), 26 deletions(-) diff --git a/build.zig b/build.zig index 1e7d5d2..486dfe3 100644 --- a/build.zig +++ b/build.zig @@ -105,6 +105,18 @@ pub fn build(b: *std.Build) void { // With a byte array both sides agree (40 / 8). Nothing here reads // uint128-typed database fields, so this only affects layout. .MMDB_UINT128_IS_BYTE_ARRAY = 1, + // Tell libmaxminddb the target's byte order. + // + // Database floats and doubles are stored big-endian, and + // `get_ieee754_double` only reverses them when this is set; otherwise it + // memcpy's the raw bytes and returns a nonsense value on a little-endian + // host. Autotools normally sets this, so a hand-written config header + // that omits it silently produces garbage coordinates. + // + // Getting this right in the C layer means callers do not have to + // compensate, which they previously did with a byteswap that was correct + // only by accident on little-endian targets and wrong on big-endian ones. + .MMDB_LITTLE_ENDIAN = @as(u8, if (target.result.cpu.arch.endian() == .little) 1 else 0), }); maxminddb.root_module.addConfigHeader(maxminddb_config); diff --git a/src/Config.zig b/src/Config.zig index dfa4819..dc693cc 100644 --- a/src/Config.zig +++ b/src/Config.zig @@ -103,7 +103,17 @@ pub fn load(allocator: std.mem.Allocator, env: *const std.process.Environ.Map) ! 24; break :blk hours * std.time.s_per_hour; }, - .geocache_file = if (env.get("WTTR_GEOCACHE_FILE")) |v| try allocator.dupe(u8, v) else try std.fs.path.join(allocator, &[_][]const u8{ default_cache_dir, "geocache.json" }), + // Derived from WTTR_CACHE_DIR like every other cache path. It previously + // ignored it and always used the default location, so pointing the server + // at a different cache directory silently left this one file behind. + .geocache_file = blk: { + if (env.get("WTTR_GEOCACHE_FILE")) |v| { + break :blk try allocator.dupe(u8, v); + } + break :blk try std.fmt.allocPrint(allocator, "{s}/geocache.json", .{ + env.get("WTTR_CACHE_DIR") orelse default_cache_dir, + }); + }, .geoip_fallback = blk: { if (env.get("WTTR_GEOIP_FALLBACK")) |v| { if (std.mem.eql(u8, v, "ip2location")) break :blk .ip2location; diff --git a/src/location/GeoIp.zig b/src/location/GeoIp.zig index 6016bd9..4c21fa3 100644 --- a/src/location/GeoIp.zig +++ b/src/location/GeoIp.zig @@ -260,7 +260,11 @@ pub fn isUSIp(self: *GeoIP, ip: []const u8) bool { /// Maximum accuracy radius (in km) to trust from GeoLite2. Entries with a /// radius above this are too coarse for weather lookups (e.g. backbone/transit -/// IPs that MaxMind maps to the wrong city) and should fall back to IP2Location. +/// IPs that MaxMind maps to the wrong city) and should fall back to the +/// configured online provider. +/// +/// Note this only catches entries GeoLite2 admits are vague. An entry can be +/// precise *and* wrong, which is what pins exist for. const max_accuracy_radius_km = 200; fn extractCoordinates(self: *GeoIP, ip: []const u8, result: c.MMDB_lookup_result_s) ?Location { @@ -268,15 +272,15 @@ fn extractCoordinates(self: *GeoIP, ip: []const u8, result: c.MMDB_lookup_result var entry_copy = result.entry; - // Check accuracy_radius first -- reject low-confidence entries so we - // fall back to the IP2Location online lookup instead. + // Check accuracy_radius first -- reject low-confidence entries so we fall + // back to the configured online provider instead. // SAFETY: accuracy_data set by MMDB_get_value var accuracy_data: c.MMDB_entry_data_s = undefined; const acc_status = c.MMDB_get_value(&entry_copy, &accuracy_data, "location", "accuracy_radius", @as([*c]const u8, null)); if (acc_status == c.MMDB_SUCCESS and accuracy_data.has_data) { const radius = accuracy_data.unnamed_0.uint16; if (radius > max_accuracy_radius_km) { - log.info("GeoLite2 accuracy_radius for ip {s} is {d} km (>{d} km threshold), falling back to IP2Location", .{ ip, radius, max_accuracy_radius_km }); + log.info("GeoLite2 accuracy_radius for ip {s} is {d} km (>{d} km threshold), falling back to the online provider", .{ ip, radius, max_accuracy_radius_km }); return null; } } @@ -301,16 +305,13 @@ fn extractCoordinates(self: *GeoIP, ip: []const u8, result: c.MMDB_lookup_result return null; } - var coords = [_]f64{ latitude_data.unnamed_0.double_value, longitude_data.unnamed_0.double_value }; - - // Depending on how this is compiled, the byteswap may or may not be necessary - // original c, compiled with zig, statically linked: byteSwap - // pre=built, dynamically linked, do not byte swap - // I'm not sure precisely what causes this - std.mem.byteSwapAllElements(f64, &coords); - - const latitude = coords[0]; - const longitude = coords[1]; + // No byte swapping here: `build.zig` sets `MMDB_LITTLE_ENDIAN` for the target, + // so libmaxminddb converts the database's big-endian doubles to host order in + // `get_ieee754_double` before we ever see them. If that define were ever + // dropped, these coordinates would silently become nonsense, which the + // "lookup works" test below is positioned to catch. + const latitude = latitude_data.unnamed_0.double_value; + const longitude = longitude_data.unnamed_0.double_value; // Extract location name parts // SAFETY: value set by MMDB_get_value diff --git a/src/location/resolver.zig b/src/location/resolver.zig index d499cca..0358e13 100644 --- a/src/location/resolver.zig +++ b/src/location/resolver.zig @@ -3,6 +3,7 @@ const GeoIp = @import("GeoIp.zig"); const GeoCache = @import("GeoCache.zig"); const Airports = @import("Airports.zig"); const Coordinates = @import("../Coordinates.zig"); +const packIp = @import("Ip2location.zig").packIp; const log = std.log.scoped(.resolver); @@ -141,7 +142,7 @@ pub const Resolver = struct { switch (result) { .address => |addr| { var buf: [64]u8 = undefined; - const ip_str = try std.fmt.bufPrint(&buf, "{f}", .{addr}); + const ip_str = try formatAddressBare(addr, &buf); return self.resolveIP(ip_str); }, .canonical_name => continue, @@ -258,6 +259,27 @@ pub const Resolver = struct { return self.resolveGeocoded(code); } + /// Formats an address with no port and no brackets. + /// + /// Every formatter in `std.Io.net` appends ":port", and the IPv6 one also + /// brackets the address. Feeding that to `resolveIP` produced strings like + /// "1.2.3.4:0", which are not addresses, so *every* `@domain` lookup failed + /// with LocationNotFound. + fn formatAddressBare(addr: std.Io.net.IpAddress, buf: []u8) ![]const u8 { + return switch (addr) { + .ip4 => |a| std.fmt.bufPrint(buf, "{d}.{d}.{d}.{d}", .{ + a.bytes[0], a.bytes[1], a.bytes[2], a.bytes[3], + }), + .ip6 => |a| blk: { + const bare: std.Io.net.Ip6Address.Unresolved = .{ + .bytes = a.bytes, + .interface_name = null, + }; + break :blk std.fmt.bufPrint(buf, "{f}", .{&bare}); + }, + }; + } + fn isAlpha(s: []const u8) bool { for (s) |c| { if (!std.ascii.isAlphabetic(c)) return false; @@ -265,17 +287,17 @@ pub const Resolver = struct { return true; } + /// Whether `s` is an address we can look up. + /// + /// This counted dots and required exactly three, so it only ever recognized + /// IPv4. An IPv6 client address fell through to `.city_name` and was sent to + /// the geocoder as a place name, e.g. asking Nominatim to find a town called + /// "2001:4860:4860::8888". + /// + /// Deferring to `packIp` keeps this in step with what the rest of the + /// pipeline accepts, including IPv4-mapped IPv6 forms. fn isIPAddress(s: []const u8) bool { - // Simple check for IPv4 - var dots: u8 = 0; - for (s) |c| { - if (c == '.') { - dots += 1; - } else if (!std.ascii.isDigit(c)) { - return false; - } - } - return dots == 3; + return packIp(s) != null; } }; @@ -284,12 +306,54 @@ test "detect IP address" { try std.testing.expect(!Resolver.isIPAddress("not.an.ip")); } +test "detect IP address: IPv6 forms" { + // Previously these were not recognized as addresses at all, so a v6 client + // was handed to the geocoder as though it were the name of a place. + try std.testing.expect(Resolver.isIPAddress("2001:4860:4860::8888")); + try std.testing.expect(Resolver.isIPAddress("::1")); + try std.testing.expect(Resolver.isIPAddress("fe80::1")); + try std.testing.expect(Resolver.isIPAddress("::ffff:12.94.132.170")); +} + +test "detect IP address: rejects things that only look numeric" { + try std.testing.expect(!Resolver.isIPAddress("1.2.3")); + try std.testing.expect(!Resolver.isIPAddress("1.2.3.4.5")); + try std.testing.expect(!Resolver.isIPAddress("999.1.1.1")); + try std.testing.expect(!Resolver.isIPAddress("London")); + try std.testing.expect(!Resolver.isIPAddress("")); + // A range is not a query; pins parse CIDR, lookups do not. + try std.testing.expect(!Resolver.isIPAddress("12.94.132.0/24")); +} + +test "formatAddressBare omits the port and brackets" { + var buf: [64]u8 = undefined; + + const v4 = try std.Io.net.IpAddress.parse("12.94.132.170", 0); + try std.testing.expectEqualStrings("12.94.132.170", try Resolver.formatAddressBare(v4, &buf)); + + const v6 = try std.Io.net.IpAddress.parse("2001:4860:4860::8888", 0); + try std.testing.expectEqualStrings("2001:4860:4860::8888", try Resolver.formatAddressBare(v6, &buf)); +} + +test "formatAddressBare output is accepted back as an address" { + var buf: [64]u8 = undefined; + // The round trip is the property that matters: `resolveIP` re-parses this + // string, and the old formatting produced "1.2.3.4:0", which it rejected. + for ([_][]const u8{ "8.8.8.8", "2001:4860:4860::8888", "::1" }) |text| { + const addr = try std.Io.net.IpAddress.parse(text, 0); + const formatted = try Resolver.formatAddressBare(addr, &buf); + try std.testing.expect(Resolver.isIPAddress(formatted)); + } +} + test "detect location type" { try std.testing.expectEqual(LocationType.ip_address, Resolver.detectType("8.8.8.8")); try std.testing.expectEqual(LocationType.domain_name, Resolver.detectType("@github.com")); try std.testing.expectEqual(LocationType.special_location, Resolver.detectType("~Eiffel+Tower")); try std.testing.expectEqual(LocationType.airport_code, Resolver.detectType("muc")); try std.testing.expectEqual(LocationType.city_name, Resolver.detectType("London")); + try std.testing.expectEqual(LocationType.ip_address, Resolver.detectType("2001:4860:4860::8888")); + try std.testing.expectEqual(LocationType.ip_address, Resolver.detectType("::1")); } test "resolver init" {