Fix invalid memory access in llvm-mc if file ends in non-newline whitespace or comma

[colinl]

Summary

Str may be empty after .find_first_not_of(" \t\r,") and the following Str[0] accesses invalid memory.

[• rafael]

Don't we also have to handle npos?

[colinl]

It looks like substr accepts npos as a start and returns the empty string

/ \param Start The index of the starting character in the substring; if
/ the index is npos or greater than the length of the string then the
/// empty substring will be returned.

[• rafael]

I find the code a bit hard to read as written.

can we rewrite it to be just a series of "if (not interesting) { eat; continue to see if there is more;}"? Something like

static bool SkipToToken(StringRef &Str) {

for (;;) {
  if (Str.empty())
    return false;

  // Strip horizontal whitespace and commas.                                   
  if (size_t Pos = Str.find_first_not_of(" \t\r,")) {
    Str = Str.substr(Pos);
    continue;
  }

  if (Str[0] == '#') {
    Str = Str.substr(Str.find_first_of('\n'));
    continue;
  }

  if (Str[0] == '\n') {
    Str = Str.substr(1);
    continue;
  }

  return true;
}

}

[colinl]

Agreed that loop is a little difficult.

It looks like we can also move the check for '\n' in to the find_first_not_of check.

[colinl]

If this looks good I'll submit it.

[• rafael]

LGTM

[colinl]

Submitted.

test/tools/llvm-mc/line_end_with_space.test

+RUN: llvm-mc -disassemble %s
+ 
 No newline at end of file

tools/llvm-mc/Disassembler.cpp

 }
 
 static bool SkipToToken(StringRef &Str) {
-  while (!Str.empty() && Str.find_first_not_of(" \t\r\n#,") != 0) {
+  for (;;) {
+    if (Str.empty())
+      return false;
+
     // Strip horizontal whitespace and commas.
-    if (size_t Pos = Str.find_first_not_of(" \t\r,")) {
+    if (size_t Pos = Str.find_first_not_of(" \t\r\n,")) {
       Str = Str.substr(Pos);
+      continue;
     }
 
-    // If this is the end of a line or start of a comment, remove the rest of
-    // the line.
-    if (Str[0] == '\n' || Str[0] == '#') {
-      // Strip to the end of line if we already processed any bytes on this
-      // line.  This strips the comment and/or the \n.
-      if (Str[0] == '\n') {
-        Str = Str.substr(1);
-      } else {
+    // If this is the start of a comment, remove the rest of the line.
+    if (Str[0] == '#') {
         Str = Str.substr(Str.find_first_of('\n'));
-        if (!Str.empty())
-          Str = Str.substr(1);
-      }
       continue;
     }
+    return true;
   }
-
-  return !Str.empty();
 }