Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lld/COFF/Config.h
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ struct Configuration {
enum ManifestKind { Default, SideBySide, Embed, No };
bool is64() const { return llvm::COFF::is64Bit(machine); }

std::unique_ptr<MemoryBuffer> stub;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe dosStub would be clearer?

llvm::COFF::MachineTypes machine = IMAGE_FILE_MACHINE_UNKNOWN;
bool machineInferred = false;
size_t wordsize;
Expand Down
4 changes: 4 additions & 0 deletions lld/COFF/Driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2418,6 +2418,10 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
config->noSEH = args.hasArg(OPT_noseh);
}

// Handle /stub
if (auto *arg = args.getLastArg(OPT_stub))
parseStub(arg->getValue());

// Handle /functionpadmin
for (auto *arg : args.filtered(OPT_functionpadmin, OPT_functionpadmin_opt))
parseFunctionPadMin(arg);
Expand Down
3 changes: 3 additions & 0 deletions lld/COFF/Driver.h
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,9 @@ class LinkerDriver {
void parseSection(StringRef);
void parseAligncomm(StringRef);

// Parses a MS-DOS stub file
void parseStub(StringRef path);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you renamed this to parseDosStub there would be no need to have a comment for it.


// Parses a string in the form of "[:<integer>]"
void parseFunctionPadMin(llvm::opt::Arg *a);

Expand Down
15 changes: 15 additions & 0 deletions lld/COFF/DriverUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,21 @@ void LinkerDriver::parseAligncomm(StringRef s) {
std::max(ctx.config.alignComm[std::string(name)], 1 << v);
}

void LinkerDriver::parseStub(StringRef path) {
std::unique_ptr<MemoryBuffer> stub =
CHECK(MemoryBuffer::getFile(path), "could not open " + path);
size_t bufferSize = stub->getBufferSize();
const char *bufferStart = stub->getBufferStart();
// MS link.exe compatibility:
// 1. stub must be greater than or equal to 64 bytes
// 2. stub must be 8-byte aligned
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean the size must be a multiple of 8? Does link.exe really require that? I suppose the PE header needs to be 8-byte aligned, but the linker could easily add padding after the stub.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean the size must be a multiple of 8? Does link.exe really require that?

Yes, I already tested that and confirmed that LINK.exe does not allow unaligned stub input. :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel too strongly about it, but unless there's a technical reason to reject such stubs, I think we should accept also non-8-byte-aligned programs and add any padding ourselves.

https://learn.microsoft.com/en-us/cpp/build/reference/stub-ms-dos-stub-file-name?view=msvc-170 says "any valid MS-DOS application [.exe file] can be a stub program", for example.

Copy link
Contributor Author

@kkent030315 kkent030315 Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I am pretty sure we should keep MZ dos signature check. On the other hand, there's nothing prevents us to accept unaligned stubs, if we don't have to replicate MS linker bug-by-bug. It's implemented on 94ac28f183d994e0ba5f61a9fcc788369be95970 and I'd love to hear feedbacks from you :) .

// 3. stub must be start with a valid dos signature 'MZ'
if (bufferSize < 0x40 || bufferSize % 8 != 0 ||
(bufferStart[0] != 'M' || bufferStart[1] != 'Z'))
Err(ctx) << "/stub: invalid format for MS-DOS stub file: " << path;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful if the error message could mention which requirement (size, signature, ..) failed.

(Useless trivia: 'zm' is also a valid dos .exe signature, but the windows loader doesn't like it: https://www.hanshq.net/making-executables.html#zm)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I'll improve the error messages. And also I tested that ZM with LINK.exe and confirmed that it does not allowed so I think we should keep the DOS magic thing there.

ctx.config.stub = std::move(stub);
}

// Parses /functionpadmin option argument.
void LinkerDriver::parseFunctionPadMin(llvm::opt::Arg *a) {
StringRef arg = a->getNumValues() ? a->getValue() : "";
Expand Down
60 changes: 41 additions & 19 deletions lld/COFF/Writer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,8 @@ static unsigned char dosProgram[] = {
};
static_assert(sizeof(dosProgram) % 8 == 0,
"DOSProgram size must be multiple of 8");

static const int dosStubSize = sizeof(dos_header) + sizeof(dosProgram);
static_assert(dosStubSize % 8 == 0, "DOSStub size must be multiple of 8");
static const uint32_t coffHeaderOffset = dosStubSize + sizeof(PEMagic);
static const uint32_t peHeaderOffset =
coffHeaderOffset + sizeof(coff_file_header);
static const uint32_t dataDirOffset64 =
peHeaderOffset + sizeof(pe32plus_header);
static_assert((sizeof(dos_header) + sizeof(dosProgram)) % 8 == 0,
"DOSStub size must be multiple of 8");

static const int numberOfDataDirectory = 16;

Expand Down Expand Up @@ -214,6 +208,7 @@ class Writer {
void run();

private:
void calculateStubDependentSizes();
void createSections();
void createMiscChunks();
void createImportTables();
Expand Down Expand Up @@ -315,6 +310,11 @@ class Writer {
uint64_t sizeOfImage;
uint64_t sizeOfHeaders;

uint32_t dosStubSize;
uint32_t coffHeaderOffset;
uint32_t peHeaderOffset;
uint32_t dataDirOffset64;

OutputSection *textSec;
OutputSection *hexpthkSec;
OutputSection *rdataSec;
Expand Down Expand Up @@ -762,6 +762,7 @@ void Writer::run() {
llvm::TimeTraceScope timeScope("Write PE");
ScopedTimer t1(ctx.codeLayoutTimer);

calculateStubDependentSizes();
if (ctx.config.machine == ARM64X)
ctx.dynamicRelocs = make<DynamicRelocsChunk>();
createImportTables();
Expand Down Expand Up @@ -1035,6 +1036,17 @@ void Writer::sortSections() {
sortBySectionOrder(it.second->chunks);
}

void Writer::calculateStubDependentSizes() {
if (ctx.config.stub)
dosStubSize = ctx.config.stub->getBufferSize();
else
dosStubSize = sizeof(dos_header) + sizeof(dosProgram);

coffHeaderOffset = dosStubSize + sizeof(PEMagic);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the PEMagic needs to be 8-byte aligned, I think we need to round up dosStubSize here: alignTo(dosStubSize, 8).

Also, we need to audit all the uses of dosStubSize. For example, Writer::writePEChecksum() uses dosStubSize to locate the coffHeader. It should use coffHeaderOffset instead.

peHeaderOffset = coffHeaderOffset + sizeof(coff_file_header);
dataDirOffset64 = peHeaderOffset + sizeof(pe32plus_header);
}

// Create output section objects and add them to OutputSections.
void Writer::createSections() {
llvm::TimeTraceScope timeScope("Output sections");
Expand Down Expand Up @@ -1668,21 +1680,31 @@ template <typename PEHeaderTy> void Writer::writeHeader() {
// When run under Windows, the loader looks at AddressOfNewExeHeader and uses
// the PE header instead.
Configuration *config = &ctx.config;

uint8_t *buf = buffer->getBufferStart();
auto *dos = reinterpret_cast<dos_header *>(buf);
buf += sizeof(dos_header);
dos->Magic[0] = 'M';
dos->Magic[1] = 'Z';
dos->UsedBytesInTheLastPage = dosStubSize % 512;
dos->FileSizeInPages = divideCeil(dosStubSize, 512);
dos->HeaderSizeInParagraphs = sizeof(dos_header) / 16;

dos->AddressOfRelocationTable = sizeof(dos_header);
dos->AddressOfNewExeHeader = dosStubSize;

// Write DOS program.
memcpy(buf, dosProgram, sizeof(dosProgram));
buf += sizeof(dosProgram);
if (config->stub) {
memcpy(buf, config->stub->getBufferStart(), config->stub->getBufferSize());
// MS link.exe accepts an invalid `e_lfanew` (AddressOfNewExeHeader) and
// updates it automatically. Replicate the same behaviour.
dos->AddressOfNewExeHeader = config->stub->getBufferSize();
buf += config->stub->getBufferSize();
} else {
buf += sizeof(dos_header);
dos->Magic[0] = 'M';
dos->Magic[1] = 'Z';
dos->UsedBytesInTheLastPage = dosStubSize % 512;
dos->FileSizeInPages = divideCeil(dosStubSize, 512);
dos->HeaderSizeInParagraphs = sizeof(dos_header) / 16;

dos->AddressOfRelocationTable = sizeof(dos_header);
dos->AddressOfNewExeHeader = dosStubSize;

memcpy(buf, dosProgram, sizeof(dosProgram));
buf += sizeof(dosProgram);
}

// Write PE magic
memcpy(buf, PEMagic, sizeof(PEMagic));
Expand Down
Binary file added lld/test/COFF/Inputs/stub63mz
Binary file not shown.
Binary file added lld/test/COFF/Inputs/stub64mz
Binary file not shown.
Binary file added lld/test/COFF/Inputs/stub64zz
Binary file not shown.
Binary file added lld/test/COFF/Inputs/stub68mz
Binary file not shown.
33 changes: 33 additions & 0 deletions lld/test/COFF/stub.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# RUN: yaml2obj %p/Inputs/ret42.yaml -o %t.obj

# RUN: lld-link /out:%t.exe /entry:main /stub:%p/Inputs/stub512mz %t.obj
# RUN: llvm-readobj --file-headers %t.exe | FileCheck -check-prefix=CHECK1 %s

CHECK1: Magic: MZ
CHECK1: UsedBytesInTheLastPage: 144
CHECK1: FileSizeInPages: 3
CHECK1: NumberOfRelocationItems: 0
CHECK1: HeaderSizeInParagraphs: 4
CHECK1: MinimumExtraParagraphs: 0
CHECK1: MaximumExtraParagraphs: 65535
CHECK1: InitialRelativeSS: 0
CHECK1: InitialSP: 184
CHECK1: Checksum: 0
CHECK1: InitialIP: 0
CHECK1: InitialRelativeCS: 0
CHECK1: AddressOfRelocationTable: 64
CHECK1: OverlayNumber: 0
CHECK1: OEMid: 0
CHECK1: OEMinfo: 0
CHECK1: AddressOfNewExeHeader: 64

## Invalid DOS signature (must be `MZ`)
# RUN: not lld-link /out:%t.exe /entry:main /stub:%p/Inputs/stub512zz %t.obj 2>&1 | FileCheck -check-prefix=CHECK-INVALID %s

## Unaligned stub (must be aligned to 8)
# RUN: not lld-link /out:%t.exe /entry:main /stub:%p/Inputs/stub516mz %t.obj 2>&1 | FileCheck -check-prefix=CHECK-INVALID %s

## Too-small stub (must be at least 64 bytes long)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this case also fail due to not being aligned to 8 bytes? So without seeing the implementation (or looking at an error message telling the exact reason), we don't strictly know if we're hitting the intended error, or erroring out due to some other reason.

Also, the numbers in the file names, 511/512/516 are quite puzzling how they relate to the actual file sizes (63/64/68 bytes). Can you enlighten me, or rename them to include the actual file size in the name instead :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and nice catch, I'll fix the file names. They used to be 511/512/516 sizes and I compressed before pushing out and forgot to rename them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this case also fail due to not being aligned to 8 bytes? So without seeing the implementation (or looking at an error message telling the exact reason), we don't strictly know if we're hitting the intended error, or erroring out due to some other reason.

Too-small stub (63 bytes) is not aligned to 8 bytes, yes.

For the latter, I was thinking the similar. Maybe it's better to put an exact cause for each malformed patterns. For now I followed the way how LINK.exe puts an error (they only put "invalid format" in any of the case). Should I do that?

Copy link
Contributor Author

@kkent030315 kkent030315 Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be something like:

  if (bufferSize < 0x40)
    Err(ctx) << "/stub: stub must be greater than or equal to 64 bytes: " << path;
  if (bufferSize % 8 != 0)
    Err(ctx) << "/stub: stub must be aligned to 8 bytes: " << path;
  if (bufferStart[0] != 'M' || bufferStart[1] != 'Z')
    Err(ctx) << "/stub: invalid DOS signature: " << path;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could probably be valuable to add more value in the error messages, even if link.exe doesn't; we don't need to match link.exe bit-by-bit exact in such details anyway. (For some larger cases, we print matching LNK123 error numbers, but most of other warnings/errors are different anyway.)

If we can grep for the right error message, this is probably fine, but otherwise it'd be good to make the file e.g. 56 bytes, so that it does pass the alignment test, so we trigger specifically the error we want to test, regardless of in which order the checks are made.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable to me, thanks!

# RUN: not lld-link /out:%t.exe /entry:main /stub:%p/Inputs/stub511mz %t.obj 2>&1 | FileCheck -check-prefix=CHECK-INVALID %s

# CHECK-INVALID: lld-link: error: /stub: invalid format for MS-DOS stub file: {{.*}}
Loading