[Object] Skip section offset checking for /<XFGHASHMAP>/

[psamolysov-intel]

Summary

Starting from Windows SDK for Windows 11 (10.0.22000.x), all the system
libraries (.lib files) contain a section with the '/<XFGHASHMAP>/' name.
This looks like the libraries are built with control flow guard enabled:
https://docs.microsoft.com/en-us/cpp/build/reference/guard-enable-control-flow-guard?view=msvc-170

To let the LLVM tools (llvm-ar, llvm-lib) work with these libraries,
this patch just skips the section offset check for sections with the
'/<XFGHASHMAP>/' name.

Closes: llvm/llvm-project#53814

Signed-off-by: Pavel Samolysov <[e-mail]>

[thieta]

Thanks for doing this. I think it looks sane over all, but I would like to see:

• Better comment about the symbol
• A test that cover this symbol and that we skip it and not bomb out, like we currently do.

llvm/lib/Object/Archive.cpp

259

I think this comment could be more descriptive. We should say what this symbol is for and that we just skip it for now.

[mstorsjo]

Can you add a testcase, e.g. somewhere in llvm/test/tools/llvm-ar or similar?

llvm/lib/Object/Archive.cpp

166

This change seems unrelated to the rest. Even if some cleanliness tool might complain about deviations from the coding convention, we normally don't change much outside of what the patch itself covers.

[psamolysov-intel]

Thank you for the comments. I'm going to revert the part about end->End renaming and add a test (or a few).

[psamolysov-intel]

Unfortunately, I have no idea how to create a library with the /<XFGHASHMAP>/ section. I'm trying to use /guard:cf directive for the compiler as well as for the linker with VS 2019 and 2022 but the generated library contains no such section. I've seen such section only in the standard libraries shipped with Microsoft Windows SDK for Windows 11 but (Kernel32.Lib, for example), as I understand, we cannot attach a 3rd party (Microsoft in this case) library to our project. I tried to create a DLL and interface library for it with a dependency from another DLL or a DLL that contain the DllMain entry point, no success. Any help is welcome.

[jhenderson]

In D120645#3348913, @psamolysov-intel wrote:

Unfortunately, I have no idea how to create a library with the /<XFGHASHMAP>/ section. I'm trying to use /guard:cf directive for the compiler as well as for the linker with VS 2019 and 2022 but the generated library contains no such section. I've seen such section only in the standard libraries shipped with Microsoft Windows SDK for Windows 11 but (Kernel32.Lib, for example), as I understand, we cannot attach a 3rd party (Microsoft in this case) library to our project. I tried to create a DLL and interface library for it with a dependency from another DLL or a DLL that contain the DllMain entry point, no success. Any help is welcome.

I'm not really familiar with the Windows lib format, but in traditional GNU archives, you can basically just concatenate a new archive member to the end of an existing archive and it'll just work. If that works for Windows libs too, you could do that using python, maybe?

[psamolysov-intel]

Thanks @jhenderson for the idea to use python to insert (or replace in my case) a section. I've updated the patch and added a test for the llvm-lib tool to list sections from a library with /<XFGHASHMAP>/. I've made a patch using the git diff -U8000 command and I'm not sure the binary file (the library for the test) has been uploaded well. If not, could you share with me the required command to attach the binary content to the patch?

[jhenderson]

In D120645#3350478, @psamolysov-intel wrote:

Thanks @jhenderson for the idea to use python to insert (or replace in my case) a section. I've updated the patch and added a test for the llvm-lib tool to list sections from a library with /<XFGHASHMAP>/. I've made a patch using the git diff -U8000 command and I'm not sure the binary file (the library for the test) has been uploaded well. If not, could you share with me the required command to attach the binary content to the patch?

We try to avoid using canned binaries as test inputs. Could you generate an archive at test time, and then append the relevant bit using python then? There are a few examples in the lit testsuite of invoking python at test time to do various bits of manipulation.

