Concep ACK. Was working on similar preventing #21771 right now :)

Updated to fix a typo, and address the CI failure.

@fanquake

I would add a check for the version of the macOS SDK using during compilation (which should help prevent issues like #21771), however there seems to be a bug in LIEF when accessing binary.build_version.sdk (although not when sdk is printed as part of build_version). I have opened an issue upstream: lief-project/LIEF#577.

Are we going to wait for LIEF 0.12 with a fix, or macOS SDK version check could be added in a separated PR?

Are we going to wait for LIEF 0.12 with a fix, or macOS SDK version check could be added in a separated PR?

I've just added one here. Apparently LIEF 0.12 wont be released until September / October.

https://cirrus-ci.com/task/5407569322704896?logs=lint#L855:

contrib/devtools/symbol-check.py:232: error: Item "None" of "Optional[Match[str]]" has no attribute "groups"
Found 1 error in 1 file (checked 200 source files)
^---- failure generated from test/lint/lint-python.sh

How about to check the solely MIN_OS error firing:

--- a/contrib/devtools/test-symbol-check.py
+++ b/contrib/devtools/test-symbol-check.py
@@ -142,6 +142,19 @@ class TestSymbolChecks(unittest.TestCase):
         self.assertEqual(call_symbol_check(cc, source, executable, ['-mmacosx-version-min=10.14', '-Wl,-platform_version', '-Wl,macos', '-Wl,10.14', '-Wl,10.15.6']),
                 (0, ''))
 
+        source = 'test5.c'
+        executable = 'test5'
+        with open(source, 'w', encoding="utf8") as f:
+            f.write('''
+                int main()
+                {
+                    return 0;
+                }
+        ''')
+
+        self.assertEqual(call_symbol_check(cc, source, executable, ['-mmacosx-version-min=10.14', '-Wl,-platform_version', '-Wl,macos', '-Wl,10.15.6', '-Wl,10.15.6']),
+                (1, executable + ': failed MIN_OS'))
+
     def test_PE(self):
         source = 'test1.c'
         executable = 'test1.exe'

?

Maybe add a test for the SUBSYSTEM_VERSION error to the test_PE function of the test-symbol-check.py?

contrib/devtools/symbol-check.py:232: error: Item "None" of "Optional[Match[str]]" has no attribute "groups"

Addressed.

How about to check the solely MIN_OS error firing:

There is some overlap between the MIN_OS and SDK test. I think we have enough coverage in this regard.

Maybe add a test for the SUBSYSTEM_VERSION error to the test_PE function of the test-symbol-check.py?

Added a test for this.

Assuming that test-symbol-check.py could and should be ran for cross builds, I see this PR closely related to #20980.

At least #20980 allows to make test-security-check for macOS cross builds on Linux.

After rebasing this PR on top of #20980 getting an error:

$ make test-security-check
.
----------------------------------------------------------------------
Ran 1 test in 0.484s

OK
F
======================================================================
FAIL: test_MACHO (__main__.TestSymbolChecks)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./contrib/devtools/test-symbol-check.py", line 102, in test_MACHO
    self.assertEqual(call_symbol_check(cc, source, executable, ['-lexpat']),
AssertionError: Tuples differ: (1, '[28 chars]LLOWED_LIBRARIES!\ntest1: failed DYNAMIC_LIBRARIES') != (1, '[28 chars]LLOWED_LIBRARIES!\ntest1: failed DYNAMIC_LIBRARIES MIN_OS SDK')

First differing element 1:
'libe[23 chars]ALLOWED_LIBRARIES!\ntest1: failed DYNAMIC_LIBRARIES'
'libe[23 chars]ALLOWED_LIBRARIES!\ntest1: failed DYNAMIC_LIBRARIES MIN_OS SDK'

  (1,
   'libexpat.1.dylib is not in ALLOWED_LIBRARIES!\n'
-  'test1: failed DYNAMIC_LIBRARIES')
+  'test1: failed DYNAMIC_LIBRARIES MIN_OS SDK')
?                                  +++++++++++


----------------------------------------------------------------------
Ran 1 test in 0.093s

FAILED (failures=1)
make: *** [Makefile:1444: test-security-check] Error 1

@fanquake

Are we going to wait for LIEF 0.12 with a fix, or macOS SDK version check could be added in a separated PR?

I've just added one here. Apparently LIEF 0.12 wont be released until September / October.

The fix is included in LIEF 0.11.5.
From the changelog:

:MachO:
  * Fix error on property :attr:`lief.MachO.BuildVersion.sdk` (see :issue:`533`)

At least #20980 allows to make test-security-check for macOS cross builds on Linux.
After rebasing this PR on top of #20980 getting an error:

Ok. What is the actual error though?

The fix is included in LIEF 0.11.5.

I've included an update to 0.11.5 in this PR, and simplified the SDK check.

At least #20980 allows to make test-security-check for macOS cross builds on Linux.
After rebasing this PR on top of #20980 getting an error:

Ok. What is the actual error though?

Well, not an error, but changed behavior: make test-security-check fails (this PR on top of the master + #20980), while it passed before (this PR on top of the master).

Well, not an error, but changed behavior:

I understand. I'm asking is what is the underlying cause of the failure. I assume it's the compiler/linker not accepting some of the newly added flags.

Well, not an error, but changed behavior:

I understand. I'm asking is what is the underlying cause of the failure. I assume it's the compiler/linker not accepting some of the newly added flags.

From #bitcoin-builds channel on 2021-03-13:

21:23:32 <dongcarl> Oh huh... I'm guessing the test-*-check.py scripts don't expect the CC to be set with an SDK?

I've removed the macOS SDK check from test-security-check. I don't think it adds much value, and given we don't control the SDK version being used in the CI, we can't necessarily test for/using the same SDK version we expect in our binaries.

Concept ACK