[Clang][Frontend] Fix a crash when -Wdocumentation is used

[bc-lee]

This commit resolves a crash issue in Clang's frontend caused while using the -Wdocumentation compiler flag.

The flaw was due to the lack of necessary checks before the extraction of text between the comment and the declaration in the ASTContext.cpp file. Specifically, there was no verification to ensure that the second component of the declaration location's decomposition is not less than the comment's end offset.

This could lead to an invalid length being passed to the StringRef constructor, triggering the crash. I have added a check to prevent this crash from occurring.

Fixes #68524.

[llvmbot]

@llvm/pr-subscribers-clang

Changes

This commit resolves a crash issue in Clang's frontend caused while using the -Wdocumentation compiler flag.

The flaw was due to the lack of necessary checks before the extraction of text between the comment and the declaration in the ASTContext.cpp file. Specifically, there was no verification to ensure that the second component of the declaration location's decomposition is not less than the comment's end offset.

This could lead to an invalid length being passed to the StringRef constructor, triggering the crash. I have added a check to prevent this crash from occurring.

Fixes #68524.

---

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

1 Files Affected:

• (modified) clang/lib/AST/ASTContext.cpp (+3)

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index cdc3d62bca00873..7b4a4202921281c 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -344,6 +344,9 @@ RawComment *ASTContext::getRawCommentForDeclNoCacheImpl(
   if (Invalid)
     return nullptr;
 
+  if (DeclLocDecomp.second < CommentEndOffset)
+    return nullptr;
+
   // Extract text between the comment and declaration.
   StringRef Text(Buffer + CommentEndOffset,
                  DeclLocDecomp.second - CommentEndOffset);

[cor3ntin]

Is there a way we could come up with a test for this?

[bc-lee]

Is there a way we could come up with a test for this?

Unfortunately, I don't think so. I cannot reduce the 600k lines of preprocessed code to a small test case that will crash the clang frontend.

[AaronBallman]

Is there a way we could come up with a test for this?

Unfortunately, I don't think so. I cannot reduce the 600k lines of preprocessed code to a small test case that will crash the clang frontend.

Have you tried using creduce or other such tool? (We generally don't accept patches without test coverage unless it really isn't possible to test the changes, that doesn't appear to be the situation here though.)

[bc-lee]

Since I'm not an expert in clang AST, it is hard to reduce the failing cases. According to my analysis, this crash only happens when the multiple files are involved, so code reduction tools like creduce doesn't helpful a lot.
Instead, I'm providing an explanation of the crash with screenshots in my local environment.

In my local environment, I was building Apple's LLVM with dac71d2e8c4cdc9e0a1254dbf3716252c302d6a5 commit.
A single line containing #include "clang/AST/ASTContext.h" and -Wdocumentation flag is enough to reproduce the crash.

(Note that I'm not making changes against Apple's LLVM. I'm just building Apple's LLVM(and Swift compiler) using the original LLVM ToT commit.)

To explain the crash, I've made modifications to clang/lib/AST/ASTContext.cpp, as shown in the screenshot.

It seems that ASTContext::getRawCommentForDeclNoCacheImpl, OffsetCommentBehindDecl, which is from CommentsInTheFile is directing clang/include/clang/AST/ASTContext.h file. More precisely, CommentBeforeDeclRawText is /// The type for the C sigjmp_buf type. and
OffsetCommentBehindDeclRawText is /// The type for the C ucontext_t type. and in this case. The offset of each element are 14832 and 14913, respectively.

However, Buffer which is given by DeclLocDecomp.first directs the another source code, clang/include/clang/AST/ExternalASTSource.h.
Since CommentEndOffset is based on CommentBeforeDecl, it doesn't make sense to compare DeclLocDecomp.second and CommentEndOffset, as they are not from the same source code.

So the crash is happened because the result of DeclLocDecomp.second - CommentEndOffset is overflowed, so operations over StringRef Text is making the crash.

The best way to fix this issue is to find out why they are not from the same source code and fix it. However, I'm not sure how to fix it, so I've made a patch to avoid the crash.

This logic is behind by CommentBeforeDecl->isDocumentation(), and the crash occurs only when the -Wdocumentation flag is enabled. I believe that this logic is intended for aggregating comments to explain the reason for the -Wdocumentation warning. In other words, clang crashes when it attempts to provide an explanation for the warning. Therefore, it might be acceptable to bypass this logic instead of crashing.

[dhoepfl]

Since I'm not an expert in clang AST, it is hard to reduce the failing cases.

Double so for me.

So the crash is happened because the result of DeclLocDecomp.second - CommentEndOffset is overflowed, so operations over StringRef Text is making the crash.

The best way to fix this issue is to find out why they are not from the same source code and fix it. However, I'm not sure how to fix it, so I've made a patch to avoid the crash.

While your fix resolves the specific crash we noticed, it does not fix the underlying problem. I found that the source of the problem is noted in this comment.

I tried to “fix” this by removing a few lines starting here (maybe you could also remove lines 561-574?) and moving them a few lines down to get:

void ASTContext::attachCommentsToJustParsedDecls(ArrayRef<Decl *> Decls,
                                                 const Preprocessor *PP) {
  if (Comments.empty() || Decls.empty())
    return;

  FileID File;
  for (Decl *D : Decls) {
    SourceLocation Loc = D->getLocation();
    if (Loc.isValid()) {
      // See if there are any new comments that are not attached to a decl.
      // The location doesn't have to be precise - we care only about the file.
      File = SourceMgr.getDecomposedLoc(Loc).first;
      break;
    }
  }

  if (File.isInvalid())
    return;

/* REMOVED HERE */

  // There is at least one comment not attached to a decl.
  // Maybe it should be attached to one of Decls?
  //
  // Note that this way we pick up not only comments that precede the
  // declaration, but also comments that *follow* the declaration -- thanks to
  // the lookahead in the lexer: we've consumed the semicolon and looked
  // ahead through comments.

  for (const Decl *D : Decls) {
    assert(D);
    if (D->isInvalidDecl())
      continue;

    D = &adjustDeclToTemplate(*D);

    const SourceLocation DeclLoc = getDeclLocForCommentSearch(D, SourceMgr);

    if (DeclLoc.isInvalid() || !DeclLoc.isFileID())
      continue;

    if (DeclRawComments.count(D) > 0)
      continue;

/* ADDED THIS */
    FileID File = SourceMgr.getDecomposedLoc(DeclLoc).first;
    if (File.isInvalid())
      continue;

    auto CommentsInThisFile = Comments.getCommentsInFile(File);
    if (!CommentsInThisFile || CommentsInThisFile->empty() ||
        CommentsInThisFile->rbegin()->second->isAttached())
      continue;
/* UNTIL HERE */

    if (RawComment *const DocComment =
            getRawCommentForDeclNoCacheImpl(D, DeclLoc, *CommentsInThisFile)) {
      cacheRawCommentForDecl(*D, *DocComment);
      comments::FullComment *FC = DocComment->parse(*this, PP, D);
      ParsedComments[D->getCanonicalDecl()] = FC;
    }
  }
}

That way it no longer crashes … I think this loads the correct comments for each decl but I am not sure if this is the correct way to fix it (or even if it still does what it is supposed to do).