Detect source location overflow due includes

[dnsampaio]

Summary

As discussed in http://lists.llvm.org/pipermail/cfe-dev/2019-October/063459.html
the overflow of the souce locations (limited to 2^31 chars) can generate all sorts of
weird things (bogus warnings, hangs, crashes, miscompilation and correct compilation).
In debug mode this assert would fail. So it might be a good start, as in PR42301,
to detect the failure and exit with a proper error message.

[dnsampaio]

clang/lib/Basic/SourceManager.cpp

587

For debug builds, I could not find any other way to not reach an assert failure other than exiting here. Perhaps there is a more llvm specific way to die? llvm_unreachable() ?

[miyuki]

clang/lib/Basic/SourceManager.cpp

587

There is a similar error err_file_too_large. How is it handled? llvm_unreachable is intended for code that is unreachable: it causes an assertion failure in debug builds and undefined behavior in release builds.

[miyuki]

clang/include/clang/Basic/DiagnosticCommonKinds.td

288

... >, DefaultFatal;

[dnsampaio]

• Add ", DefaultFatal";

clang/lib/Basic/SourceManager.cpp

587

The err_file_too_large above in this file allows the function to finish and return further on. Here, we assert false before the message being printed.
If llvm_unreachable is undefined behavior, then I should stick to exit(1) ?

[miyuki]

clang/lib/Basic/SourceManager.cpp

587

