Fix use after free and leak in get_arg_values

Previously get_arg_values was returning a vector of uint64_t values
that could be passed directly to printf(3). For string values
get_arg_values was returning a pointer to a char*. For some cases it
was attempting to handle freeing the char* memory via a stack
allocated std::vector. Unfortunately, this was stack allocated in
get_arg_values so the char* data would get freed before it was used in
the subsequent call to printf().

In other cases get_arg_values was not freeing char* values and was
leaking memory (probe, stack, and ustack).

get_arg_values() now returns a vector of objects of type IPrintable
instead of uint64_t values. Each object has a method .value() that
returns the uint64_t value usable by printf(). For strings this allows
us to keep around the original std::string until after we've called
printf(), so we don't need to strdup() anymore.

Fixes #194

src/bpftrace.cpp

@@ -21,6 +21,7 @@
 #include "bpforc.h"
 #include "bpftrace.h"
 #include "attached_probe.h"
+#include "printf.h"
 #include "triggers.h"
 #include "resolve_cgroupid.h"
 
@@ -263,7 +264,7 @@ void perf_event_printer(void *cb_cookie, void *data, int size)
     auto id = printf_id - asyncactionint(AsyncAction::syscall);
     auto fmt = std::get<0>(bpftrace->system_args_[id]).c_str();
     auto args = std::get<1>(bpftrace->system_args_[id]);
-    std::vector<uint64_t> arg_values = bpftrace->get_arg_values(args, arg_data);
+    auto arg_values = bpftrace->get_arg_values(args, arg_data);
 
     char buffer [255];
 
@@ -273,30 +274,30 @@ void perf_event_printer(void *cb_cookie, void *data, int size)
         system(fmt);
         break;
       case 1:
-        snprintf(buffer, 255, fmt, arg_values.at(0));
+        snprintf(buffer, 255, fmt, arg_values.at(0)->value());
         system(buffer);
         break;
       case 2:
-        snprintf(buffer, 255, fmt, arg_values.at(0), arg_values.at(1));
+        snprintf(buffer, 255, fmt, arg_values.at(0)->value(), arg_values.at(1)->value());
         system(buffer);
         break;
       case 3:
-        snprintf(buffer, 255, fmt, arg_values.at(0), arg_values.at(1), arg_values.at(2));
+        snprintf(buffer, 255, fmt, arg_values.at(0)->value(), arg_values.at(1)->value(), arg_values.at(2)->value());
         system(buffer);
         break;
       case 4:
-        snprintf(buffer, 255, fmt, arg_values.at(0), arg_values.at(1), arg_values.at(2),
-          arg_values.at(3));
+        snprintf(buffer, 255, fmt, arg_values.at(0)->value(), arg_values.at(1)->value(), arg_values.at(2)->value(),
+          arg_values.at(3)->value());
         system(buffer);
         break;
       case 5:
-        snprintf(buffer, 255, fmt, arg_values.at(0), arg_values.at(1), arg_values.at(2),
-          arg_values.at(3), arg_values.at(4));
+        snprintf(buffer, 255, fmt, arg_values.at(0)->value(), arg_values.at(1)->value(), arg_values.at(2)->value(),
+          arg_values.at(3)->value(), arg_values.at(4)->value());
         system(buffer);
         break;
       case 6:
-        snprintf(buffer, 255, fmt, arg_values.at(0), arg_values.at(1), arg_values.at(2),
-          arg_values.at(3), arg_values.at(4), arg_values.at(5));
+        snprintf(buffer, 255, fmt, arg_values.at(0)->value(), arg_values.at(1)->value(), arg_values.at(2)->value(),
+          arg_values.at(3)->value(), arg_values.at(4)->value(), arg_values.at(5)->value());
         system(buffer);
         break;
       default:
@@ -309,7 +310,7 @@ void perf_event_printer(void *cb_cookie, void *data, int size)
   // printf
   auto fmt = std::get<0>(bpftrace->printf_args_[printf_id]).c_str();
   auto args = std::get<1>(bpftrace->printf_args_[printf_id]);
