[LNT] lnt profile upgrade command for large Spec2017 perf.data aborts

[PrzemekWirkus]

Summary

Hi,

This patch addresses issue with *stack smashing* while processing large perf.data files (>350 MB) e.g. obtained during Spec2017 sub-benchmarks' runs:
Example lnt profile upgrade failure:

$ lnt profile upgrade perf.data.510.parest_r.0 perf.data.510.parest_r.0.lnt
*** stack smashing detected ***: /opt/venv_lnt/bin/python terminated

Change Description

Arbitrary size of temp buffers One, Two, Three & Four in function NmOutput::fetchSymbols() is in some circumstances too small and buffer(s) overflow while processing line of data.

1. Changes
2. New function NmOutput::splitLine(line, vector_of_substrings) manages strings properly.
3. Encapsulate One, Two, Three & Four into std::string objects for proper variable string length memory management.

Patch testing

Tested done by running stock LNT against my modification and comparison between output lnt profile files (size, md5).

[PrzemekWirkus]

Small curly braces indent issue fixed (consistent coding style)

[cmatthews]

I cam't provide any good feedback on cpp code, and we don't use this for Mac. @kristof.beyls or @MatzeB, any feedback?

[kristof.beyls]

I hope that in the long run, we can get rid of this cPerf.cpp C++ implementation, and instead make use of "perf data convert --to-ctf" to convert perf.data files into a CTF format which should be easier to parse in python.
At the time this C++ implementation was added, the "perf data convert --to-ctf" functionality tended not to be compiled in to perf as distributed by the main linux distros.
Looking at a reason stack overflow issue about this suggests that the situation may not have changed much (https://stackoverflow.com/questions/43576997/building-perf-with-babeltrace-for-perf-to-ctf-conversion), hence we'll probably need this C++ implementation for some time to come.

The code changes look fine to me to fix the stack buffer overflow bug in the current implementation.

[PrzemekWirkus]

@MatzeB Are you able to have a quick look at this patch please?

[MatzeB]

No feedback from my side, I#m not familiar with the profile code.

[PrzemekWirkus]

@MatzeB There is nothing perf specific in this code, just plain C++ to wrap buffers into containers.

@cmatthews Can we merge ?

lnt/testing/profile/cPerf.cpp

void fetchSymbols(Map *M, bool Dynamic) {
 
     char *Line = nullptr;
     size_t LineLen = 0;
     while (true) {
       ssize_t Len = getline(&Line, &LineLen, Stream);
       if (Len == -1)
         break;
 
-      char One[32], Two[32], Three[32], Four[512];
-      if (sscanf(Line, "%s %s %s %s", One, Two, Three, Four) < 4)
-        continue;
+      std::vector<std::string> SplittedLine;
+      if (splitLine(std::string(Line), SplittedLine) < 4)
+        continue; 
 
+      const std::string& One = SplittedLine[0];
+      const std::string& Two = SplittedLine[1];
+      const std::string& Three = SplittedLine[2];
+      std::string& Four = SplittedLine[3];
+
       char *EndPtr = NULL;
-      uint64_t Start = strtoull(One, &EndPtr, 16);
-      if (EndPtr == One)
+      uint64_t Start = strtoull(One.c_str(), &EndPtr, 16);
+      if (EndPtr == One.c_str())
         continue;
-      uint64_t Extent = strtoull(Two, &EndPtr, 16);
-      if (EndPtr == Two)
+      uint64_t Extent = strtoull(Two.c_str(), &EndPtr, 16);
+      if (EndPtr == Two.c_str())
         continue;
-      if (strlen(Three) != 1)
+      if (Three.length() != 1)
         continue;
-      switch (Three[0]) {
+      switch (Three.front()) {
       default:
         continue;
       case 'T':
       case 't': // Text section
       case 'V':
       case 'v': // Weak object
       case 'W':
       case 'w': // Weak object (not tagged as such)
         break;
       }
-      std::string S(Four);
-      if (S.back() == '\n')
-        S.pop_back();
-      push_back({Start, Start + Extent, S});
+      if (Four.back() == '\n')
+        Four.pop_back();
+      push_back({Start, Start + Extent, Four});
     }
     if (Line)
       free(Line);
 
     fclose(Stream);
     wait(NULL);
   }
 
   void reset(Map *M) {
     clear();
     // Fetch both dynamic and static symbols, sort and unique them.
     fetchSymbols(M, true);
     fetchSymbols(M, false);
     
     std::sort(begin(), end());
     auto NewEnd = std::unique(begin(), end());
     erase(NewEnd, end());
   }
+
+protected:
+  int splitLine(const std::string& line, std::vector<std::string>& output, char delim = ' ') {
+    std::stringstream ss(line);
+    std::string token;
+    while (std::getline(ss, token, delim)) {
+        output.push_back(token);
+    }
+    return output.size();
+  }
 };
 
 class ObjdumpOutput {
 public:
   std::string Objdump;
   FILE *Stream;
   char *ThisText;
   uint64_t ThisAddress;