FWIW, I don't believe you can upload a binary to Phabricator.

[psamolysov-intel]

Thank you for the suggestion. I've added a python script to replace a section with /<XFGHASHMAP>/ and re-created a test to generate the library on the fly.

[thieta]

Thanks! This looks great!

[psamolysov-intel]

@thieta Thank you for the approve. I have no commit right to the repository, so it would be great if someone with the commit rights lands the patch.

[thieta]

In D120645#3350669, @psamolysov-intel wrote:

@thieta Thank you for the approve. I have no commit right to the repository, so it would be great if someone with the commit rights lands the patch.

I can land it for you - but I would like another reviewer to approve it as well first.

[jhenderson]

Basically looks fine, although I'm not a Windows archive expert. @mstorsjo might have some other feedback.

llvm/lib/Object/Archive.cpp

259–260

Some grammar nits.

llvm/test/tools/llvm-lib/Inputs/xfghashmap-inserter.py

8 ↗

(On Diff #412000)

I believe you can just do this?

llvm/test/tools/llvm-lib/xfghashmap-list.test

2–6

You can simplify this slightly.

8

Rather than put the python file in a separate file to the test file, you can include it inline as follows:

1. Add comment markers to the start of every RUN and CHECK line, (e.g. # RUN: ...).
2. Change the existing comment to use double-comment markers (not strictly needed, but it helps it stand out (i.e. ## This should ...).
3. Add the contents of the python script to the end of this test file.
4. Reference the script by using %s in your python execution.

Also, I'd put a brief comment somewhere, either at the start of the python script, or immediately before this RUN line, explaining what the script does.

10

Nit: missing trailing "." at the end of the comment.

[psamolysov-intel]

@jhenderson thank you for the review. I'm not an English native speaker, so some grammar may be bad, sorry. I've put the python script into the test itself as you suggested and added a few comments why this script is used at all.

[psamolysov-intel]

llvm/lib/Object/Archive.cpp

259–260

Thank you, I've applied the suggestion.

llvm/test/tools/llvm-lib/xfghashmap-list.test

8

Thank you for the idea, I've encapsulated the script into the test, it works!

[mstorsjo]

I’m not entirely clear about the big picture here… The real Windows SDK library files contain members that cause us to error out (for some reason), and we choose to just skip those members, to avoid erroring out.

To test this, we artificially create libraries with a member named like this (but which is a real regular object file), and we check that we ignore it. But the test doesn’t replicate the aspect of the real files that made us error out on them to begin with, right? So the testcase can’t be used for testing other ways of dealing with the root cause, other than ignoring - is that right?

(I’m not saying the it has to be done differently, I’m just trying to get a grasp of the issue and what this tests.)

[psamolysov-intel]

@mstorsjo I agree with you, we make some manipulations to get a library with a member named like this. Unfortunately, the member is not documented (or I cannot find the documentation) and it is difficult to build the right library using the Microsoft toolchain.

[jhenderson]

LGTM, but please work with @mstorsjo about the testing etc.

llvm/test/tools/llvm-lib/xfghashmap-list.test

25

Still can use a byte literal as suggested earlier.

[psamolysov-intel]

@jhenderson Thank you, I missed the suggestion about the byte literal, sorry. Updated the test.

[psamolysov-intel]

x64 debian failed

I see there is no failed test in LLVM, only some tests in Clang but they look as not related to the patch.

[jhenderson]

In D120645#3353560, @psamolysov-intel wrote:

x64 debian failed

I see there is no failed test in LLVM, only some tests in Clang but they look as not related to the patch.

That's not unusual. It means there were issues in main at the point the auto-test ran.

[mstorsjo]

In D120645#3351103, @jhenderson wrote:

LGTM, but please work with @mstorsjo about the testing etc.

No objections from me, thanks for clarifying the situation.

Do you have commit access to push the patch, or do you want me to do it for you? In that case, can you provide your preferred git author line ("Real Name <email@address>") to use for pushing the commit? (Via the github issue I do see your own commit for this patch too, at https://github.com/psamolysov-intel/llvm/commit/7467ea9a6a173829ab61693dd199ae0d94a0a787, so I guess I could pick your preferred info from there too.)

[psamolysov-intel]

Thank you @mstorsjo! I have no commit access, if you can commit this patch, it would be great. You can use the info from https://github.com/psamolysov-intel/llvm/commit/7467ea9a6a173829ab61693dd199ae0d94a0a787

[psamolysov-intel]

Thank you @mstorsjo for landing.

llvm/lib/Object/Archive.cpp

Expected<StringRef> ArchiveMemberHeader::getRawName() const {
   else
     EndCond = '/';
   StringRef::size_type end =
       StringRef(ArMemHdr->Name, sizeof(ArMemHdr->Name)).find(EndCond);
   if (end == StringRef::npos)
     end = sizeof(ArMemHdr->Name);
   assert(end <= sizeof(ArMemHdr->Name) && end > 0);
   // Don't include the EndCond if there is one.
   return StringRef(ArMemHdr->Name, end);
 }
 
 Expected<uint64_t>
 getArchiveMemberDecField(Twine FieldName, const StringRef RawField,
                          const Archive *Parent,
                          const AbstractArchiveMemberHeader *MemHeader) {
   uint64_t Value;
   if (RawField.getAsInteger(10, Value)) {
Expected<StringRef> ArchiveMemberHeader::getName(uint64_t Size) const {
   StringRef Name = NameOrErr.get();
 
   // Check if it's a special name.
   if (Name[0] == '/') {
     if (Name.size() == 1) // Linker member.
       return Name;
     if (Name.size() == 2 && Name[1] == '/') // String table.
       return Name;
+    // System libraries from the Windows SDK for Windows 11 contain this symbol.
+    // It looks like a CFG guard: we just skip it for now.
+    if (Name.equals("/<XFGHASHMAP>/"))
+      return Name;
     // It's a long name.
     // Get the string table offset.
     std::size_t StringOffset;
     if (Name.substr(1).rtrim(' ').getAsInteger(10, StringOffset)) {
       std::string Buf;
       raw_string_ostream OS(Buf);
       OS.write_escaped(Name.substr(1).rtrim(' '));
       OS.flush();

llvm/test/tools/llvm-lib/xfghashmap-list.test

+# RUN: rm -rf %t && mkdir -p %t && cd %t
+# RUN: llvm-mc -triple=x86_64-pc-windows-msvc -filetype=obj -o a.obj %S/Inputs/a.s
+# RUN: llvm-mc -triple=x86_64-pc-windows-msvc -filetype=obj -o b.obj %S/Inputs/b.s
+# RUN: llvm-lib /out:xfghashmap.lib a.obj b.obj
+
+## Replace a section in the library file with /<XFGHASHMAP>/ emulating
+## a library from the Windows SDK for Windows 11.
+# RUN: %python %s xfghashmap.lib b.obj/
+
+## This should print the /<XFGHASHMAP>/ section as well as an .obj one.
+# RUN: llvm-lib /list %t/xfghashmap.lib | FileCheck %s
+
+# CHECK: a.obj
+# CHECK: /<XFGHASHMAP>/
+# CHECK-NOT: b.obj
+
+import sys
+
+if len(sys.argv) < 3:
+  print("Use: python3 xfghashmap-list.test <LIBRARY_FILE> <TEMPLATE>")
+  exit(1)
+
+template = bytes(sys.argv[2], 'utf-8')
+xfghashmap = b'/<XFGHASHMAP>/'
+
+data = None
+with open(sys.argv[1], "rb") as inp:
+  data = inp.read()
+with open(sys.argv[1], "wb") as outp:
+  pos = data.find(template)
+  outp.write(data[:pos])
+  outp.write(xfghashmap)
+  outp.write(data[pos + len(xfghashmap):])