Remove goto/sprintf from NVRAM parsing

osquery/core/darwin/conversions.cpp

@@ -8,6 +8,8 @@
  *
  */
 
+#include <iomanip>
+
 #include <boost/lexical_cast.hpp>
 
 #include "osquery/core/conversions.h"
@@ -32,20 +34,28 @@ std::string stringFromCFString(const CFStringRef& cf_string) {
 
 std::string stringFromCFData(const CFDataRef& cf_data) {
   CFRange range = CFRangeMake(0, CFDataGetLength(cf_data));
+
   char* buffer = (char*)malloc(range.length + 1);
   memset(buffer, 0, range.length + 1);
 
+  std::stringstream result;
+  uint8_t byte;
   CFDataGetBytes(cf_data, range, (UInt8*)buffer);
   for (CFIndex i = 0; i < range.length; ++i) {
-    if (buffer[i] == 0) {
-      buffer[i] = ' ';
+    byte = buffer[i];
+    if (isprint(byte)) {
+      result << byte;
+    } else if (buffer[i] == 0) {
+      result << ' ';
+    } else {
+      result << '%' << std::setfill('0') << std::setw(2) << std::hex
+             << (int)byte;
     }
   }
 
   // Cleanup allocations.
-  std::string result(buffer);
   free(buffer);
-  return result;
+  return result.str();
 }
 
 std::string stringFromCFNumber(const CFDataRef& cf_number) {

osquery/tables/system/darwin/nvram.cpp

@@ -20,61 +20,6 @@
 namespace osquery {
 namespace tables {
 
-std::string variableFromNumber(const void *value) {
-  uint32_t number;
-  char number_buffer[10];
-
-  memset(number_buffer, 0, 10);
-  CFNumberGetValue((CFNumberRef)value, kCFNumberSInt32Type, &number);
-  if (number == 0xFFFFFFFF) {
-    sprintf(number_buffer, "-1");
-  } else if (number < 1000) {
-    sprintf(number_buffer, "%d", number);
-  } else {
-    sprintf(number_buffer, "0x%x", number);
-  }
-
-  return std::string(number_buffer);
-}
-
-std::string variableFromData(const void *value) {
-  std::string variable;
-
-  uint32_t length;
-  const uint8_t *data_ptr;
-  char *buffer = 0;
-  uint32_t count, count2;
-  uint8_t byte;
-
-  length = CFDataGetLength((CFDataRef)value);
-  if (length == 0) {
-    return "";
-  }
-
-  buffer = (char *)malloc(length * 3 + 1);
-  if (buffer == NULL) {
-    return "";
-  }
-
-  memset(buffer, 0, length * 3 + 1);
-  data_ptr = CFDataGetBytePtr((CFDataRef)value);
-  for (count = count2 = 0; count < length; count++) {
-    byte = data_ptr[count];
-    if (isprint(byte)) {
-      buffer[count2++] = byte;
-    } else {
-      sprintf(buffer + count2, "%%%02x", byte);
-      count2 += 3;
-    }
-  }
-
-  // Cleanup
-  variable = std::string(buffer);
-  free(buffer);
-
-  return variable;
-}
-
 void genVariable(const void *key, const void *value, void *results) {
   Row nvram_row;
   std::string value_string;
@@ -94,23 +39,19 @@ void genVariable(const void *key, const void *value, void *results) {
 
   // Based on the type, get a texual representation of the variable.
   if (type_id == CFBooleanGetTypeID()) {
-    // Boolean!
     value_string = (CFBooleanGetValue((CFBooleanRef)value)) ? "true" : "false";
   } else if (type_id == CFNumberGetTypeID()) {
-    // Number!
-    value_string = variableFromNumber(value);
+    value_string = stringFromCFNumber((CFDataRef)value);
   } else if (type_id == CFStringGetTypeID()) {
-    // CFString!
     value_string = stringFromCFString((CFStringRef)value);
   } else if (type_id == CFDataGetTypeID()) {
-    // Binary Data
-    value_string = variableFromData(value);
+    value_string = stringFromCFData((CFDataRef)value);
   } else {
-    // Who knows?
+    // Unknown result type, do not attempt to decode/format.
     value_string = "<INVALID>";
   }
 
-  // Finally, add the variable's value to the row.
+  // Finally, add the variable value to the row.
   nvram_row["value"] = value_string;
   ((QueryData *)results)->push_back(nvram_row);
 }
@@ -122,33 +63,30 @@ QueryData genNVRAM(QueryContext &context) {
   mach_port_t master_port;
   io_registry_entry_t options_ref;
 
-  status = IOMasterPort(bootstrap_port, &master_port);
-  if (status != KERN_SUCCESS) {
-    LOG(ERROR) << "Error getting the IOMaster port";
+  auto kr = IOMasterPort(bootstrap_port, &master_port);
+  if (kr != KERN_SUCCESS) {
+    VLOG(1) << "Could not get the IOMaster port";
     return {};
   }
 
   // NVRAM registry entry is :/options.
-  options_ref = IORegistryEntryFromPath(master_port, "IODeviceTree:/options");
-  if (options_ref == 0) {
-    LOG(ERROR) << "NVRAM is not supported on this system";
+  auto options = IORegistryEntryFromPath(master_port, "IODeviceTree:/options");
+  if (options == 0) {
+    VLOG(1) << "NVRAM is not supported on this system";
     return {};
   }
 
   CFMutableDictionaryRef options_dict;
-
-  status = IORegistryEntryCreateCFProperties(options_ref, &options_dict, 0, 0);
-  if (status != KERN_SUCCESS) {
-    LOG(ERROR) << "Error getting the firmware variables";
-    goto cleanup;
+  kr = IORegistryEntryCreateCFProperties(options, &options_dict, 0, 0);
+  if (kr != KERN_SUCCESS) {
+    VLOG(1) << "Could not get NVRAM properties";
+  } else {
+    CFDictionaryApplyFunction(options_dict, &genVariable, &results);
   }
 
-  CFDictionaryApplyFunction(options_dict, &genVariable, &results);
-
-cleanup:
   // Cleanup (registry entry context).
-  IOObjectRelease(options_ref);
   CFRelease(options_dict);
+  IOObjectRelease(options);
   return results;
 }
 }