Fix radamsa binary for ARM64

[JuanMBriones]

When executing Mac ARM64 swarming tasks we execute radamsa binary,
hence having it as a ARM64 is needed.

• Ran python butler.py py_unittest -t core -p engine_common_test.py (passed).

Bug: 561690132

[g-ortuno]

It's not great that we have "resources/platform/[platform]" AND "fuzzers/bin/[platform]". Could we move RADAMSA to "resources/platform/" and then do something like:

def get_radamsa_path():
  """Return path to radamsa binary for current platform."""
  if environment.platform() not in ('LINUX', 'MAC'):
    return None
  return environment.get_default_tool_path('radamsa')

[JuanMBriones]

It's not great that we have "resources/platform/[platform]" AND "fuzzers/bin/[platform]". Could we move RADAMSA to "resources/platform/" and then do something like:

def get_radamsa_path():
  """Return path to radamsa binary for current platform."""
  if environment.platform() not in ('LINUX', 'MAC'):
    return None
  return environment.get_default_tool_path('radamsa')

Pls excuse the delay. I think I got a better solution for the platform + arch binaries.

For x86 binaries they'll be located on resources/platform/[platform] and for other archs (different than x86) they will be located on resources/platform/[platform]/arch/

Pls let me know what you think:D

[g-ortuno]

So you're saying we would have:

resources/
  platform/
    macos/
      llvm-symbolizer
      radamsa
      arm64/
        llvm-symbolizer
        radamsa

Instead of:

resources/
  platform/
    macos/
      llvm-symbolizer
      radamsa
    macos_arm64/
      llvm-symbolizer
      radamsa

Was there an issue with the latter option? I don't have a super strong opinion with a small preference for the second one since it's less nested dirs and matches the format of deployment targets i.e. macos and macos_arm64.

[JuanMBriones]

So you're saying we would have:

resources/
  platform/
    macos/
      llvm-symbolizer
      radamsa
      arm64/
        llvm-symbolizer
        radamsa

Instead of:

resources/
  platform/
    macos/
      llvm-symbolizer
      radamsa
    macos_arm64/
      llvm-symbolizer
      radamsa

Was there an issue with the latter option? I don't have a super strong opinion with a small preference for the second one since it's less nested dirs and matches the format of deployment targets i.e. macos and macos_arm64.

Yeah I opted for the first option. But, tbh I would love feedback on it. I'm open to choose what will suit the best:D

AFAIK platform is the operating system itself and I think is logical to have a subsection for the archtecture and not blend it together with the platform.

On the other hand, I support your arguments and I think having the same naming as the deployment target is a better practice as well.

There's another thing I've been looking into the last couple of days... having a Universal executable at least for Mac 1,2 it basically have a header that is used by Mac to determine whether a executable might me compatible with the current architecture (the same as when we use file util). Meaning that we can avoid this at least for Mac.

[g-ortuno]

Up to you which dir structure you prefer! RE: the universal binary, that seems cool, but I think it adds another layer of complexity e.g. we would have to manually package all of our binaries. That seems more complicate than just downloading the right binaries to the right directory.

[g-ortuno]

Just one comment otherwise, lgtm

[g-ortuno]

@PauloVLB PTAL!

[PauloVLB]

I might be missing something, but I think this could break radamsa on Linux and Intel Mac.

get_radamsa_path() now resolves to resources/platform/<os>/radamsa, but only mac_arm64/radamsa was added. On Linux, resources/platform/linux/radamsa is an existing folder (the one with libradamsa.so), and on Intel Mac resources/platform/mac/radamsa doesn't exist. The real binaries still seem to be in bot/fuzzers/bin/{linux,mac}/, and I couldn't find anything in the stack moving them.

I wrote a quick test that checks the returned path is an actual file (without mocking os.chmod): https://paste.googleplex.com/4727108878860288

To try it, save it as src/clusterfuzz/_internal/tests/core/bot/fuzzers/radamsa_path_check_test.py and run:
python butler.py py_unittest -t core -p radamsa_path_check_test.py

It passes on master, but fails on this branch: Linux returns a directory, and Intel Mac raises FileNotFoundError since the file doesn't exist.

Is there a move I'm not seeing? If not, would it make sense to keep the bin/ layout and just add bin/mac_arm64/radamsa? Maybe also return None if the file is missing, so it doesn't raise mid-task.

[g-ortuno]

