[PATCH] D109972: Regenerate LC_CODE_SIGNATURE during llvm-objcopy operations

Mon Sep 20 13:01:09 PDT 2021

drodriguez added a comment.

@jhenderson: thanks for the feedback, we will apply those when we figure out the best shape for these changes.

================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:441
+    }
+    object::CodeSignatureSection SignatureBuilder =
+        object::CodeSignatureSection(
----------------
alexander-shaposhnikov wrote:
> drodriguez wrote:
> > alexander-shaposhnikov wrote:
> > > It looks like I've missed the previous diff https://reviews.llvm.org/D109803, sorry about being late.
> > > 
> > > 1/ MachO.h is probably not the ideal place for this functionality, according to the top file comment "MachO.h declares the MachOObjectFile class". My first impression is that the public declaration probably should be placed into a separate file, e.g. "MachOCodeSignature.h" (and we need a detailed comment explaining what this class or function does)
> > > 
> > > 2/ The responsibility of the class CodeSignatureSection seems to be somewhat unclear. Perhaps, we don't really need a class here at all. What would you say to just creating a function, e.g. std::string buildCodeSignatureStab(...) or void writeCodeSignatureStab(...) with some clear separation of the input / output and removing the unrelated pieces of functionality (e.g. msync on the buffer, this is probably not suitable for a library function) ? (and not expose the implementation details in the header file).
> > > 
> > > 3/ Some minor comments, e.g. stripOutputFilePath - I suspect Suport/Path.h might already have some helper utilities
> > > 
> > > What I would recommend here - can we (temporarily) take a step back and  reopen & revert D109803, if it were not a library interface I would not be so worried, but for some common and frequently used libraries like libObject I would highly suggest we should clean up the interface first and I'm more than happy to help.
> > > 
> > > 
> > > 
> > Hi Alex,
> > 
> > We would prefer to fix forward instead of reverting, if possible. As it sits, D109803 haven't changed anything and it is just "dormant" code as far as LLVM respects. Only this diff uses that functionality.
> > 
> > I will review your comment next week more carefully and provide better answers.
> > 
> > 1/ I think we can do this. It should not be a problem.
> > 2/ The class tries to imitate the existing code in LLD, which was a class. There's two related functionalities to the class: measuring the size, and serializing. For both one needs the same inputs, so it was interesting to tie data and behaviour together.
> > 2b/ `msync` on the buffer is actually important, and `llvm-objcopy` might not work unless we make that call. IIRC without that, if one was writing the same file, and that file was mmap'ed into memory, the kernel will not realize that the signature had changed, and will not evaluate again. I can look for a reference that explained the problem in a little bit more detail.
> > 3/ We realize that it was not portable for Windows, and Nuri changed that this afternoon to use `llvm::path` instead. This diff has the fix, since the previous one was already landed.
> > 
> > Thanks for the help offering. Any feedback is welcome. We will try to get to the best version of this that satisfies everyone's requirements.
> Hi! Many thanks!
> My 0.02$ . The current design looks a little bit suboptimal, especially for a library, so I suggest let's improve the interface first and put things into the right place. It's a bit hard to post comments outside of the code context, but yeah, I'm late to the party. The LLD-specific details ("CodeSignatureSection") can still live in the LLD codebase and make use of the function from libObject.
> If there are no significant performance differences I would be biased towards readability / simplicity, e.g. one can consider something like this (I didn't verify the details, so maybe some parameters are unnecessary or, instead, missing)
> ```
> SmallVector<char, 64> buildCodeSignatureStab(
>     uint64_t SignatureOffset,
>     MachO::HeaderFileType FileType, StringRef FilePath,
>     uint64_t TextSegmentOffset, StringRef TextSegmentContent)
> 
> ```
> and in LLD the method CodeSignatureSection::finalize would call this function - would that be sufficient ? (maybe I'm missing something, feel free to correct me). (Plus in this case we will avoid having two different  CodeSignatureSection classes)
> and (mostly motivated by the "separation of concerns") I'd leave the unfortunate workaround with msync on the application's side, hopefully it'll be gone in the future.
> 
> 
About the ideal place for this code:

We thought about libObject and specifically MachO.h because it seemed like the best place, but we are fine relocating the code if necessary. We have also seen libBinaryFormat which includes some support for MachO and which might be a good place for these. The basic structs for the code signature (`CS_CodeDirectory`) is there, so it might be a good idea to keep everything together. Our only requirement is that it should be accessible to both llvm-objcopy and LLD to avoid code duplication.

These are not LLD specific details, as far as we understand, but part of how MachO binaries are put together and every tool that creates a MachO binary will need to play by these rules (they apply to each slice independently, so in our testing Universal MachO does not need changes, but there might be edge cases we are not aware).

About the shape of the code:

We don’t mind removing the class and having a couple of free functions instead, but there is the possibility of the responsibilities of the class growing bigger in the future, and we would like to avoid functions with a lot of parameters (there’s already 4 that both the serialization and the calculation of the size would need to receive).

About your proposal of `buildCodeSignatureStab`, is there an example of that pattern already in the code. We have looked for similar constructs and we cannot find it. Also, being “Stab” a term of art related to dSYM, maybe it is not the best name to use (alternatives can be something like “Blob”, or use “serialize” instead of “build”). Also, using `SmallVector` might be limited for this usage since the actual signature depends on the size of the binary, and for each 4 KB of binary we need an extra 32 bytes. I have seen signatures being 15 KB for LLVM binaries. We would prefer the versions that write directly into memory to avoid excessive copy of data.

Finally, about `msync`:

While we can move the `msync` calls into the application code, and add a note in the documentation of the method that the caller must ensure that `msync` gets invoked, we think it is better to keep the `msync` only in one place with a clear explanation of why it is necessary.

The details are in https://openradar.appspot.com/FB8914231 but in short, when mmaping an executable, the kernel seems to cache the results of the signature verification and might not check for the validity of the binary again, even if the signature gets regenerated. This is very typical while iterating on a binary, and creating the binary over and over, so it is something that can happen in both LLD and llvm-objcopy (specially in llvm-strip).

Apple might fix their side eventually, but until then we need the workaround everywhere, so we think a central place is better.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109972/new/

https://reviews.llvm.org/D109972