[PATCH] D127369: [Object][COFF] Fix section name parsing error when the name field is not null-padded

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:1171
     } else {
-      if (Name.substr(1).getAsInteger(10, Offset))
         return createStringError(object_error::parse_failed,
----------------
pzheng wrote:
> efriedma wrote:
> > pzheng wrote:
> > > pzheng wrote:
> > > > rnk wrote:
> > > > > I think it's a bug that getAsInteger doesn't work on non-null terminated StringRefs. It's not an invariant that StringRefs are null terminated. We explicitly form a non-null terminated StringRef on line 1161 above.
> > > > hmm..., not sure if getAsInteger is supposed to handle a situation like this, the description of the function isn't very clear, but I tend to agree with you that this could be bug unless there are already code in LLVM which assumes getAsInteger should fail given such input.
> > > Looking at the implementation of getAsInteger, it looks like it's actually supposed to fail with such input where only the first part of it is a valid integer. getAsInteger requires the whole string to be consumed or else it's considered as a failure.
> > In the testcase, `Name.substr(1)` contains the value "4\0abcde", i.e. an embedded null.  Since '\0' isn't a digit, getAsInteger() is correctly rejecting it.  consumeInteger() just stops parsing at '\0'.
> > 
> > I suspect this code shouldn't be passing down a StringRef with embedded nulls, though.  Maybe the `if (Sec->Name[COFF::NameSize - 1] == 0)` check is wrong.  (The spec says "null-padded", but maybe in practice Microsoft tools just treat it as "null-terminated".)
> Interesting point. The object which originally exposed the issue has an embedded null right after "/4" and some other garbage characters after that embedded null. Therefore, I used that in my test case. However, dumpbin is able to parse the section name correctly even if the string does not have an embedded null, e.g., "/4abcdef". So, modifying the `if (Sec->Name[COFF::NameSize - 1] == 0)` check to not pass down a StringRef with embeded nulls alone is probably not enough to prevent getAsInteger from failing with inputs like "/4abcdef", and we may still end up having to use consumeInteger to ignore the junks after "/4".
FWIW, even if dumpbin accepts a string like `/4abcdef` I'm not sure if we strictly need to accept that - if there's no observed cases of such object files in the wild. But tolerating `/4\0abcdef` seems reasonable to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127369