Replying since I'm the one that suggested moving the file. I think it's confusing for us to have two directories with binaries: bin/ and resources/platform/, so I suggested we move radamsa to resources/platform/. I see that we missed moving the existing ones, which we should fix, but do you think we could standardize on a single directory for binaries?

[PauloVLB]

Sounds good to me! One catch is that resources/platform/linux/radamsa is already a folder, so we can't just move the binary there. But it looks like #3131 removed the only thing using it, and nothing references it anymore (worth double-checking), so I think we can just delete it.

[g-ortuno]

How about we special-case macOS arm64 in this method and then we follow up with moving radamsa for mac or removing it for linux? That way we can revert that PR if it breaks things.

[PauloVLB]

Sounds good to me!

[JuanMBriones]

Thanks! Just made these changes on latest PS. Can you pls take a look @PauloVLB ?

[g-ortuno]

I keep forgetting to bring this up, but why is this line needed? The binary on linux is executable already and we don't need to chmod it.

[JuanMBriones]

Just resolved this. It's pretty interesting!!
Why it is not necessary for other platforms? Because it works on other platforms, so it was an error on Mac ARM64 side. As this is a path that is referenced on the actual bot. It means that it had something to do with the actual files that are being passed to the actual bot. So the error must be on the startup scripts.

It turns out that ZipFile.extractall() deletes all UNIX permissions and this is a known issue since 2012 (lol) python/cpython#59999 There's an ongoing PR to solve this python/cpython#150061 yet it's still in the limbo of review, on top of that add that we might wait a bit till the upstream changes gets reflected on our CIPD packages. That's why I tried to mimic the solution on startup scripts.

I filed a PR (382) on Cf config

[JuanMBriones]

I did some experiments. Also I'm leaving the results here for future reference

$ cat /tmp/check_permissions_extractall.py
import os
import stat
import tempfile
import sys
import zipfile


def main():
	archive_name = "../clusterfuzz/deployment/macos_arm64-3.zip"
	with tempfile.TemporaryDirectory() as tmpdir:
		with zipfile.ZipFile(archive_name, "r") as zip_ref:
			zip_ref.extractall(tmpdir)
		for rel in ["clusterfuzz/resources/platform/mac_arm64/radamsa", "clusterfuzz/resources/platform/mac_arm64/llvm-symbolizer"]:
			p = os.path.join(tmpdir, rel)
			mode = stat.S_IMODE(os.stat(p).st_mode)
			print(f"   {rel} -> {oct(mode)} (executable={os.access(p, os.X_OK)})")
			assert os.access(p, os.X_OK), f"{rel} is not executable!"
			print(f"OK: {rel} -> {oct(mode)} (executable={os.access(p, os.X_OK)})")
#def

if __name__ == "__main__":
	main()
$ python3 /tmp/check_permissions_extractall.py
   clusterfuzz/resources/platform/mac_arm64/radamsa -> 0o640 (executable=False)
Traceback (most recent call last):
  File "/tmp/check_permissions_extractall.py", line 22, in <module>
    main()
    ~~~~^^
  File "/tmp/check_permissions_extractall.py", line 17, in main
    assert os.access(p, os.X_OK), f"{rel} is not executable!"
           ~~~~~~~~~^^^^^^^^^^^^
AssertionError: clusterfuzz/resources/platform/mac_arm64/radamsa is not executable!

$ cat /tmp/check_permissions_restore_permissions.py
import os
import stat
import tempfile
import sys

sys.path.insert(0, "../startup_scripts/swarming")
import setup_mac_env


def main():
	with tempfile.TemporaryDirectory() as tmpdir:
		setup_mac_env._extract_zip("../clusterfuzz/deployment/macos_arm64-3.zip", tmpdir)
		for rel in ["clusterfuzz/resources/platform/mac_arm64/radamsa", "clusterfuzz/resources/platform/mac_arm64/llvm-symbolizer"]:
			p = os.path.join(tmpdir, rel)
			mode = stat.S_IMODE(os.stat(p).st_mode)
			assert os.access(p, os.X_OK), f"{rel} is not executable!"
			print(f"OK: {rel} -> {oct(mode)} (executable={os.access(p, os.X_OK)})")
#def

if __name__ == "__main__":
	main()
python3 /tmp/check_permissions_restore_permissions.py
OK: clusterfuzz/resources/platform/mac_arm64/radamsa -> 0o750 (executable=True)
OK: clusterfuzz/resources/platform/mac_arm64/llvm-symbolizer -> 0o755 (executable=True)