[all-commits] [llvm/llvm-project] e5d255: Fix OOM in FormatDiagnostic (#108187)

[Vakhurin Sergei via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: e5d255607d200f59c5f7474b8dde6fe72d53e348
      https://github.com/llvm/llvm-project/commit/e5d255607d200f59c5f7474b8dde6fe72d53e348
  Author: Vakhurin Sergei <igelbox at gmail.com>
  Date:   2024-09-16 (Mon, 16 Sep 2024)

  Changed paths:
    M clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
    M clang/include/clang/Basic/Diagnostic.h
    M clang/include/clang/Basic/DiagnosticIDs.h
    M clang/include/clang/Basic/PartialDiagnostic.h
    M clang/include/clang/Sema/Sema.h
    M clang/lib/Basic/Diagnostic.cpp
    M clang/lib/Basic/DiagnosticIDs.cpp
    M clang/lib/Basic/SourceManager.cpp
    M clang/lib/Frontend/Rewrite/FixItRewriter.cpp
    M clang/lib/Frontend/TextDiagnosticPrinter.cpp
    M clang/lib/Sema/Sema.cpp
    M clang/lib/Sema/SemaBase.cpp
    M clang/lib/Serialization/ASTReader.cpp
    A clang/test/PCH/race-condition.cpp
    M clang/unittests/Basic/DiagnosticTest.cpp
    M clang/unittests/Driver/DXCModeTest.cpp

  Log Message:
  -----------
  Fix OOM in FormatDiagnostic (#108187)

Resolves: #70930 (and probably latest comments from
https://github.com/clangd/clangd/issues/251)
by fixing racing for the shared `DiagStorage` value which caused messing
with args inside the storage and then formatting the following message
with `getArgSInt(1)` == 2:
```
def err_module_odr_violation_function : Error<
  "%q0 has different definitions in different modules; "
  "%select{definition in module '%2'|defined here}1 "
  "first difference is "
```
which causes `HandleSelectModifier` to go beyond the `ArgumentLen` so
the recursive call to `FormatDiagnostic` was made with `DiagStr` >
`DiagEnd` that leads to infinite `while (DiagStr != DiagEnd)`.

**The Main Idea:**
Reuse the existing `DiagStorageAllocator` logic to make all
`DiagnosticBuilder`s having independent states.
Also, encapsulating the rest of state (e.g. ID and Loc) into
`DiagnosticBuilder`.

**TODO (if it will be requested by reviewer):**
- [x] add a test (I have no idea how to turn a whole bunch of my
proprietary code which leads `clangd` to OOM into a small public
example.. probably I must try using
[this](https://github.com/llvm/llvm-project/issues/70930#issuecomment-2209872975)
instead)
- [x] [`Diag.CurDiagID !=
diag::fatal_too_many_errors`](https://github.com/llvm/llvm-project/pull/108187#pullrequestreview-2296395489)
- [ ] ? get rid of `DiagStorageAllocator` at all and make
`DiagnosticBuilder` having they own `DiagnosticStorage` coz it seems
pretty small so should fit the stack for short-living
`DiagnosticBuilder` instances



To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications