[XRay] Sanitize DOT labels in graph output

[tetsuo-cpp]

Summary

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=39701

This patch is to convert certain characters to their XML escape sequences when generating labels for a DOT graph.

I had trouble reproducing the exact issue described on the tracker. I ran llvm-xray graph on a log from a test program that included function templates but wasn't able to get the dot tool to complain about the < and > characters. The documentation also suggests that the escape sequences should only be necessary when using HTML string labels which XRay doesn't use (label=<...> as opposed to label="..."). Perhaps newer versions of Graphviz silently handle this in the case of quoted-string labels.

In any case, the generated labels still look correct after this patch and should also fix the reporter's issue.

I was a bit unsure how to add a test for this since the existing tests seem to only care about func-id rather than giving an actual name. If you could give me a hint on the best way to go about this, that'd be much appreciated!

[lebedev.ri]

I think you just want to call llvm::DOT::EscapeString().

[tetsuo-cpp]

@lebedev.ri
Thanks for taking a look. I did come across that function earlier but didn't really understand it since I don't see any escaping rules in the docs that look like that. It also doesn't do anything with the & character which will appear in function names too.
But if that solves the problem you described then I'll just use that instead.

[lebedev.ri]

I don't actually know if that one fixes my issue, but i would think they are solving the same issue (? i think ?),
so it is possible that one is simply not complete.

[tetsuo-cpp]

Understood. If you're still running the same setup, could you please post your dot -V output? I can try testing with the same one.

[lebedev.ri]

In D69461#1722205, @tetsuo-cpp wrote:

Understood. If you're still running the same setup, could you please post your dot -V output? I can try testing with the same one.

Hm, the bug was Reported: 2018-11-18, so as per https://tracker.debian.org/pkg/graphviz i suspect back then i was using [[ https://tracker.debian.org/news/992788/accepted-graphviz-2401-5-source-amd64-all-into-unstable/ | 2.40.1-5 ]]

[dberris]

LGTM with one nit. Thanks!

llvm/tools/llvm-xray/xray-graph.cpp

168

nit: Consider using StringRef instead?

[tetsuo-cpp]

Use StringRef.

[tetsuo-cpp]

I was running 2.40.1-6 which is identical (plus a CVE fix) but still couldn't get dot to error with a test program with templates. The output of dot looks correct for different formats (I was looking mainly at SVG and JSON) before and after this change.
So I'm still not 100% sure that you need this change since I haven't seen the problem first hand. But it doesn't hurt either way.

[tetsuo-cpp]

I compiled multiple older versions of dot and gave them a try (I went back to 2.30.1) but still didn't see this problem. I've been testing like this (mainly testing SVG and JSON):

tetsuo@garland-1572667608860-c-16-sgp1-01:~/dev/llvm-project$ build/bin/llvm-xray graph ./xray-log.a.out.uRHPKt -instr_map ./a.out | dot -Tsvg

@dberris Since I can't reproduce this, should I abandon this revision? Or would you still like the patch.

[dberris]

In D69461#1739991, @tetsuo-cpp wrote:

I compiled multiple older versions of dot and gave them a try (I went back to 2.30.1) but still didn't see this problem. I've been testing like this (mainly testing SVG and JSON):

tetsuo@garland-1572667608860-c-16-sgp1-01:~/dev/llvm-project$ build/bin/llvm-xray graph ./xray-log.a.out.uRHPKt -instr_map ./a.out | dot -Tsvg

@dberris Since I can't reproduce this, should I abandon this revision? Or would you still like the patch.

I would still like the patch, because it's still the right thing to do. :)

[tetsuo-cpp]

Thank you. check-llvm looks healthy with this patch.
Could you please commit this for me?

[tetsuo-cpp]

Friendly ping!

llvm/tools/llvm-xray/xray-graph.cpp

static void updateStat(GraphRenderer::TimeStat &S, int64_t L) {
   S.Count++;
   if (S.Min > L || S.Min == 0)
     S.Min = L;
   if (S.Max < L)
     S.Max = L;
   S.Sum += L;
 }
 
+// Labels in a DOT graph must be legal XML strings so it's necessary to escape
+// certain characters.
+static std::string escapeString(StringRef Label) {
+  std::string Str;
+  Str.reserve(Label.size());
+  for (const auto C : Label) {
+    switch (C) {
+    case '&':
+      Str.append("&amp;");
+      break;
+    case '<':
+      Str.append("&lt;");
+      break;
+    case '>':
+      Str.append("&gt;");
+      break;
+    default:
+      Str.push_back(C);
+      break;
+    }
+  }
+  return Str;
+}
+
 // Evaluates an XRay record and performs accounting on it.
 //
 // If the record is an ENTER record it pushes the FuncID and TSC onto a
 // structure representing the call stack for that function.
 // If the record is an EXIT record it checks computes computes the ammount of
 // time the function took to complete and then stores that information in an
 // edge of the graph. If there is no matching ENTER record the function tries
 // to recover by assuming that there were EXIT records which were missed, for
for (const auto &E : G.edges()) {
     OS << "];\n";
   }
 
   for (const auto &V : G.vertices()) {
     const auto &VA = V.second;
     if (V.first == 0)
       continue;
     OS << "F" << V.first << " [label=\"" << (VT != StatType::NONE ? "{" : "")
-       << (VA.SymbolName.size() > 40 ? VA.SymbolName.substr(0, 40) + "..."
-                                     : VA.SymbolName);
+       << escapeString(VA.SymbolName.size() > 40
+                           ? VA.SymbolName.substr(0, 40) + "..."
     if (VT != StatType::NONE)
       OS << "|" << VA.S.getString(VT) << "}\"";
     else
       OS << "\"";
     if (VC != StatType::NONE)
       OS << " color=\""
          << CHelper.getColorString(
                 std::sqrt(VA.S.getDouble(VC) / G.GraphVertexMax.getDouble(VC)))