-  std::vector<uint64_t> arg_values = bpftrace->get_arg_values(args, arg_data);
+  auto arg_values = bpftrace->get_arg_values(args, arg_data);
 
   switch (args.size())
   {
@@ -317,38 +318,35 @@ void perf_event_printer(void *cb_cookie, void *data, int size)
       printf(fmt);
       break;
     case 1:
-      printf(fmt, arg_values.at(0));
+      printf(fmt, arg_values.at(0)->value());
       break;
     case 2:
-      printf(fmt, arg_values.at(0), arg_values.at(1));
+      printf(fmt, arg_values.at(0)->value(), arg_values.at(1)->value());
       break;
     case 3:
-      printf(fmt, arg_values.at(0), arg_values.at(1), arg_values.at(2));
+      printf(fmt, arg_values.at(0)->value(), arg_values.at(1)->value(), arg_values.at(2)->value());
       break;
     case 4:
-      printf(fmt, arg_values.at(0), arg_values.at(1), arg_values.at(2),
-          arg_values.at(3));
+      printf(fmt, arg_values.at(0)->value(), arg_values.at(1)->value(), arg_values.at(2)->value(),
+        arg_values.at(3)->value());
       break;
     case 5:
-      printf(fmt, arg_values.at(0), arg_values.at(1), arg_values.at(2),
-          arg_values.at(3), arg_values.at(4));
+      printf(fmt, arg_values.at(0)->value(), arg_values.at(1)->value(), arg_values.at(2)->value(),
+        arg_values.at(3)->value(), arg_values.at(4)->value());
       break;
     case 6:
-      printf(fmt, arg_values.at(0), arg_values.at(1), arg_values.at(2),
-          arg_values.at(3), arg_values.at(4), arg_values.at(5));
+      printf(fmt, arg_values.at(0)->value(), arg_values.at(1)->value(), arg_values.at(2)->value(),
+        arg_values.at(3)->value(), arg_values.at(4)->value(), arg_values.at(5)->value());
       break;
     default:
       abort();
   }
 }
 
