[clang][ASTImporter] fix assert fail due to offset overflow

[jcsxky]

This patch tries to fix assert failed due to offset overflow in SourceManager. UIntTy in SourceLocation is unsigned int and has 2G limit(highest bit represent whether it is macro). When importing a huge file or importing to many files, NextLocalOffset in SourceManager will overflow and createFileID returns a invalid FileID. This will lead to assert failed and crash.

[jcsxky]

We encounter this problem in OpenBCM. But, it's not convenient and proper to create such a huge file to test.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Qizhi Hu (jcsxky)

Changes

This patch tries to fix assert failed due to offset overflow in SourceManager. UIntTy in SourceLocation is unsigned int and has 2G limit(low bit represent whether it is macro). When importing a huge file or importing to many files, NextLocalOffset in SourceManager will overflow and createFileID returns a invalid FileID. This will lead to assert failed and crash.

---

Full diff: https://github.com/llvm/llvm-project/pull/79084.diff

1 Files Affected:

• (modified) clang/lib/AST/ASTImporter.cpp (+15-1)

diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 12734d62ed9fb76..ae9c9e8d04fc98a 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -9837,6 +9837,13 @@ Expected<SourceRange> ASTImporter::Import(SourceRange FromRange) {
   return SourceRange(ToBegin, ToEnd);
 }
 
+static bool isBufferSizeOverflow(SourceManager &SM, size_t BufferSize) {
+  unsigned Offset = SM.getNextLocalOffset();
+  unsigned FullSize = Offset + BufferSize + 1;
+  SourceLocation L = SourceLocation().getFromRawEncoding(FullSize);
+  return !L.isFileID() || !(FullSize > Offset && SM.isLoadedSourceLocation(L));
+}
+
 Expected<FileID> ASTImporter::Import(FileID FromID, bool IsBuiltin) {
   llvm::DenseMap<FileID, FileID>::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
@@ -9896,9 +9903,13 @@ Expected<FileID> ASTImporter::Import(FileID FromID, bool IsBuiltin) {
         // FIXME: The filename may be a virtual name that does probably not
         // point to a valid file and we get no Entry here. In this case try with
         // the memory buffer below.
-        if (Entry)
+        if (Entry) {
+          if (isBufferSizeOverflow(ToSM, Entry->getSize()))
+            return llvm::make_error<ASTImportError>(
+                ASTImportError::UnsupportedConstruct);
           ToID = ToSM.createFileID(*Entry, ToIncludeLocOrFakeLoc,
                                    FromSLoc.getFile().getFileCharacteristic());
+        }
       }
     }
 
@@ -9913,6 +9924,9 @@ Expected<FileID> ASTImporter::Import(FileID FromID, bool IsBuiltin) {
       std::unique_ptr<llvm::MemoryBuffer> ToBuf =
           llvm::MemoryBuffer::getMemBufferCopy(FromBuf->getBuffer(),
                                                FromBuf->getBufferIdentifier());
+      if (isBufferSizeOverflow(ToSM, ToBuf->getBufferSize()))
+        return llvm::make_error<ASTImportError>(
+            ASTImportError::UnsupportedConstruct);
       ToID = ToSM.createFileID(std::move(ToBuf),
                                FromSLoc.getFile().getFileCharacteristic());
     }

[balazske]

It is better to use Unknown as error kind here (it is used already in this function), or add a new type of error to ASTImportError::ErrorKind.

[danix800]

ASTImportError::Unknown would crash CTU module too, ASTImportError::UnsupportedConstruct might be deliberately chosen so the analysis could continue in this case.

[balazske]

This error is clearly not an unsupported AST construct. For this case a new error type should be added like ASTImportError::SourceLocationOverflow or ResourceOverflow. Code in CrossTranslationUnitContext::importDefinitionImpl should be updated to count this new error.
Alternatively the llvm_unreachable from UnknownError can be removed. But UnknownError should be used if something happens that is not supposed to happen, even if now it is used like "other error".

[Michael137]

This error is clearly not an unsupported AST construct. For this case a new error type should be added like ASTImportError::SourceLocationOverflow or ResourceOverflow

Agreed

[balazske]

Did you check that these FIXME's have no connection to the problem?

[danix800]

Invalid file could exist in real case (causes no Entry) while SourceLocation overflow is normally caused by cumulative effect of multiple (large enough) files imported so I think these two are not connected.

[Michael137]

Is there a way to reproduce this? By creating some very large dummy files? Just to confirm what the crash is exactly (since I don't see an attached stacktrace)