[Lex][Clang] Add checking to HeaderMapImpl::getString to make it more robust

[shafik]

Static analysis identified the Len - 1 expression in HeaderMapImpl::getString as problematic if Len is zero. After filing this issue:

#130185

Indeed we should be checking MaxLen and verify it is not zero as well.

Fixes: #130185

[llvmbot]

@llvm/pr-subscribers-clang

Author: Shafik Yaghmour (shafik)

Changes

Static analysis identified the Len - 1 expression in HeaderMapImpl::getString as problematic if Len is zero. After filing this issue:

#130185

Indeed we should be checking MaxLen and verify it is not zero as well.

Fixes: #130185

---

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

1 Files Affected:

• (modified) clang/lib/Lex/HeaderMap.cpp (+3)

diff --git a/clang/lib/Lex/HeaderMap.cpp b/clang/lib/Lex/HeaderMap.cpp
index 14731f29486c8..1b64ecb775da0 100644
--- a/clang/lib/Lex/HeaderMap.cpp
+++ b/clang/lib/Lex/HeaderMap.cpp
@@ -157,6 +157,9 @@ std::optional<StringRef> HeaderMapImpl::getString(unsigned StrTabIdx) const {
   unsigned MaxLen = FileBuffer->getBufferSize() - StrTabIdx;
   unsigned Len = strnlen(Data, MaxLen);
 
+  if (MaxLen == 0)
+    return std::nullopt;
+
   // Check whether the buffer is null-terminated.
   if (Len == MaxLen && Data[Len - 1])
     return std::nullopt;

[tahonermann]

I added a suggested change, but am approving on the basis that the code in the PR works as is.

[AaronBallman]

LGTM with nit addressed. Test coverage would be nice to add, but not strictly required if this is purely a defensive measure (which it seems to be).

[shafik]

LGTM with nit addressed. Test coverage would be nice to add, but not strictly required if this is purely a defensive measure (which it seems to be).

Exactly.