-std::vector<uint64_t> BPFtrace::get_arg_values(std::vector<Field> args, uint8_t* arg_data)
+std::vector<std::unique_ptr<IPrintable>> BPFtrace::get_arg_values(std::vector<Field> args, uint8_t* arg_data)
 {
-  std::vector<uint64_t> arg_values;
-  std::vector<std::unique_ptr<char>> resolved_symbols;
-  std::vector<std::unique_ptr<char>> resolved_usernames;
+  std::vector<std::unique_ptr<IPrintable>> arg_values;
 
-  char *name;
   for (auto arg : args)
   {
     switch (arg.type.type)
@@ -357,54 +355,52 @@ std::vector<uint64_t> BPFtrace::get_arg_values(std::vector<Field> args, uint8_t*
         switch (arg.type.size)
         {
           case 8:
-            arg_values.push_back(*(uint64_t*)(arg_data+arg.offset));
+            arg_values.emplace_back(new PrintableInt(*(uint64_t*)(arg_data+arg.offset)));
             break;
           case 4:
-            arg_values.push_back(*(uint32_t*)(arg_data+arg.offset));
+            arg_values.emplace_back(new PrintableInt(*(uint32_t*)(arg_data+arg.offset)));
             break;
           case 2:
-            arg_values.push_back(*(uint16_t*)(arg_data+arg.offset));
+            arg_values.emplace_back(new PrintableInt(*(uint16_t*)(arg_data+arg.offset)));
             break;
           case 1:
-            arg_values.push_back(*(uint8_t*)(arg_data+arg.offset));
+            arg_values.emplace_back(new PrintableInt(*(uint8_t*)(arg_data+arg.offset)));
             break;
           default:
             abort();
         }
         break;
       case Type::string:
-        arg_values.push_back((uint64_t)(arg_data+arg.offset));
+        arg_values.emplace_back(new PrintableString(
+          std::string((char *) arg_data+arg.offset)));
         break;
       case Type::sym:
-        resolved_symbols.emplace_back(strdup(
+        arg_values.emplace_back(new PrintableString(
           resolve_sym(*(uint64_t*)(arg_data+arg.offset)).c_str()));
-        arg_values.push_back((uint64_t)resolved_symbols.back().get());
         break;
       case Type::usym:
-        resolved_symbols.emplace_back(strdup(
+        arg_values.emplace_back(new PrintableString(
           resolve_usym(*(uint64_t*)(arg_data+arg.offset), *(uint64_t*)(arg_data+arg.offset + 8)).c_str()));
-        arg_values.push_back((uint64_t)resolved_symbols.back().get());
         break;
       case Type::inet:
         name = strdup(resolve_inet(*(uint64_t*)(arg_data+arg.offset), *(uint64_t*)(arg_data+arg.offset+8)).c_str());
         arg_values.push_back((uint64_t)name);
         break;
       case Type::username:
-        resolved_usernames.emplace_back(strdup(
+        arg_values.emplace_back(new PrintableString(
           resolve_uid(*(uint64_t*)(arg_data+arg.offset)).c_str()));
-        arg_values.push_back((uint64_t)resolved_usernames.back().get());
         break;
       case Type::probe:
-        name = strdup(resolve_probe(*(uint64_t*)(arg_data+arg.offset)).c_str());
-        arg_values.push_back((uint64_t)name);
+        arg_values.emplace_back(new PrintableString(
+          resolve_probe(*(uint64_t*)(arg_data+arg.offset)).c_str()));
         break;
       case Type::stack:
-        name = strdup(get_stack(*(uint64_t*)(arg_data+arg.offset), false, 8).c_str());
-        arg_values.push_back((uint64_t)name);
+        arg_values.emplace_back(new PrintableString(
+          get_stack(*(uint64_t*)(arg_data+arg.offset), false, 8).c_str()));
         break;
       case Type::ustack:
-        name = strdup(get_stack(*(uint64_t*)(arg_data+arg.offset), true, 8).c_str());
-        arg_values.push_back((uint64_t)name);
+        arg_values.emplace_back(new PrintableString(
+          get_stack(*(uint64_t*)(arg_data+arg.offset), true, 8).c_str()));
         break;
       default:
         abort();

src/bpftrace.h

@@ -11,6 +11,7 @@
 #include "ast.h"
 #include "attached_probe.h"
 #include "imap.h"
+#include "printf.h"
 #include "struct.h"
 #include "types.h"
 
@@ -68,7 +69,7 @@ public:
   std::string extract_func_symbols_from_path(const std::string &path);
   std::string resolve_probe(uint64_t probe_id);
   uint64_t resolve_cgroupid(const std::string &path);
-  std::vector<uint64_t> get_arg_values(std::vector<Field> args, uint8_t* arg_data);
+  std::vector<std::unique_ptr<IPrintable>> get_arg_values(std::vector<Field> args, uint8_t* arg_data);
   void add_param(const std::string &param);
   bool is_numeric(std::string str);
   std::string get_param(int index);

src/printf.cpp

@@ -64,4 +64,14 @@ std::string verify_format_string(const std::string &fmt, std::vector<Field> args
   return "";
 }
 
+uint64_t PrintableString::value()
+{
+  return (uint64_t)_value.c_str();
+}
+
+uint64_t PrintableInt::value()
+{
+  return _value;
+}
+
 } // namespace bpftrace

src/printf.h

+#pragma once
+
 #include <sstream>
 
 #include "ast.h"
@@ -9,4 +11,30 @@ struct Field;
 
 std::string verify_format_string(const std::string &fmt, std::vector<Field> args);
 
+class IPrintable
+{
+public:
+  virtual ~IPrintable() { };
+  virtual uint64_t value() = 0;
+};
+
+class PrintableString : public virtual IPrintable
+{
+public:
+  PrintableString(std::string value) : _value(value) { }
+  uint64_t value();
+private:
+  std::string _value;
+};
+
+class PrintableInt : public virtual IPrintable
+{
+public:
+  PrintableInt(uint64_t value) : _value(value) { }
+  uint64_t value();
+private:
+  uint64_t _value;
+};
+
+
 } // namespace bpftrace