[PATCH] D78736: [DWARF5]: Added support for dumping strx forms in llvm-dwarfdump

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp:154
       StrOffset = Data.getRelocatedValue(/*OffsetSize=*/4, &Offset);
-      E.MacroStr = StringExtractor.getCStr(&StrOffset);
+      assert(StringExtractor && "String Extractor not found");
+      E.MacroStr = StringExtractor->getCStr(&StrOffset);
----------------
SouraVX wrote:
> ikudrin wrote:
> > SouraVX wrote:
> > > ikudrin wrote:
> > > > `assert` should not be used for the cases when malicious user input may trigger it.
> > > This `String Extractor` is innocuous in a sense, it is passed as parameters by functions up in the stack. So I left this ``assert` as it is. ? Are you okay with this.
> > > Other asserts, I've replaced with error handling.
> > What if a malformed `.debug_macinfo` section contains a `DW_MACRO_define_strp` entry? Does the code have any protection against that?
> Ah, thanks for noting that. I think we should consider `assert` for these cases `DW_MACRO_define_strp` and `DW_MACRO_define_strx` since these forms belongs and should only be present when section is `debug_macro`. + Test cases to test these cases.
> Does this sounds fair to you ? 
As I have already said, `assert`s should not be used to handle invalid user input.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78736/new/

https://reviews.llvm.org/D78736