From 84f480fdd1cd7479f1390d93586b363b1bb15b40 Mon Sep 17 00:00:00 2001 From: Nick Bourdakos Date: Mon, 1 May 2023 14:19:12 -0700 Subject: [PATCH] Fix IP address bug where 4 byte version wasn't converted to dotted decimal form Introduces a tested `GNCIPAddress` class to make it more explicit of which IP address format is being used at call sites to prevent similar issues in the future. PiperOrigin-RevId: 528572993 --- internal/platform/implementation/apple/BUILD | 2 + .../implementation/apple/Mediums/BUILD | 2 + .../apple/Mediums/WiFiLAN/GNCIPv4Address.h | 60 +++++++++ .../apple/Mediums/WiFiLAN/GNCIPv4Address.m | 59 +++++++++ .../apple/Mediums/WiFiLAN/GNCWiFiLANMedium.h | 5 +- .../apple/Mediums/WiFiLAN/GNCWiFiLANMedium.m | 7 +- .../Mediums/WiFiLAN/GNCWiFiLANServerSocket.h | 5 +- .../Mediums/WiFiLAN/GNCWiFiLANServerSocket.m | 20 +-- .../platform/implementation/apple/Tests/BUILD | 9 +- .../apple/Tests/GNCIPAddressTest.mm | 114 ++++++++++++++++++ .../platform/implementation/apple/wifi_lan.mm | 12 +- 11 files changed, 272 insertions(+), 23 deletions(-) create mode 100644 internal/platform/implementation/apple/Mediums/WiFiLAN/GNCIPv4Address.h create mode 100644 internal/platform/implementation/apple/Mediums/WiFiLAN/GNCIPv4Address.m create mode 100644 internal/platform/implementation/apple/Tests/GNCIPAddressTest.mm diff --git a/internal/platform/implementation/apple/BUILD b/internal/platform/implementation/apple/BUILD index a5d76eba..b52e1b0e 100644 --- a/internal/platform/implementation/apple/BUILD +++ b/internal/platform/implementation/apple/BUILD @@ -65,8 +65,10 @@ objc_library( "//third_party/apple_frameworks:Foundation", "//third_party/apple_frameworks:Network", "//third_party/objective_c/google_toolbox_for_mac:GTM_Logger", + "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/strings", + "@com_google_absl//absl/synchronization", "@com_google_absl//absl/time", "@com_google_absl//absl/types:optional", "@nlohmann_json//:json", diff --git a/internal/platform/implementation/apple/Mediums/BUILD b/internal/platform/implementation/apple/Mediums/BUILD index 649bd04f..e5ec3b9f 100644 --- a/internal/platform/implementation/apple/Mediums/BUILD +++ b/internal/platform/implementation/apple/Mediums/BUILD @@ -25,6 +25,7 @@ objc_library( "GNCLeaks.h", "GNCLeaks.m", "GNCMConnection.m", + "WiFiLAN/GNCIPv4Address.m", "WiFiLAN/GNCWiFiLANError.m", "WiFiLAN/GNCWiFiLANMedium.m", "WiFiLAN/GNCWiFiLANServerSocket.m", @@ -37,6 +38,7 @@ objc_library( "Ble/GNCMBlePeripheral.h", "Ble/GNCMBleUtils.h", "GNCMConnection.h", + "WiFiLAN/GNCIPv4Address.h", "WiFiLAN/GNCWiFiLANError.h", "WiFiLAN/GNCWiFiLANMedium.h", "WiFiLAN/GNCWiFiLANServerSocket.h", diff --git a/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCIPv4Address.h b/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCIPv4Address.h new file mode 100644 index 00000000..6ffce1cc --- /dev/null +++ b/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCIPv4Address.h @@ -0,0 +1,60 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#import + +/** A container for IPv4 address information. */ +@interface GNCIPv4Address : NSObject + +/** + * @remark init is not an available initializer. + */ +- (nonnull instancetype)init NS_UNAVAILABLE; + +/** + * Creates a container for IPv4 address information. + * + * @param byte1 The first byte of the IP address. + * @param byte2 The second byte of the IP address. + * @param byte3 The third byte of the IP address. + * @param byte4 The forth byte of the IP address. + */ +- (nonnull instancetype)initWithByte1:(uint8_t)byte1 + byte2:(uint8_t)byte2 + byte3:(uint8_t)byte3 + byte4:(uint8_t)byte4 NS_DESIGNATED_INITIALIZER; + ++ (nonnull instancetype)addressFromFourByteInt:(uint32_t)address; + ++ (nonnull instancetype)addressFromData:(nonnull NSData *)address; + +/** The first byte of the IP address. */ +@property(nonatomic, readonly) uint8_t byte1; + +/** The second byte of the IP address. */ +@property(nonatomic, readonly) uint8_t byte2; + +/** The third byte of the IP address. */ +@property(nonatomic, readonly) uint8_t byte3; + +/** The forth byte of the IP address. */ +@property(nonatomic, readonly) uint8_t byte4; + +/** The 4 byte binary representation for the IPv4 address. */ +@property(nonatomic, nonnull, readonly) NSData *binaryRepresentation; + +/** The human readable dotted representation for the IPv4 address. */ +@property(nonatomic, nonnull, readonly) NSString *dottedRepresentation; + +@end diff --git a/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCIPv4Address.m b/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCIPv4Address.m new file mode 100644 index 00000000..3333dda7 --- /dev/null +++ b/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCIPv4Address.m @@ -0,0 +1,59 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#import "internal/platform/implementation/apple/Mediums/WiFiLAN/GNCIPv4Address.h" + +@implementation GNCIPv4Address + +- (instancetype)initWithByte1:(uint8_t)byte1 + byte2:(uint8_t)byte2 + byte3:(uint8_t)byte3 + byte4:(uint8_t)byte4 { + self = [super init]; + if (self) { + _byte1 = byte1; + _byte2 = byte2; + _byte3 = byte3; + _byte4 = byte4; + } + return self; +} + ++ (instancetype)addressFromFourByteInt:(uint32_t)address { + uint8_t byte1 = (address >> (8 * 0)) & 0xff; + uint8_t byte2 = (address >> (8 * 1)) & 0xff; + uint8_t byte3 = (address >> (8 * 2)) & 0xff; + uint8_t byte4 = (address >> (8 * 3)) & 0xff; + return [[GNCIPv4Address alloc] initWithByte1:byte1 byte2:byte2 byte3:byte3 byte4:byte4]; +} + ++ (instancetype)addressFromData:(NSData *)address { + NSAssert(address.length == 4, @"Address must be 4 bytes"); + const uint8_t *bytes = address.bytes; + return [[GNCIPv4Address alloc] initWithByte1:bytes[0] + byte2:bytes[1] + byte3:bytes[2] + byte4:bytes[3]]; +} + +- (NSData *)binaryRepresentation { + const uint8_t bytes[] = {_byte1, _byte2, _byte3, _byte4}; + return [NSData dataWithBytes:bytes length:4]; +} + +- (NSString *)dottedRepresentation { + return [NSString stringWithFormat:@"%d.%d.%d.%d", _byte1, _byte2, _byte3, _byte4]; +} + +@end diff --git a/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANMedium.h b/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANMedium.h index a6918f5a..9ac55431 100644 --- a/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANMedium.h +++ b/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANMedium.h @@ -14,6 +14,7 @@ #import +@class GNCIPv4Address; @class GNCWiFiLANServerSocket; @class GNCWiFiLANSocket; @@ -102,12 +103,12 @@ typedef void (^ServiceUpdateHandler)(NSString *_Nonnull serviceName, /** * Connects to an IP address and port. * - * @param host The 4 byte binary representation IPv4 address to connect to. + * @param host The IPv4 address to connect to. * @param port The port to connect to. * @param[out] error Error that will be populated on failure. * @return Returns a connected socket or nil if an error has occured. */ -- (nullable GNCWiFiLANSocket *)connectToHost:(nonnull NSString *)host +- (nullable GNCWiFiLANSocket *)connectToHost:(nonnull GNCIPv4Address *)host port:(NSInteger)port error:(NSError **_Nullable)error; diff --git a/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANMedium.m b/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANMedium.m index 202a95df..b1783208 100644 --- a/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANMedium.m +++ b/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANMedium.m @@ -17,6 +17,7 @@ #import #import +#import "internal/platform/implementation/apple/Mediums/WiFiLAN/GNCIPv4Address.h" #import "internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANError.h" #import "internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANServerSocket+Internal.h" #import "internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANServerSocket.h" @@ -258,9 +259,11 @@ NSDictionary *GNCTXTRecordForBrowseResult(nw_browse_resu return [self connectToEndpoint:endpoint error:error]; } -- (GNCWiFiLANSocket *)connectToHost:(NSString *)host port:(NSInteger)port error:(NSError **)error { +- (GNCWiFiLANSocket *)connectToHost:(GNCIPv4Address *)host + port:(NSInteger)port + error:(NSError **)error { nw_endpoint_t endpoint = - nw_endpoint_create_host([host UTF8String], [[@(port) stringValue] UTF8String]); + nw_endpoint_create_host(host.dottedRepresentation.UTF8String, @(port).stringValue.UTF8String); return [self connectToEndpoint:endpoint error:error]; } diff --git a/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANServerSocket.h b/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANServerSocket.h index 9acbc1c7..278193f5 100644 --- a/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANServerSocket.h +++ b/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANServerSocket.h @@ -14,6 +14,7 @@ #import +@class GNCIPv4Address; @class GNCWiFiLANSocket; @interface GNCWiFiLANServerSocket : NSObject @@ -31,9 +32,9 @@ - (nonnull instancetype)initWithPort:(NSInteger)port NS_DESIGNATED_INITIALIZER; /** - * The 4 byte binary representation for the IPv4 address of the physical network interface. + * The IPv4 address of the physical network interface. */ -@property(nonatomic, readonly, copy) NSString *ipAddress; +@property(nonatomic, readonly, copy) GNCIPv4Address *ipAddress; /** * The port of the server socket. diff --git a/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANServerSocket.m b/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANServerSocket.m index d4163457..66a6bac8 100644 --- a/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANServerSocket.m +++ b/internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANServerSocket.m @@ -22,6 +22,7 @@ #include #include +#import "internal/platform/implementation/apple/Mediums/WiFiLAN/GNCIPv4Address.h" #import "internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANError.h" #import "internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANServerSocket+Internal.h" #import "internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANSocket.h" @@ -71,7 +72,7 @@ @synthesize ipAddress = _ipAddress; -- (NSString *)ipAddress { +- (GNCIPv4Address *)ipAddress { if (!_ipAddress) { _ipAddress = [GNCWiFiLANServerSocket lookupIpAddress]; } @@ -235,7 +236,7 @@ * Returns the IP address as a 4 byte string. If not available, this returns an empty string to * align with the Windows implementation. */ -+ (NSString *)lookupIpAddress { ++ (GNCIPv4Address *)lookupIpAddress { struct ifaddrs *ifaddr; // Note: The data returned by `getifaddrs()` is dynamically allocated and should be freed using @@ -243,7 +244,7 @@ // // See: https://linux.die.net/man/3/getifaddrs if (getifaddrs(&ifaddr) == -1) { - return @""; + return [GNCIPv4Address addressFromFourByteInt:0]; } // Walk through linked list, maintaining head pointer so we can free list later. @@ -264,22 +265,13 @@ continue; } - // Break the 4 byte binary representation of the hostname into 4 separate bytes. uint32_t host = ((struct sockaddr_in *)address)->sin_addr.s_addr; - uint8_t byte1 = (host >> (8 * 0)) & 0xff; - uint8_t byte2 = (host >> (8 * 1)) & 0xff; - uint8_t byte3 = (host >> (8 * 2)) & 0xff; - uint8_t byte4 = (host >> (8 * 3)) & 0xff; - - // Join the bytes into a 4 character string. - NSString *hostString = [NSString stringWithFormat:@"%c%c%c%c", byte1, byte2, byte3, byte4]; - freeifaddrs(ifaddr); - return hostString; + return [GNCIPv4Address addressFromFourByteInt:host]; } freeifaddrs(ifaddr); - return @""; + return [GNCIPv4Address addressFromFourByteInt:0]; } @end diff --git a/internal/platform/implementation/apple/Tests/BUILD b/internal/platform/implementation/apple/Tests/BUILD index 21c91daf..16e7ef0d 100644 --- a/internal/platform/implementation/apple/Tests/BUILD +++ b/internal/platform/implementation/apple/Tests/BUILD @@ -26,6 +26,7 @@ objc_library( "GNCBleTest.mm", "GNCBluetoothAdapterTest.mm", "GNCCryptoTest.mm", + "GNCIPAddressTest.mm", "GNCMultiThreadExecutorTest.mm", "GNCScheduledExecutorTest.mm", "GNCSingleThreadExecutorTest.mm", @@ -33,9 +34,15 @@ objc_library( ], features = ["-layering_check"], deps = [ + "//internal/platform:base", + "//internal/platform/implementation:comm", + "//internal/platform/implementation:platform", + "//internal/platform/implementation:types", "//internal/platform/implementation/apple", - "//internal/platform/implementation/apple:Shared", + "//internal/platform/implementation/apple/Mediums", + "//third_party/apple_frameworks:Foundation", "//third_party/apple_frameworks:XCTest", + "@com_google_absl//absl/time", ], ) diff --git a/internal/platform/implementation/apple/Tests/GNCIPAddressTest.mm b/internal/platform/implementation/apple/Tests/GNCIPAddressTest.mm new file mode 100644 index 00000000..65166717 --- /dev/null +++ b/internal/platform/implementation/apple/Tests/GNCIPAddressTest.mm @@ -0,0 +1,114 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#import +#import + +#include + +#import "internal/platform/implementation/apple/Mediums/WiFiLAN/GNCIPv4Address.h" + +@interface GNCIPv4AddressTest : XCTestCase +@end + +@implementation GNCIPv4AddressTest + +- (void)testInitFromIntWithZero { + GNCIPv4Address *address = [GNCIPv4Address addressFromFourByteInt:0]; + XCTAssertEqual(address.byte1, 0); + XCTAssertEqual(address.byte2, 0); + XCTAssertEqual(address.byte3, 0); + XCTAssertEqual(address.byte4, 0); +} + +- (void)testInitFromIntWithRealIP { + GNCIPv4Address *address = [GNCIPv4Address addressFromFourByteInt:2869012672]; + XCTAssertEqual(address.byte1, 192); + XCTAssertEqual(address.byte2, 168); + XCTAssertEqual(address.byte3, 1); + XCTAssertEqual(address.byte4, 171); +} + +- (void)testInitFromDataWithZero { + std::string addressString = "\0\0\0\0"; + NSData *addressData = [NSData dataWithBytes:addressString.data() length:4]; + GNCIPv4Address *address = [GNCIPv4Address addressFromData:addressData]; + XCTAssertEqual(address.byte1, 0); + XCTAssertEqual(address.byte2, 0); + XCTAssertEqual(address.byte3, 0); + XCTAssertEqual(address.byte4, 0); +} + +- (void)testInitFromDataWithLeadingEmptyBytes { + std::string addressString = "\0\0\0\0"; + addressString[3] = static_cast(6); + + NSData *addressData = [NSData dataWithBytes:addressString.data() length:4]; + GNCIPv4Address *address = [GNCIPv4Address addressFromData:addressData]; + XCTAssertEqual(address.byte1, 0); + XCTAssertEqual(address.byte2, 0); + XCTAssertEqual(address.byte3, 0); + XCTAssertEqual(address.byte4, 6); +} + +- (void)testInitFromDataWithRealIP { + std::string addressString = "\0\0\0\0"; + addressString[0] = static_cast(192); + addressString[1] = static_cast(168); + addressString[2] = static_cast(1); + addressString[3] = static_cast(171); + + NSData *addressData = [NSData dataWithBytes:addressString.data() length:4]; + GNCIPv4Address *address = [GNCIPv4Address addressFromData:addressData]; + XCTAssertEqual(address.byte1, 192); + XCTAssertEqual(address.byte2, 168); + XCTAssertEqual(address.byte3, 1); + XCTAssertEqual(address.byte4, 171); +} + +- (void)testInitFromDataThrowsForWrongByteSize { + std::string addressString = "\0"; + NSData *addressData = [NSData dataWithBytes:addressString.data() length:addressString.size()]; + XCTAssertThrows([GNCIPv4Address addressFromData:addressData]); +} + +- (void)testDottedRepressentationWithZero { + GNCIPv4Address *address = [[GNCIPv4Address alloc] initWithByte1:0 byte2:0 byte3:0 byte4:0]; + XCTAssertEqualObjects(address.dottedRepresentation, @"0.0.0.0"); +} + +- (void)testDottedRepressentationWithRealIP { + GNCIPv4Address *address = [[GNCIPv4Address alloc] initWithByte1:192 byte2:168 byte3:1 byte4:171]; + XCTAssertEqualObjects(address.dottedRepresentation, @"192.168.1.171"); +} + +- (void)testBinaryRepressentationWithZero { + GNCIPv4Address *address = [[GNCIPv4Address alloc] initWithByte1:0 byte2:0 byte3:0 byte4:0]; + const uint8_t *bytes = (uint8_t *)address.binaryRepresentation.bytes; + XCTAssertEqual(bytes[0], 0); + XCTAssertEqual(bytes[1], 0); + XCTAssertEqual(bytes[2], 0); + XCTAssertEqual(bytes[3], 0); +} + +- (void)testBinaryRepressentationWithRealIP { + GNCIPv4Address *address = [[GNCIPv4Address alloc] initWithByte1:192 byte2:168 byte3:1 byte4:171]; + const uint8_t *bytes = (uint8_t *)address.binaryRepresentation.bytes; + XCTAssertEqual(bytes[0], 192); + XCTAssertEqual(bytes[1], 168); + XCTAssertEqual(bytes[2], 1); + XCTAssertEqual(bytes[3], 171); +} + +@end diff --git a/internal/platform/implementation/apple/wifi_lan.mm b/internal/platform/implementation/apple/wifi_lan.mm index 7cadd50a..5f2cc663 100644 --- a/internal/platform/implementation/apple/wifi_lan.mm +++ b/internal/platform/implementation/apple/wifi_lan.mm @@ -13,11 +13,13 @@ // limitations under the License. #import "internal/platform/implementation/apple/wifi_lan.h" +#import #include #include #include +#import "internal/platform/implementation/apple/Mediums/WiFiLAN/GNCIPv4Address.h" #import "internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANMedium.h" #import "internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANServerSocket.h" #import "internal/platform/implementation/apple/Mediums/WiFiLAN/GNCWiFiLANSocket.h" @@ -94,7 +96,8 @@ WifiLanServerSocket::WifiLanServerSocket(GNCWiFiLANServerSocket* server_socket) : server_socket_(server_socket) {} std::string WifiLanServerSocket::GetIPAddress() const { - return [server_socket_.ipAddress UTF8String]; + NSData* addressData = server_socket_.ipAddress.binaryRepresentation; + return std::string((char*)addressData.bytes, addressData.length); } int WifiLanServerSocket::GetPort() const { return server_socket_.port; } @@ -204,7 +207,12 @@ std::unique_ptr WifiLanMedium::ConnectToService( std::unique_ptr WifiLanMedium::ConnectToService( const std::string& ip_address, int port, CancellationFlag* cancellation_flag) { NSError* error = nil; - NSString* host = @(ip_address.c_str()); + if (ip_address.size() != 4) { + GTMLoggerError(@"Error IP address must be 4 bytes, but is %lu bytes", ip_address.size()); + return nil; + } + NSData* hostData = [NSData dataWithBytes:ip_address.data() length:ip_address.size()]; + GNCIPv4Address* host = [GNCIPv4Address addressFromData:hostData]; GNCWiFiLANSocket* socket = [medium_ connectToHost:host port:port error:&error]; if (socket != nil) { return std::make_unique(socket);