Fix buffer overrun in WindowsResourceCOFFWriter::writeSymbolTable()

[inglorion]

Summary

We were using sprintf(..., "$R06X", <some uint32_t>) to create strings
that are expected to be exactly length 8, but this results in longer
strings if the uint32_t is greater than 0xffffff. This change modifies
the behavior as follows:

• Uses the loop counter instead of the data offset. This gives us sequential symbol names, avoiding collisions as much as possible.
• Masks the value to 0xffffff to avoid generating names longer than 8 bytes.
• Uses formatv instead of sprintf.

Fixes PR35581.

[inglorion]

I'll see if I can cook up a test for this tomorrow; basically we need to look at the generated symbol names and see that they match \$R......, where the last 6 bytes match the last 6 bytes of the symbol's value, encoded in hexadecimal.

[ruiu]

llvm/lib/Object/WindowsResource.cpp

564 ↗

(On Diff #127061)

There seems to be a small chance that this line could create duplicate names, and if two symbols have the same name, something strange could happen. As we discussed, I think it is better to use i instead of DataOffsets[i] to generate a naem.

[zturner]

Uhh, not to ask the obvious, but why are we even using sprintf at all? Use something like formatv, or even raw_ostream

[pcc]

llvm/lib/Object/WindowsResource.cpp

564 ↗

(On Diff #127061)

But these are static symbols, right? Do we need to give them unique names?

Also, if they only exist for the purpose of relocations, do we even need symbols at all? Can we make each of the relocations relative to the section symbol and store the data offset in the section data?

[inglorion]

I also thought of using the iteration count for the names instead of the offset, but decided to match Microsoft's cvtres for now. Thinking about this some more, I don't think the symbol names actually matter, and so I'm happy to go with using the loop counter instead. I would like to keep the symbols at least for now, as at least some tools seem to expect them (e.g. https://github.com/dotnet/roslyn/blob/614299ff83da9959fa07131c6d0ffbc58873b6ae/src/Compilers/Core/Portable/CvtRes.cs).

[zturner]

Can you still get rid of the sprintf? I don't think we should be using this function in production code honestly.

[ruiu]

I think this is where we should use snprintf to write directly to Symbol->Name.ShortName.

[inglorion]

Rewrote using formatv and using loop counter instead of offset.

[ruiu]

llvm/lib/Object/WindowsResource.cpp

566 ↗

(On Diff #127403)

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).

[inglorion]

llvm/lib/Object/WindowsResource.cpp

566 ↗

(On Diff #127403)

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.

[inglorion]

fixed format specifier and updated tests

[ruiu]

LGTM

llvm/trunk/lib/Object/WindowsResource.cpp

 //===-- WindowsResource.cpp -------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the .res file class.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Object/WindowsResource.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Support/FileOutputBuffer.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/MathExtras.h"
 #include <ctime>
 #include <queue>
 #include <system_error>
 
 using namespace llvm;
 using namespace object;
 
void WindowsResourceCOFFWriter::writeSymbolTable() {
   Aux->NumberOfLinenumbers = 0;
   Aux->CheckSum = 0;
   Aux->NumberLowPart = 0;
   Aux->Selection = 0;
   CurrentOffset += sizeof(coff_aux_section_definition);
 
   // Now write a symbol for each relocation.
   for (unsigned i = 0; i < Data.size(); i++) {
-    char RelocationName[9];
-    sprintf(RelocationName, "$R%06X", DataOffsets[i]);
+    auto RelocationName = formatv("$R{0:X-6}", i & 0xffffff).sstr<COFF::NameSize>();
     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];
     Symbol->SectionNumber = 2;
     Symbol->Type = COFF::IMAGE_SYM_DTYPE_NULL;
     Symbol->StorageClass = COFF::IMAGE_SYM_CLASS_STATIC;
     Symbol->NumberOfAuxSymbols = 0;
     CurrentOffset += sizeof(coff_symbol16);
   }
 }

llvm/trunk/test/tools/llvm-cvtres/machine.test

 
 RUN: llvm-cvtres /machine:ARM64 /out:%t %p/Inputs/test_resource.res
 RUN: llvm-readobj -h -relocations %t | FileCheck %s -check-prefix=ARM64
 
 X86:         Machine: IMAGE_FILE_MACHINE_I386 (0x14C)
 X86-DAG:   Relocations [
 X86-DAG:                 .rsrc$01 {
 X86-NEXT:      0x1E8 IMAGE_REL_I386_DIR32NB $R000000
-X86-NEXT:      0x198 IMAGE_REL_I386_DIR32NB $R000018
-X86-NEXT:      0x1A8 IMAGE_REL_I386_DIR32NB $R000340
-X86-NEXT:      0x1C8 IMAGE_REL_I386_DIR32NB $R000668
-X86-NEXT:      0x1D8 IMAGE_REL_I386_DIR32NB $R000698
-X86-NEXT:      0x1F8 IMAGE_REL_I386_DIR32NB $R000708
-X86-NEXT:      0x1B8 IMAGE_REL_I386_DIR32NB $R000720
-X86-NEXT:      0x188 IMAGE_REL_I386_DIR32NB $R000750
+X86-NEXT:      0x198 IMAGE_REL_I386_DIR32NB $R000001
+X86-NEXT:      0x1A8 IMAGE_REL_I386_DIR32NB $R000002
+X86-NEXT:      0x1C8 IMAGE_REL_I386_DIR32NB $R000003
+X86-NEXT:      0x1D8 IMAGE_REL_I386_DIR32NB $R000004
+X86-NEXT:      0x1F8 IMAGE_REL_I386_DIR32NB $R000005
+X86-NEXT:      0x1B8 IMAGE_REL_I386_DIR32NB $R000006
+X86-NEXT:      0x188 IMAGE_REL_I386_DIR32NB $R000007
 
 X64:         Machine: IMAGE_FILE_MACHINE_AMD64 (0x8664)
 X64-DAG:   Relocations [
 X64-DAG:                 .rsrc$01 {
 X64-NEXT:      0x1E8 IMAGE_REL_AMD64_ADDR32NB $R000000
-X64-NEXT:      0x198 IMAGE_REL_AMD64_ADDR32NB $R000018
-X64-NEXT:      0x1A8 IMAGE_REL_AMD64_ADDR32NB $R000340
-X64-NEXT:      0x1C8 IMAGE_REL_AMD64_ADDR32NB $R000668
-X64-NEXT:      0x1D8 IMAGE_REL_AMD64_ADDR32NB $R000698
-X64-NEXT:      0x1F8 IMAGE_REL_AMD64_ADDR32NB $R000708
-X64-NEXT:      0x1B8 IMAGE_REL_AMD64_ADDR32NB $R000720
-X64-NEXT:      0x188 IMAGE_REL_AMD64_ADDR32NB $R000750
+X64-NEXT:      0x198 IMAGE_REL_AMD64_ADDR32NB $R000001
+X64-NEXT:      0x1A8 IMAGE_REL_AMD64_ADDR32NB $R000002
+X64-NEXT:      0x1C8 IMAGE_REL_AMD64_ADDR32NB $R000003
+X64-NEXT:      0x1D8 IMAGE_REL_AMD64_ADDR32NB $R000004
+X64-NEXT:      0x1F8 IMAGE_REL_AMD64_ADDR32NB $R000005
+X64-NEXT:      0x1B8 IMAGE_REL_AMD64_ADDR32NB $R000006
+X64-NEXT:      0x188 IMAGE_REL_AMD64_ADDR32NB $R000007
 
 ARM:         Machine: IMAGE_FILE_MACHINE_ARMNT (0x1C4)
 ARM-DAG:   Relocations [
 ARM-DAG:                 .rsrc$01 {
 ARM-NEXT:      0x1E8 IMAGE_REL_ARM_ADDR32NB $R000000
-ARM-NEXT:      0x198 IMAGE_REL_ARM_ADDR32NB $R000018
-ARM-NEXT:      0x1A8 IMAGE_REL_ARM_ADDR32NB $R000340
-ARM-NEXT:      0x1C8 IMAGE_REL_ARM_ADDR32NB $R000668
-ARM-NEXT:      0x1D8 IMAGE_REL_ARM_ADDR32NB $R000698
-ARM-NEXT:      0x1F8 IMAGE_REL_ARM_ADDR32NB $R000708
-ARM-NEXT:      0x1B8 IMAGE_REL_ARM_ADDR32NB $R000720
-ARM-NEXT:      0x188 IMAGE_REL_ARM_ADDR32NB $R000750
+ARM-NEXT:      0x198 IMAGE_REL_ARM_ADDR32NB $R000001
+ARM-NEXT:      0x1A8 IMAGE_REL_ARM_ADDR32NB $R000002
+ARM-NEXT:      0x1C8 IMAGE_REL_ARM_ADDR32NB $R000003
+ARM-NEXT:      0x1D8 IMAGE_REL_ARM_ADDR32NB $R000004
+ARM-NEXT:      0x1F8 IMAGE_REL_ARM_ADDR32NB $R000005
+ARM-NEXT:      0x1B8 IMAGE_REL_ARM_ADDR32NB $R000006
+ARM-NEXT:      0x188 IMAGE_REL_ARM_ADDR32NB $R000007
 
 ARM64:       Machine: IMAGE_FILE_MACHINE_ARM64 (0xAA64)
 ARM64-DAG: Relocations [
 ARM64-DAG:               .rsrc$01 {
 ARM64-NEXT:    0x1E8 IMAGE_REL_ARM64_ADDR32NB $R000000
-ARM64-NEXT:    0x198 IMAGE_REL_ARM64_ADDR32NB $R000018
-ARM64-NEXT:    0x1A8 IMAGE_REL_ARM64_ADDR32NB $R000340
-ARM64-NEXT:    0x1C8 IMAGE_REL_ARM64_ADDR32NB $R000668
-ARM64-NEXT:    0x1D8 IMAGE_REL_ARM64_ADDR32NB $R000698
-ARM64-NEXT:    0x1F8 IMAGE_REL_ARM64_ADDR32NB $R000708
-ARM64-NEXT:    0x1B8 IMAGE_REL_ARM64_ADDR32NB $R000720
-ARM64-NEXT:    0x188 IMAGE_REL_ARM64_ADDR32NB $R000750
+ARM64-NEXT:    0x198 IMAGE_REL_ARM64_ADDR32NB $R000001
+ARM64-NEXT:    0x1A8 IMAGE_REL_ARM64_ADDR32NB $R000002
+ARM64-NEXT:    0x1C8 IMAGE_REL_ARM64_ADDR32NB $R000003
+ARM64-NEXT:    0x1D8 IMAGE_REL_ARM64_ADDR32NB $R000004
+ARM64-NEXT:    0x1F8 IMAGE_REL_ARM64_ADDR32NB $R000005
+ARM64-NEXT:    0x1B8 IMAGE_REL_ARM64_ADDR32NB $R000006
+ARM64-NEXT:    0x188 IMAGE_REL_ARM64_ADDR32NB $R000007

llvm/trunk/test/tools/llvm-cvtres/symbols.test

 // Check COFF emission of cvtres
 // The input was generated with the following command, using the original Windows
 // rc.exe:
 // > rc /fo test_resource.res /nologo test_resource.rc
 // The object file we are comparing against was generated with this command using
 // the original Windows cvtres.exe.
 // > cvtres /machine:X86 /readonly /nologo /out:test_resource.obj.coff \
 //   test_resource.res
 
 RUN: llvm-cvtres /verbose /out:%t %p/Inputs/test_resource.res
 RUN: llvm-readobj -symbols %t | FileCheck %s
 
 CHECK:        Name: $R000000
 CHECK-NEXT:    Value: 0
 CHECK-NEXT:    Section: .rsrc$02
-CHECK:        Name: $R000018
+CHECK:        Name: $R000001
 CHECK-NEXT:    Value: 24
 CHECK-NEXT:    Section: .rsrc$02
-CHECK:        Name: $R000340
+CHECK:        Name: $R000002
 CHECK-NEXT:    Value: 832
 CHECK-NEXT:    Section: .rsrc$02
-CHECK:        Name: $R000668
+CHECK:        Name: $R000003
 CHECK-NEXT:    Value: 1640
 CHECK-NEXT:    Section: .rsrc$02
-CHECK:        Name: $R000698
+CHECK:        Name: $R000004
 CHECK-NEXT:    Value: 1688
 CHECK-NEXT:    Section: .rsrc$02
-CHECK:        Name: $R000720
+CHECK:        Name: $R000006
 CHECK-NEXT:    Value: 1824
 CHECK-NEXT:    Section: .rsrc$02
-CHECK:        Name: $R000750
+CHECK:        Name: $R000007
 CHECK-NEXT:    Value: 1872
 CHECK-NEXT:    Section: .rsrc$02