[Mac] Make the local_discovery client more resilient to invalid UTF-8.

chrome/browser/local_discovery/service_discovery_client_mac.mm

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/local_discovery/service_discovery_client_mac.h"
 
 #import <arpa/inet.h>
 #import <Foundation/Foundation.h>
 #import <net/if_dl.h>
 #include <stddef.h>
 #include <stdint.h>
 
 #include "base/debug/dump_without_crashing.h"
 #include "base/memory/singleton.h"
 #include "base/metrics/histogram.h"
 #include "base/single_thread_task_runner.h"
+#include "base/strings/sys_string_conversions.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "net/base/ip_address.h"
 #include "net/base/ip_endpoint.h"
 
 using local_discovery::ServiceWatcherImplMac;
 using local_discovery::ServiceResolverImplMac;
 
 @interface NetServiceBrowserDelegate
     : NSObject<NSNetServiceBrowserDelegate, NSNetServiceDelegate> {
@@ skipping 26 matching lines @@
 const NSTimeInterval kResolveTimeout = 10.0;
 
 // Extracts the instance name, name type and domain from a full service name or
 // the service type and domain from a service type. Returns true if successful.
 // TODO(justinlin): This current only handles service names with format
 // <name>._<protocol2>._<protocol1>.<domain>. Service names with
 // subtypes will not parse correctly:
 // <name>._<type>._<sub>._<protocol2>._<protocol1>.<domain>.
 bool ExtractServiceInfo(const std::string& service,
                         bool is_service_name,
-                        std::string* instance,
-                        std::string* type,
-                        std::string* domain) {
+                        base::scoped_nsobject<NSString>* instance,
+                        base::scoped_nsobject<NSString>* type,
+                        base::scoped_nsobject<NSString>* domain) {
   if (service.empty())
     return false;
 
   const size_t last_period = service.find_last_of('.');
   if (last_period == std::string::npos || service.length() <= last_period)
     return false;
 
   if (!is_service_name) {
-    *instance = std::string();
-    *type = service.substr(0, last_period) + ".";
+    type->reset(base::SysUTF8ToNSString(service.substr(0, last_period) + "."),

[Vitaly Buka (NO REVIEWS), 2016/07/11 20:30:25]

Maybe safer to do all string transformations after conversion from UTF8?

[Robert Sesek, 2016/07/11 20:35:18]

I don't think it's any less safe to do it this way. The return value conveys
whether the conversions succeeded.

[Vitaly Buka (NO REVIEWS), 2016/07/11 20:48:24]

'safer' -> 'cleaner'

My concerns is that find_last_of for ascii char and utf8 string is not correct
in general. Although it should work for '.'

[Robert Sesek, 2016/07/11 20:56:08]

find_last_of isn't going to be searching code points though, it'll just go by
bytes. std::string generally doesn't  care about the validity of the bytes
within it.

+                base::scoped_policy::RETAIN);
   } else {
     // Find third last period that delimits type and instance name.
     size_t type_period = last_period;
     for (int i = 0; i < 2; ++i) {
       type_period = service.find_last_of('.', type_period - 1);
       if (type_period == std::string::npos)
         return false;
     }
 
-    *instance = service.substr(0, type_period);
-    *type = service.substr(type_period + 1, last_period - type_period);
+    instance->reset(base::SysUTF8ToNSString(service.substr(0, type_period)),
+                    base::scoped_policy::RETAIN);
+    type->reset(
+        base::SysUTF8ToNSString(
+            service.substr(type_period + 1, last_period - type_period)),
+        base::scoped_policy::RETAIN);
   }
-  *domain = service.substr(last_period + 1) + ".";
+  domain->reset(base::SysUTF8ToNSString(service.substr(last_period + 1) + "."),
+                base::scoped_policy::RETAIN);
 
-  return !domain->empty() &&
-         !type->empty() &&
-         (!is_service_name || !instance->empty());
+  return [*domain length] > 0 &&
+         [*type length] > 0 &&
+         (!is_service_name || [*instance length] > 0);
 }
 
 void ParseTxtRecord(NSData* record, std::vector<std::string>* output) {
-  if (record == nil || [record length] <= 1)
+  if ([record length] <= 1)
     return;
 
   VLOG(1) << "ParseTxtRecord: " << [record length];
 
-  const char* record_bytes = reinterpret_cast<const char*>([record bytes]);
-  const char* const record_end = record_bytes + [record length];
-  // TODO(justinlin): More strict bounds checking.
-  while (record_bytes < record_end) {
-    uint8_t size = *record_bytes++;
-    if (size <= 0)
-      continue;
+  const uint8_t* bytes = reinterpret_cast<const uint8_t*>([record bytes]);
+  size_t size = [record length];
+  size_t offset = 0;
+  while (offset < size) {
+    uint8_t record_size = bytes[offset++];
+    if (offset > size - record_size)
+      break;
 
-    if (record_bytes + size <= record_end) {
-      VLOG(1) << "TxtRecord: "
-              << std::string(record_bytes, static_cast<size_t>(size));
-      output->push_back(
-          [[[NSString alloc] initWithBytes:record_bytes
-                             length:size
-                             encoding:NSUTF8StringEncoding] UTF8String]);
+    base::scoped_nsobject<NSString> txt_record(
+        [[NSString alloc] initWithBytes:&bytes[offset]
+                                 length:record_size
+                               encoding:NSUTF8StringEncoding]);
+    if (txt_record) {
+      std::string txt_record_string = base::SysNSStringToUTF8(txt_record);
+      VLOG(1) << "TxtRecord: " << txt_record_string;
+      output->push_back(std::move(txt_record_string));
+    } else {
+      VLOG(1) << "TxtRecord corrupted at offset " << offset;
     }
-    record_bytes += size;
+
+    offset += record_size;
   }
 }
 
 }  // namespace
 
 ServiceDiscoveryClientMac::ServiceDiscoveryClientMac() {}
 ServiceDiscoveryClientMac::~ServiceDiscoveryClientMac() {}
 
 std::unique_ptr<ServiceWatcher> ServiceDiscoveryClientMac::CreateServiceWatcher(
     const std::string& service_type,
@@ skipping 69 matching lines @@
   DCHECK(IsOnServiceDiscoveryThread());
 
   delegate_.reset([[NetServiceBrowserDelegate alloc] initWithContainer:this]);
   browser_.reset([[NSNetServiceBrowser alloc] init]);
   [browser_ setDelegate:delegate_];
 }
 
 void
 ServiceWatcherImplMac::NetServiceBrowserContainer::DiscoverOnDiscoveryThread() {
   DCHECK(IsOnServiceDiscoveryThread());
-  std::string instance;
-  std::string type;
-  std::string domain;
+  base::scoped_nsobject<NSString> instance, type, domain;
 
   if (!ExtractServiceInfo(service_type_, false, &instance, &type, &domain))
     return;
 
-  DCHECK(instance.empty());
-  DVLOG(1) << "Listening for service type '" << type
-           << "' on domain '" << domain << "'";
+  DCHECK(![instance length]);
+  DVLOG(1) << "Listening for service type '" << [type UTF8String]
+           << "' on domain '" << [domain UTF8String] << "'";
 
   base::Time start_time = base::Time::Now();
-  [browser_ searchForServicesOfType:[NSString stringWithUTF8String:type.c_str()]
-            inDomain:[NSString stringWithUTF8String:domain.c_str()]];
+  [browser_ searchForServicesOfType:type inDomain:domain];
   UMA_HISTOGRAM_TIMES("LocalDiscovery.MacBrowseCallTimes",
                       base::Time::Now() - start_time);
 }
 
 void ServiceWatcherImplMac::NetServiceBrowserContainer::OnServicesUpdate(
     ServiceWatcher::UpdateType update,
     const std::string& service) {
   callback_runner_->PostTask(FROM_HERE, base::Bind(callback_, update, service));
 }
 
@@ skipping 70 matching lines @@
                  weak_factory_.GetWeakPtr()));
 }
 
 void ServiceResolverImplMac::NetServiceContainer::DeleteSoon() {
   service_discovery_runner_->DeleteSoon(FROM_HERE, this);
 }
 
 void
 ServiceResolverImplMac::NetServiceContainer::StartResolvingOnDiscoveryThread() {
   DCHECK(IsOnServiceDiscoveryThread());
-  std::string instance;
-  std::string type;
-  std::string domain;
+  base::scoped_nsobject<NSString> instance, type, domain;
 
   // The service object is set ahead of time by tests.
   if (service_)
     return;
 
   if (!ExtractServiceInfo(service_name_, true, &instance, &type, &domain))
     return OnResolveUpdate(ServiceResolver::STATUS_KNOWN_NONEXISTENT);
 
   VLOG(1) << "ServiceResolverImplMac::ServiceResolverImplMac::"
           << "StartResolvingOnDiscoveryThread: Success";
   delegate_.reset([[NetServiceDelegate alloc] initWithContainer:this]);
   service_.reset(
-      [[NSNetService alloc]
-          initWithDomain:[[NSString alloc] initWithUTF8String:domain.c_str()]
-          type:[[NSString alloc] initWithUTF8String:type.c_str()]
-          name:[[NSString alloc] initWithUTF8String:instance.c_str()]]);
+      [[NSNetService alloc] initWithDomain:domain type:type name:instance]);
   [service_ setDelegate:delegate_];
 
   [service_ resolveWithTimeout:kResolveTimeout];
 
   VLOG(1) << "ServiceResolverImplMac::NetServiceContainer::"
           << "StartResolvingOnDiscoveryThread: " << service_name_
