[llvm-objcopy] Fix crash when writing empty binary output

[rupprecht]

Summary

When using llvm-objcopy -O binary and the resulting file will be empty (e.g. removing the only section that would be written, or using --only-keep with a section that doesn't exist/isn't SHF_ALLOC), we crash because FileOutputBuffer expects Size > 0. Add a regression test, and change Buffer to open/truncate the output file in this case.

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/binary-output-empty.test

3 ↗

(On Diff #182138)

Nit: existant -> existent

26 ↗

(On Diff #182138)

I'd like this to be above the yaml, to make it easier to see the expected output.

llvm/tools/llvm-objcopy/Buffer.cpp

33 ↗

(On Diff #182138)

Could you return here, to avoid the need to write "else"/indent below?

39 ↗

(On Diff #182138)

There's a weird mismatch between using auto here and not above. I'd err on the side of not using auto in either case.

[rupprecht]

• Misc code review comments

llvm/tools/llvm-objcopy/Buffer.cpp

33 ↗

(On Diff #182138)

I did that... but personally found it to be a bit awkward for some reason. It felt like the return was hidden in a bunch of code.
Instead, I refactored out the bit of creating/truncating the file, and added an early handle+return condition for that instead (so the reverse order), which I think is a more readable/common pattern of returning early. WDYT?

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/Buffer.cpp

31 ↗

(On Diff #182389)

so given we are trying to get rid of "reportError" (to enable us to build a library from this code in the future), maybe we can return an error from the method "allocate" instead ? (similarly to what "commit" does)

[rupprecht]

• Return Error instead of void

llvm/tools/llvm-objcopy/Buffer.cpp

31 ↗

(On Diff #182389)

SGTM, though I'd like to punt on changing the signature of allocate() in this patch. I'll start working on a separate patch now...

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/Buffer.cpp

31 ↗

(On Diff #182389)

oh, probably my bad,
my second thought is the following: basically if i remember correctly we call Buffer::commit in the very end (and in the case of FileBuffer it does smth like renaming the temporary file backing the buffer (which was mmaped into the memory) - i don't remember precisely, need to double check it).
If there is an error in the tool somewhere in the middle of the whole pipeline, the files are untouched and I think it's a nice property to have. But if we truncate a file inside the method "allocate" this won't be true anymore. So ideally all the "real" changes should be applied to the file system only inside the "commit" method + would be cool to have a test for this scenario. What do you think ?

[rupprecht]

Planning to move file truncation from finalize() to commit()

llvm/tools/llvm-objcopy/Buffer.cpp

31 ↗

(On Diff #182389)

No worries, error handling here looks like something we should fix. I have something in progress (https://reviews.llvm.org/differential/diff/182462/) but I actually want to increase the scope of that refactoring, so it's not ready yet. I just want it to be orthogonal to this crash fix :)

I see what you mean about commit(). In the instance of the crash I'm fixing, it doesn't seem like it's at all possible for a crash in between finalize() and write(), but I can't say I've ruled out other situations where Size = 0.
I don't know if it's testable, but I'll try to move this block to commit().

[rupprecht]

• Make sure file modification only happens in commit()

[rupprecht]

llvm/tools/llvm-objcopy/Buffer.cpp

31 ↗

(On Diff #182389)

I couldn't come up with a way to add a test for this, but I manually verified it by throwing in an assert(false) and making sure the file isn't changed.

$ echo abcd > /tmp/existing-file.txt
$ llvm-objcopy -R .text -O binary binary-output-empty.test.tmp.o /tmp/existing-file.txt
<crash>
$ cat /tmp/existing-file.txt
abcd

(Before your suggestion, it was printing an empty file)

Given the state of the code, I don't think a crash is really possible -- unless something like malloc failed -- so I don't have a test to offer.

[jhenderson]

llvm/tools/llvm-objcopy/Buffer.cpp

57 ↗

(On Diff #183125)

Instead of this assert, how practical would it be to call allocate in the constructor?

31 ↗

(On Diff #182389)

This is probably fine as is, but a couple of ideas I had where this might fail (not 100% sure) is a) if another file has a lock on it, or b) if the file is read-only maybe? Are either of those testable in the current framework? I think you could test a) by using python to open it and then spawn llvm-objcopy without closing the file.

[rupprecht]

llvm/tools/llvm-objcopy/Buffer.cpp

57 ↗

(On Diff #183125)

allocate() is often called long after construction, e.g. llvm-objcopy.cpp creates the FileBuffer and ELF/Object.cpp calls allocate() after the object has been processed (and then it knows how much to allocate).

31 ↗

(On Diff #182389)

I think both of those would describe createAndTruncateFile() failures. If commit() fails, then that's "fine" in the sense that there are no side effects -- any file modification should fail. The original problem was:

1. allocate() is called, which truncates some existing data
2. some extra code is called, which crashes
3. commit() is never called, so the processed object is never written, but the original data is still destroyed

vs as implemented in the latest version of the patch:

1. allocate() is called, which remembers to defer truncating until later
2. some extra code is called, which crashes
3. commit() is never called, so truncation never happens and the original data is still there

But, I don't see any reasonable ways for the part in between to crash. The relevent snippet I'm trying to break is this:

// From finalize()
for (auto &Section : Obj.sections()) {
  if ((Section.Flags & SHF_ALLOC) == 0)
    continue;
  AllocatedSections.push_back(&Section);
}
TotalSize = 0;
for (const auto &Section : AllocatedSections) {
  if (Section->Type != SHT_NOBITS)
    TotalSize = std::max(TotalSize, Section->Offset + Section->Size);
}
Buf.allocate(TotalSize);
SecWriter = llvm::make_unique<BinarySectionWriter>(Buf);

// From write()
for (auto &Section : Obj.sections()) {
  if ((Section.Flags & SHF_ALLOC) == 0)
    continue;
  Section.accept(*SecWriter);
}
return Buf.commit();

The things that could fail:

• SecWriter construction fails, e.g. malloc failure
• Section.accept() fails, but given the constraint that TotalSize is 0 (in order for allocate(0) to be called), the only sections it could be called on would be SHT_NOBITS or have zero offset/size. It looks like section visitors check SHT_NOBITS and return early, and I don't know if zero offset/size is valid or possible to construct.

[alexander-shaposhnikov]

thanks!

[jakehehrlich]

Does overwriting a file create a new file? --build-id-link-dir and friends relay on that behavior. In general tools should *never* modify a file in place. Consider any build that uses GN (like Fuchsia, or Chromeium, and now optionally llvm/clang). Say they have a rule (like how we strip in Fuchsia) that writes a file. Now say a copy rule is used to put that somewhere else or you use --build-id-link-dir to copy into a .build-id directory (both happen in Fuchsia). The correct behavior should be for a new file to be created on disk, not modification of the existing one. This for instance keeps the file in the .build-id directory correct. I think this truncates the same existing inode.

[jakehehrlich]

Sorry, meant to request changes.

[rupprecht]

• Make sure a new file is created instead of truncating an inode in place

[rupprecht]

Yup, this was truncating in-place. Changed to nuke the old file + create a new one now (using CD_CreateNew to make sure it's not in-place). PTAL.

[rupprecht]

• Fix casing

[jakehehrlich]

llvm/tools/llvm-objcopy/Buffer.cpp

25–26 ↗

(On Diff #183365)

Remove has a default parameter that sets weather or not it errors out if the file already exists. This removes a (well for this it doesn't actually matter in any case I've seen) race condition. To eliminate the condition entirely you need to create a temporary empty file on the same partition and then rename it into place. That behaves atomically. That's what FileBuffer is doing under the hood.

[rupprecht]

• Use tempfile

[rupprecht]

llvm/tools/llvm-objcopy/Buffer.cpp

25–26 ↗

(On Diff #183365)

Thanks for that pointer! Even though this race condition is probably overkill to worry about, it ends up being much less code to use a temp file anyway, so I'm happy to steal it :D

[rupprecht]

• typo

[jhenderson]

Looks good from my point of view, but obviously wait for @jakehehrlich in particular to confirm he's happy.

[jakehehrlich]

LGTM.

llvm/trunk/test/tools/llvm-objcopy/ELF/binary-output-empty.test

+# RUN: yaml2obj %s -o %t.o
+
+# Writing an empty output to a non-existent file will still create it.
+# RUN: rm -f %t-new.txt
+# RUN: llvm-objcopy -R .text -O binary %t.o %t-new.txt
+# RUN: wc -c %t-new.txt | FileCheck %s
+
+# Writing an empty output to an existing file will truncate it.
+# RUN: echo abcd > %t-existing.txt
+# RUN: llvm-objcopy -R .text -O binary %t.o %t-existing.txt
+# RUN: wc -c %t-existing.txt | FileCheck %s
+
+# In both cases, the file should be empty.
+# CHECK: 0
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Content:         "c3c3c3c3"
+    Size:            0x1000

llvm/trunk/tools/llvm-objcopy/Buffer.h

public:
   virtual Error commit() = 0;
 
   explicit Buffer(StringRef Name) : Name(Name) {}
   StringRef getName() const { return Name; }
 };
 
 class FileBuffer : public Buffer {
   std::unique_ptr<FileOutputBuffer> Buf;
+  // Indicates that allocate(0) was called, and commit() should create or
+  // truncate a file instead of using a FileOutputBuffer.
+  bool EmptyFile = false;
 
 public:
   Error allocate(size_t Size) override;
   uint8_t *getBufferStart() override;
   Error commit() override;
 
   explicit FileBuffer(StringRef FileName) : Buffer(FileName) {}
 };

llvm/trunk/tools/llvm-objcopy/Buffer.cpp

 //===- Buffer.cpp ---------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Buffer.h"
 #include "llvm-objcopy.h"
 #include "llvm/Support/FileOutputBuffer.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Process.h"
 #include <memory>
 
 namespace llvm {
 namespace objcopy {
 
 Buffer::~Buffer() {}
 
+static Error createEmptyFile(StringRef FileName) {
+  // Create an empty tempfile and atomically swap it in place with the desired
+  // output file.
+  Expected<sys::fs::TempFile> Temp =
+      sys::fs::TempFile::create(FileName + ".temp-empty-%%%%%%%");
+  return Temp ? Temp->keep(FileName) : Temp.takeError();
+}
+
 Error FileBuffer::allocate(size_t Size) {
+  // When a 0-sized file is requested, skip allocation but defer file
+  // creation/truncation until commit() to avoid side effects if something
+  // happens between allocate() and commit().
+  if (Size == 0) {
+    EmptyFile = true;
+    return Error::success();
+  }
+
   Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
       FileOutputBuffer::create(getName(), Size, FileOutputBuffer::F_executable);
   // FileOutputBuffer::create() returns an Error that is just a wrapper around
   // std::error_code. Wrap it in FileError to include the actual filename.
   if (!BufferOrErr)
     return createFileError(getName(), BufferOrErr.takeError());
   Buf = std::move(*BufferOrErr);
   return Error::success();
 }
 
 Error FileBuffer::commit() {
+  if (EmptyFile)
+    return createEmptyFile(getName());
+
+  assert(Buf && "allocate() not called before commit()!");
   Error Err = Buf->commit();
   // FileOutputBuffer::commit() returns an Error that is just a wrapper around
   // std::error_code. Wrap it in FileError to include the actual filename.
   return Err ? createFileError(getName(), std::move(Err)) : std::move(Err);
 }
 
 uint8_t *FileBuffer::getBufferStart() {
   return reinterpret_cast<uint8_t *>(Buf->getBufferStart());