[NativePDB] Fix crash in llvm-pdbutil

[gv]

Fix out of buffer read when value of --type-index was too big

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-platform-windows

Author: Vladimir Gorsunov (gv)

Changes

Fix out of buffer read when value of --type-index was too big

---

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

2 Files Affected:

• (modified) llvm/lib/DebugInfo/CodeView/LazyRandomTypeCollection.cpp (+2)
• (added) llvm/test/DebugInfo/PDB/Native/pdb-native-index-overflow.test (+10)

diff --git a/llvm/lib/DebugInfo/CodeView/LazyRandomTypeCollection.cpp b/llvm/lib/DebugInfo/CodeView/LazyRandomTypeCollection.cpp
index 6c23ba8f3c466..69b9597f4f811 100644
--- a/llvm/lib/DebugInfo/CodeView/LazyRandomTypeCollection.cpp
+++ b/llvm/lib/DebugInfo/CodeView/LazyRandomTypeCollection.cpp
@@ -194,6 +194,8 @@ Error LazyRandomTypeCollection::visitRangeForType(TypeIndex TI) {
   }
 
   visitRange(TIB, Prev->Offset, TIE);
+  if (Records.size() <= TI.toArrayIndex())
+    return make_error<CodeViewError>("Type index too big");
   return Error::success();
 }
 
diff --git a/llvm/test/DebugInfo/PDB/Native/pdb-native-index-overflow.test b/llvm/test/DebugInfo/PDB/Native/pdb-native-index-overflow.test
new file mode 100755
index 0000000000000..230a53903f425
--- /dev/null
+++ b/llvm/test/DebugInfo/PDB/Native/pdb-native-index-overflow.test
@@ -0,0 +1,10 @@
+; Test that the native PDB reader isn't crashed by index value bigger than
+; number of types in TPI stream
+; RUN: llvm-pdbutil dump %p/../Inputs/empty.pdb --type-index=20000000\
+; RUN:   | FileCheck -check-prefix=NOT_FOUND %s
+
+NOT_FOUND:                     Types (TPI Stream)                     
+NOT_FOUND:============================================================
+NOT_FOUND:  Showing 1 records.
+NOT_FOUND:  Type 0x1312D00 doesn't exist in TPI stream
+

[llvmbot]

@llvm/pr-subscribers-debuginfo

Author: Vladimir Gorsunov (gv)

Changes

Fix out of buffer read when value of --type-index was too big

---

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

2 Files Affected:

• (modified) llvm/lib/DebugInfo/CodeView/LazyRandomTypeCollection.cpp (+2)
• (added) llvm/test/DebugInfo/PDB/Native/pdb-native-index-overflow.test (+10)

diff --git a/llvm/lib/DebugInfo/CodeView/LazyRandomTypeCollection.cpp b/llvm/lib/DebugInfo/CodeView/LazyRandomTypeCollection.cpp
index 6c23ba8f3c466..69b9597f4f811 100644
--- a/llvm/lib/DebugInfo/CodeView/LazyRandomTypeCollection.cpp
+++ b/llvm/lib/DebugInfo/CodeView/LazyRandomTypeCollection.cpp
@@ -194,6 +194,8 @@ Error LazyRandomTypeCollection::visitRangeForType(TypeIndex TI) {
   }
 
   visitRange(TIB, Prev->Offset, TIE);
+  if (Records.size() <= TI.toArrayIndex())
+    return make_error<CodeViewError>("Type index too big");
   return Error::success();
 }
 
diff --git a/llvm/test/DebugInfo/PDB/Native/pdb-native-index-overflow.test b/llvm/test/DebugInfo/PDB/Native/pdb-native-index-overflow.test
new file mode 100755
index 0000000000000..230a53903f425
--- /dev/null
+++ b/llvm/test/DebugInfo/PDB/Native/pdb-native-index-overflow.test
@@ -0,0 +1,10 @@
+; Test that the native PDB reader isn't crashed by index value bigger than
+; number of types in TPI stream
+; RUN: llvm-pdbutil dump %p/../Inputs/empty.pdb --type-index=20000000\
+; RUN:   | FileCheck -check-prefix=NOT_FOUND %s
+
+NOT_FOUND:                     Types (TPI Stream)                     
+NOT_FOUND:============================================================
+NOT_FOUND:  Showing 1 records.
+NOT_FOUND:  Type 0x1312D00 doesn't exist in TPI stream
+

[gv]

@dwblaikie Hi David, the Mainainers.md file lists you under "Debug info" as "especially type information" - could you review and/or merge this 2 line fix?

[dwblaikie]

@zmodem - any idea who should be looking at PDB things these days?

[Nerixyz]

Could you explain why is this required here? At this point, we already visited the types. I'd expect the method to not visit any type if the type index is too high. Though it looks like, it would visit the last element. However, this wouldn't result in a crash.

[gv]

The crash happens in tryGetType

llvm-project/llvm/lib/DebugInfo/CodeView/LazyRandomTypeCollection.cpp

Line 106 in 9e7a3ee

return Records[Index.toArrayIndex()].Type;

when the index value goes outside the buffer. That's allowed to occur because ensureTypeExists doesn't return error when the index argument is too big.

I put the check in visitRangeForType because it already does some TI validation for ensureTypeExists in line

llvm-project/llvm/lib/DebugInfo/CodeView/LazyRandomTypeCollection.cpp

Line 186 in 9e7a3ee

return make_error<CodeViewError>("Invalid type index");

[Nerixyz]

I see, thank you for the explanation.

A comment explaining why the check is done there (similar to the comment you linked) would be great.

[gv]

Moved the check to tryGetType to replace the assertion per @aganea's suggestion. I think it's overall good idea to get rid of assertions based on input data. Even if some part of debuginfo became corrupted it doesn't mean the entire debug session needs to be aborted

[tru]

@aganea has a pretty good handle on the PDB stuff as well (if he has the time).

[zmodem]

any idea who should be looking at PDB things these days?

I think the right people are on it now. I tried to take a look, but this code is pretty hard to grok :)

[zmodem]

I think --id-index has the same problem, and might also be fixed by this patch.

[gv]

Yes it has and it ts, added it to the test

[aganea]

I feel LazyRandomTypeCollection::tryGetType() is a bit restrictive currently and shouldn't be asserting, since it is returning a std::optional. If you change the assert(contains(Index)) by returning a std::nullopt instead, this fixes the crash. @hans This also fixes --id-index.

[aganea]

LGTM, thanks!