Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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); }

llvm::SmallVector<uint8_t> stub;
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use std::unique_ptr<MemoryBuffer> here to avoid copying the blob in parseStub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes :).

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 or equal than 64 bytes
Copy link
Member

Choose a reason for hiding this comment

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

Small nitpick, but, I presume you mean "greater than or equal (to)"?

// 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.append(bufferStart, bufferStart + bufferSize);
}

// 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.size())
dosStubSize = ctx.config.stub.size();
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.size()) {
memcpy(buf, config->stub.data(), config->stub.size());
// MS link.exe accepts an invalid `e_lfanew` and updates it automatically.
Copy link
Member

Choose a reason for hiding this comment

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

I presume e_lfanew is a different name for AddressOfNewExeHeader? This is a bit hard to follow if one doesn't have the struct definitions handy while reading the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, dos_header::AddressOfNewExeHeader is IMAGE_DOS_HEADER::e_lfanew defined in COFF.h. I'll mention that in the comment.

// Replicate the same behaviour.
dos->AddressOfNewExeHeader = config->stub.size();
buf += config->stub.size();
} 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/stub511mz
Binary file not shown.
Binary file added lld/test/COFF/Inputs/stub512mz
Binary file not shown.
Binary file added lld/test/COFF/Inputs/stub512zz
Binary file not shown.
Binary file added lld/test/COFF/Inputs/stub516mz
Binary file not shown.
31 changes: 31 additions & 0 deletions lld/test/COFF/stub.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# 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

# RUN: not lld-link /out:%t.exe /entry:main /stub:%p/Inputs/stub512zz %t.obj 2>&1 | FileCheck -check-prefix=CHECK2 %s
# CHECK2: lld-link: error: /stub: invalid format for MS-DOS stub file: {{.*}}

# RUN: not lld-link /out:%t.exe /entry:main /stub:%p/Inputs/stub516mz %t.obj 2>&1 | FileCheck -check-prefix=CHECK3 %s
# CHECK3: lld-link: error: /stub: invalid format for MS-DOS stub file: {{.*}}
Copy link
Member

Choose a reason for hiding this comment

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

As all these three checks for the negative tests are the same, you could just as well use one single CHECK-INVALID: for all of 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.

CHECK{2,3,4} are technically different negative tests.

  • CHECK1: Invalid DOS signature (must be MZ)
  • CHECK2: Unaligned stub (must be aligned to 8)
  • CHECK3: Too-small stub (must be at least 64 bytes long)

I'll add comments for this one as well :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm combining to a single CHECK-INVALID, done.


# RUN: not lld-link /out:%t.exe /entry:main /stub:%p/Inputs/stub511mz %t.obj 2>&1 | FileCheck -check-prefix=CHECK4 %s
# CHECK4: lld-link: error: /stub: invalid format for MS-DOS stub file: {{.*}}
Loading