I think you should return an invalid file ID (i.e. return FileID(); and propagate the error through the call stack. Clang can be used as a library, so you can't just call exit(), this would terminate the user's program.

[dnsampaio]

• Return an invalid FileID instead of exiting.

[dnsampaio]

clang/lib/Basic/SourceManager.cpp

587

It will still reach an false assert in builds that enable them. But in release it linger on and ends with the correct warning.

[dnsampaio]

Ping

[miyuki]

clang/lib/Basic/SourceManager.cpp

587

Has this problem been fixed?

[dnsampaio]

Yes. It does return a non-valid FileID, and in builds without assert you get the expected error message.

[miyuki]

In D70183#1751552, @dnsampaio wrote:

Yes. It does return a non-valid FileID, and in builds without assert you get the expected error message.

I was asking about "It will still reach an false assert in builds that enable them". Has this been fixed?

[dnsampaio]

clang/lib/Basic/SourceManager.cpp

587

No. Don't intend to fix the in this patch, as they would be failing at the moment anyway for such cases.

[miyuki]

This essentially means that if there is a problem related to this change, say something goes wrong after return FileID(); it would be impossible to debug until the corresponding assertions are fixed. Also the patch lacks tests (and adding a test is impossible because it would fail in debug builds).

[dnsampaio]

Git diff 979ae80af7ec49624b932954d22cb91900f17121 did not send a test as well. Feel free to send me a reasonable sized reproducer, the one I have is about 36MB. Don't think it will be that well received.

[miyuki]

In D70183#1751813, @dnsampaio wrote:

Git diff 979ae80af7ec49624b932954d22cb91900f17121 did not send a test as well. Feel free to send me a reasonable sized reproducer, the one I have is about 36MB. Don't think it will be that well received.

#!/usr/bin/env python3

def gen_100k():
    s = '/*' + (' '*95) + '*/\n'
    with open('inc100k.h', 'wt') as out_f:
        for _ in range(0, 1000):
            out_f.write(s)

def gen_100M():
    s = '#include "inc100k.h"\n'
    with open('inc100M.h', 'wt') as out_f:
        for _ in range(0, 1000):
            out_f.write(s)

def gen_main():
    s = '#include "inc100M.h"\n'
    with open('reproducer.c', 'wt') as out_f:
        for _ in range(0, 25):
            out_f.write(s)

def main():
    gen_100k()
    gen_100M()
    gen_main()

if __name__ == '__main__':
    main()

[dnsampaio]

Just wondering, I don't fully understand why is that important from the point I do a return FileID();. The critical error has already been inserted to the error list. clang shall either die due an assert in debug, or with a nice message in release. Unless clang is all broken, and the error message would be corrupted, which I don't believe is the case.

[miyuki]

clang shall either die due an assert in debug [...]

I think I have already mentioned the reasons why I don't think this is acceptable:

• it would break tests, as they are usually run with enabled assertions (and since this diagnostic is quite easy to test, it should be tested)
• it would make the code harder to debug (i.e. if someone finds another problem related to large includes, they would first need to fix the failing assertions before they could start debugging their problem)

IMHO we should discuss this in person, that would be quicker.

[dnsampaio]

Rebased

Created a early exit when a source location overflow is detected,
avoiding further processing or reaching other assert failures.
For that, it was required to create a 'Failure' kind for includes,
as well add return points along the callstack.

Added test that works in both release and debug modes.

[miyuki]

LGTM, but since we both work at Arm, let's wait a week for other folks to chime in if they have any objections.

[dnsampaio]

• Fixed to return correct token

clang/include/clang/Basic/DiagnosticCommonKinds.td

def err_opt_not_valid_on_target : Error<
   "option '%0' cannot be specified on this target">;
 
 // Source manager
 def err_cannot_open_file : Error<"cannot open file '%0': %1">, DefaultFatal;
 def err_file_modified : Error<
   "file '%0' modified since it was first processed">, DefaultFatal;
 def err_file_too_large : Error<
   "sorry, unsupported: file '%0' is too large for Clang to process">;
+def err_include_too_large : Error<
+  "sorry, this include generates a translation unit too large for"
+  " Clang to process. This may by a result from multiple"
+  " inclusions of unguarded header files.">, DefaultFatal;
 def err_unsupported_bom : Error<"%0 byte order mark detected in '%1', but "
   "encoding is not supported">, DefaultFatal;
 def err_unable_to_rename_temp : Error<
   "unable to rename temporary '%0' to output file '%1': '%2'">;
 def err_unable_to_make_temp : Error<
   "unable to make temporary file: %0">;
 
 // Modules

clang/lib/Basic/SourceManager.cpp

if (LoadedID < 0) {
     unsigned Index = unsigned(-LoadedID) - 2;
     assert(Index < LoadedSLocEntryTable.size() && "FileID out of range");
     assert(!SLocEntryLoaded[Index] && "FileID already loaded");
     LoadedSLocEntryTable[Index] = SLocEntry::get(
         LoadedOffset, FileInfo::get(IncludePos, File, FileCharacter, Filename));
     SLocEntryLoaded[Index] = true;
     return FileID::get(LoadedID);
   }
+  unsigned FileSize = File->getSize();
+  if (!(NextLocalOffset + FileSize + 1 > NextLocalOffset &&
+        NextLocalOffset + FileSize + 1 <= CurrentLoadedOffset)) {
+    // From this point, there is no sensible way to point to the current
+    // source-location and say: This include at line ### generates a too
+    // big file, as the IncludePos received is
+    Diag.Report(IncludePos, diag::err_include_too_large);
+    return FileID();
+  }
   LocalSLocEntryTable.push_back(
       SLocEntry::get(NextLocalOffset,
                      FileInfo::get(IncludePos, File, FileCharacter, Filename)));
-  unsigned FileSize = File->getSize();
-  assert(NextLocalOffset + FileSize + 1 > NextLocalOffset &&
-         NextLocalOffset + FileSize + 1 <= CurrentLoadedOffset &&
-         "Ran out of source locations!");
   // We do a +1 here because we want a SourceLocation that means "the end of the
   // file", e.g. for the "no newline at the end of the file" diagnostic.
   NextLocalOffset += FileSize + 1;
 
   // Set LastFileIDLookup to the newly created file.  The next getFileID call is
   // almost guaranteed to be from that file.
   FileID FID = FileID::get(LocalSLocEntryTable.size()-1);
   return LastFileIDLookup = FID;