-          << ", instance: " << instance << ", type: " << type
-          << ", domain: " << domain;
+          << ", instance: " << [instance UTF8String]
+          << ", type: " << [type UTF8String]
+          << ", domain: " << [domain UTF8String];
 }
 
 void ServiceResolverImplMac::NetServiceContainer::OnResolveUpdate(
     RequestStatus status) {
   if (status != STATUS_SUCCESS) {
     callback_runner_->PostTask(
         FROM_HERE, base::Bind(callback_, status, ServiceDescription()));
     return;
   }
 
@@ skipping 80 matching lines @@
 
 - (id)initWithContainer:
           (ServiceWatcherImplMac::NetServiceBrowserContainer*)container {
   if ((self = [super init])) {
     container_ = container;
     services_.reset([[NSMutableArray alloc] initWithCapacity:1]);
   }
   return self;
 }
 
-- (void)netServiceBrowser:(NSNetServiceBrowser *)netServiceBrowser
-        didFindService:(NSNetService *)netService
-        moreComing:(BOOL)moreServicesComing {
+- (void)netServiceBrowser:(NSNetServiceBrowser*)netServiceBrowser
+           didFindService:(NSNetService*)netService
+               moreComing:(BOOL)moreServicesComing {
   // Start monitoring this service for updates.
   [netService setDelegate:self];
   [netService startMonitoring];
   [services_ addObject:netService];
 
   container_->OnServicesUpdate(local_discovery::ServiceWatcher::UPDATE_ADDED,
-                               [[netService name] UTF8String]);
+                               base::SysNSStringToUTF8([netService name]));
 }
 
-- (void)netServiceBrowser:(NSNetServiceBrowser *)netServiceBrowser
-        didRemoveService:(NSNetService *)netService
-        moreComing:(BOOL)moreServicesComing {
+- (void)netServiceBrowser:(NSNetServiceBrowser*)netServiceBrowser
+         didRemoveService:(NSNetService*)netService
+               moreComing:(BOOL)moreServicesComing {
   NSUInteger index = [services_ indexOfObject:netService];
   if (index != NSNotFound) {
     container_->OnServicesUpdate(
         local_discovery::ServiceWatcher::UPDATE_REMOVED,
-        [[netService name] UTF8String]);
+        base::SysNSStringToUTF8([netService name]));
 
     // Stop monitoring this service for updates.
     [[services_ objectAtIndex:index] stopMonitoring];
     [services_ removeObjectAtIndex:index];
   }
 }
 
-- (void)netService:(NSNetService *)sender
-        didUpdateTXTRecordData:(NSData *)data {
+- (void)netService:(NSNetService*)sender
+    didUpdateTXTRecordData:(NSData*)data {
   container_->OnServicesUpdate(local_discovery::ServiceWatcher::UPDATE_CHANGED,
-                               [[sender name] UTF8String]);
+                               base::SysNSStringToUTF8([sender name]));
 }
 
 @end
 
 @implementation NetServiceDelegate
 
 - (id)initWithContainer:
           (ServiceResolverImplMac::NetServiceContainer*)container {
   if ((self = [super init])) {
     container_ = container;
   }
   return self;
 }
 
-- (void)netServiceDidResolveAddress:(NSNetService *)sender {
+- (void)netServiceDidResolveAddress:(NSNetService*)sender {
   container_->OnResolveUpdate(local_discovery::ServiceResolver::STATUS_SUCCESS);
 }
 
-- (void)netService:(NSNetService *)sender
-        didNotResolve:(NSDictionary *)errorDict {
+- (void)netService:(NSNetService*)sender
+        didNotResolve:(NSDictionary*)errorDict {
   container_->OnResolveUpdate(
       local_discovery::ServiceResolver::STATUS_REQUEST_TIMEOUT);
 }
 
 @end