[PATCH] D41270: Fix buffer overrun in WindowsResourceCOFFWriter::writeSymbolTable()

[Bob Haarman via Phabricator via llvm-commits]

inglorion added inline comments.


================
Comment at: llvm/lib/Object/WindowsResource.cpp:566
     Symbol = reinterpret_cast<coff_symbol16 *>(BufferStart + CurrentOffset);
-    strncpy(Symbol->Name.ShortName, RelocationName, (size_t)COFF::NameSize);
+    memcpy(Symbol->Name.ShortName, RelocationName.data(), (size_t) COFF::NameSize);
     Symbol->Value = DataOffsets[i];
----------------
ruiu wrote:
> I don't know much about the format string of the formatv function, but is RelocationName guaranteed to be COFF:NameSize byte long? If not, this memcpy overruns a given buffer.
> 
> I think snprintf is much better. People are familiar with that, and that's exactly what you want to do here (format a string while not overrunning a given string buffer).
One problem with snprintf is that it always writes a NUL terminator. We need the string to be NUL terminated if it is less than 8 bytes long, and not NUL terminated if it is exactly 8 bytes long - which, in our case, it always is. So I don't think we can use snprintf to avoid copying the bytes here.

The thing we get out of formatv(...).sstr<8> is a SmallString<8>, and the format specifier combined with the bitmask causes is to always write all 8 bytes, so that memcpy should be safe here.

We could do without the memcpy if we had some way to write directly into the 8 bytes Symbol->Name.ShortName already has, but neither formatv nor raw_ostream seem to provide that, and snprintf would truncate the string, so we would have to implement something else for that. Could be done, but I would like to fix the buffer overrun first.


https://reviews.llvm.org/D41270