scripts: security-check.py refactors #18796

pull fanquake wants to merge 4 commits into bitcoin:master from fanquake:security_check_no_more_32bit changing 1 files +45 −79

fanquake commented at 7:31 AM on April 28, 2020: member
- Remove 32-bit Windows checks.
- Remove NONFATAL checking. Added in #8249, however unused since #13764.
- Add run_command to de-duplicate all of the subprocess calls. Mentioned in #18713.
- Add additional type annotations.
- Print stderr when there is an issue running a command.
fanquake added the label Scripts and tools on Apr 28, 2020
fanquake force-pushed on Apr 28, 2020
fanquake force-pushed on Apr 28, 2020
practicalswift commented at 10:47 AM on April 28, 2020: contributor

Concept ACK
in contrib/devtools/security-check.py:25 in 00dd74a25e outdated
```
  26 | -    p = subprocess.Popen([READELF_CMD, '-h', '-W', executable], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True)
  27 | +def run_command(command) -> str:
  28 | +    p = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True)
  29 |      (stdout, stderr) = p.communicate()
  30 |      if p.returncode:
  31 | +        print(stderr)
```
laanwj commented at 9:57 AM on April 30, 2020:

Do we want to print stderr to stderr maybe? (in this case, why PIPE it at all)

I'm not sure Error opening file is a good error message here. Might want to print the command that failed with the exit status.

Launching a command and raising an error on non-zero exit status what subprocess.check_call does. This might be good enough instead of defining our own function.
fanquake commented at 12:12 PM on May 14, 2020:

I've swapped to just using subprocess.run.
laanwj commented at 1:23 PM on April 30, 2020: member

ACK otherwise, NON_FATAL was always a temporary hack it's good to see it gone.
scripts: no-longer check for 32 bit windows in security-check.py 061acf62a1
scripts: remove NONFATAL from security-check.py 13f606b4f9

scripts: add run_command to security-check.py

Deduplicate all the subprocess code as mentioned in 18713.

83d063e954

scripts: add additional type annotations to security-check.py eacedfb023
fanquake force-pushed on May 14, 2020
laanwj commented at 5:59 PM on May 14, 2020: member

ACK eacedfb0230978748cbcfb13817fed7e7c756ba7
laanwj merged this on May 14, 2020
laanwj closed this on May 14, 2020
fanquake deleted the branch on May 14, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#8249 Enable (and check for) 64-bit ASLR on Windows #13764 contrib: Fix test-security-check fail in Ubuntu 18.04 #18713 scripts: Add MACHO stack canary check to security-check.py