[PATCH] D49676: [DWARF] support for .debug_addr (consumer)

[Victor Leschuk via Phabricator via llvm-commits]

vleschuk added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:54
+      uint32_t TmpLength = getLength();
+      HeaderData.Length = 0;
+      return createStringError(errc::invalid_argument,
----------------
vleschuk wrote:
> jhenderson wrote:
> > vleschuk wrote:
> > > jhenderson wrote:
> > > > jhenderson wrote:
> > > > > You might as well use the existing function for this.
> > > > > 
> > > > > But actually, here and elsewhere below, I suggest you just assume that the Length is valid. I'd only invalidate it when the version is unsupported, DWARF64 is detected or the section is too short for a length field. That way the caller can just skip the rest of the table and attempt to read another one a bit later, which might work.
> > > > Do you have any comments on the second half of this inline comment? Because I don't think that's been done.
> > > Actually I don't see how it might work. The caller relies on the fact that length value from the header  is valid and parses the rest of the section based on this assumption. In this case the length field is definitely invalid and telling caller that everything is ok will result in incorrect parsing. Here is an example:
> > > 
> > > 
> > > ```
> > > .Ldebug_addr0:                                                                  
> > >   .long 1 # unit_length (should be 12 here)                                                        
> > >   .short 5 # version                                                            
> > >   .byte 4 # address_size                                                        
> > >   .byte 0 # segment_selector_size                                               
> > >   .long 0x00000000                                                              
> > >   .long 0x00000001          
> > > ```                                                    
> > > 
> > > ```
> > >       if (!AddrTable.hasValidLength())                                          
> > >         break; // If we do not break here we will increase Offset by 5 and not by 16 -> incorrect result.                                                                   
> > >       uint64_t Length = AddrTable.getLength();                                  
> > >       Offset = TableOffset + Length;                                            
> > > ```
> > I think this comes down to "who should we trust more?". What do we do in other cases in the DWARF library?
> The most similar case is .debug_rnglists implementation where the same "mechanism" (break if length is invalid) is used, however the RnglistsTable implementation doesn't use it all: there is know explicit invalidation in case of error (I think we should work on it and see if there are any problems).
> 
> In our case I suggest to leave invalidation here: we assume that the header is correct and read the Length value from it, after that we see that our data contradicts our assumption and either Length is invalid or the section is malformed. In any case it is better to report an error than produce potentially invalid output.
> there is know explicit invalidation

there is no explicit invalidation


https://reviews.llvm.org/D49676