[scan-build-py] use subprocess wrapper

[rizsotto.mailinglist]

Summary

It's a chunk from D26390

Introduce run_command for subprocess.check_ouput

• which logs the command execution details
• has the same behavior cross different python versions

Kills SELinux check, which would prevent the dynamic loader trick when that is enabled. Tested on Fedora it works fine.

[dcoughlin]

Thanks for breaking up the patch!

Does this mean that library-preload interposition works on SELinux?

LGTM with the added comment noting the python version where check_output()'s behavior changed.

tools/scan-build-py/libscanbuild/__init__.py

50

I think it would be good to mention what version of python the behavior changed in. This will allow future maintainers to determine whether this logic can safely be removed when support for earlier python versions is dropped.

tools/scan-build-py/libscanbuild/runner.py

126

This change doesn't seem to be related to the functionality mentioned in the summary. Should it be in a separate commit? LLVM's incremental development policy encourages unrelated changes to be in separate patches/commits. This makes it easier for maintainers to determine which commit caused a regression and revert if it necessary. It also makes it easier (and therefore faster) to review.

[rizsotto.mailinglist]

About the SELinux change. If the SELinux setup forbid the library preload, it won't work. But till now I have not met system which was configured like that. (Tested on Fedora 22-25.)

tools/scan-build-py/libscanbuild/__init__.py

50

Good idea, but Python does not work like this. Python packages are back ported to earlier Python versions. So a statement that describe this piece of code needed by version X might be obsolete later. If I can determinate which version requires the decode method, I would have been set up the condition based on the Python version numbers. But I can't...

tools/scan-build-py/libscanbuild/runner.py

126

I agree, that it was also a bugfix too. Next time I will commit those in separate commits.

[dcoughlin]

In D29245#659746, @rizsotto.mailinglist wrote:

About the SELinux change. If the SELinux setup forbid the library preload, it won't work. But till now I have not met system which was configured like that. (Tested on Fedora 22-25.)

Is there an easy way to detect at run time whether the SELinux setup forbids library preload?

tools/scan-build-py/libscanbuild/__init__.py

50

I'm not asking you to use a version number to determine whether decoding is needed at run time. Instead, I'm asking you to document the version number *after which either decoding is always needed or it is always not*. That is, what is the version of python after which the instance check is definitely not needed?

This will enable us to remove the instance check when we drop support for Python versions before that version. At that point the question of whether the package has been backported to those older versions is not relevant.

Given how packages work in the python ecosystem, is this version possible to determine?

[rizsotto.mailinglist]

Is there an easy way to detect at run time whether the SELinux setup forbids library preload?

I'm not aware any query command which does that. We can run a test and see does it produce the desired output or not. That would be very portable approach! (As I learned IBM AIX does have security feature that might prevent preload too. Fortunately enough this setup was not yet targeted AIX build support. :))

tools/scan-build-py/libscanbuild/__init__.py

50

Yes, I got the question to document it. But pydoc does not say a word about the return type of these methods. (Might be that some dependency changed, which cause this.) To give the answer what version do what, I need to test against all python versions. Or we should use a dedicated library which tries to hide these kind of miss alignment between Python versions. (But that would be a dependency out of standard packages.)

To my best knowledge, I did isolate this strange Python feature into a single function. And I wish to document the version things, but I don't know any source which provide such information. And see no guarantees that it will be up to date when deprecation happens.

The old versions returns str, which is a no op. The new version returns byte, which needs to be encoded. The underlying check_output call is deprecated in Python 3.5, but still present in 3.6 and 3.7a.

tools/scan-build-py/libscanbuild/__init__.py

 #
 # This file is distributed under the University of Illinois Open Source
 # License. See LICENSE.TXT for details.
-"""
-This module responsible to run the Clang static analyzer against any build
-and generate reports.
-"""
+""" This module is a collection of methods commonly used in this project. """
+import functools
+import logging
+import os
+import os.path
+import subprocess
+import sys
 
 
 def duplicate_check(method):

 def tempdir():
     """ Return the default temorary directory. """
 
-    from os import getenv
-    return getenv('TMPDIR', getenv('TEMP', getenv('TMP', '/tmp')))
+    return os.getenv('TMPDIR', os.getenv('TEMP', os.getenv('TMP', '/tmp')))
+
+
+def run_command(command, cwd=None):
+    """ Run a given command and report the execution.
+
+    :param command: array of tokens
+    :param cwd: the working directory where the command will be executed
+    :return: output of the command
+    """
+    def decode_when_needed(result):
+        """ check_output returns bytes or string depend on python version """
+        return result.decode('utf-8') if isinstance(result, bytes) else result
+
+    try:
+        directory = os.path.abspath(cwd) if cwd else os.getcwd()
+        logging.debug('exec command %s in %s', command, directory)
+        output = subprocess.check_output(command,
+                                         cwd=directory,
+                                         stderr=subprocess.STDOUT)
+        return decode_when_needed(output).splitlines()
+    except subprocess.CalledProcessError as ex:
+        ex.output = decode_when_needed(ex.output).splitlines()
+        raise ex
 
 
 def initialize_logging(verbose_level):
     """ Output content controlled by the verbosity level. """
 
-    import sys
-    import os.path
-    import logging
     level = logging.WARNING - min(logging.WARNING, (10 * verbose_level))
 
     if verbose_level <= 3:

 def command_entry_point(function):
     """ Decorator for command entry points. """
 
-    import functools
-    import logging
-
     @functools.wraps(function)
     def wrapper(*args, **kwargs):
 

tools/scan-build-py/libscanbuild/clang.py

 a subset of that, it makes sense to create a function specific wrapper. """
 
 import re
-import subprocess
-import logging
+from libscanbuild import run_command
 from libscanbuild.shell import decode
 
 __all__ = ['get_version', 'get_arguments', 'get_checkers']

     :param clang:   the compiler we are using
     :return:        the version string printed to stderr """
 
-    output = subprocess.check_output([clang, '-v'], stderr=subprocess.STDOUT)
-    return output.decode('utf-8').splitlines()[0]
+    output = run_command([clang, '-v'])
+    # the relevant version info is in the first line
+    return output[0]
 
 
 def get_arguments(command, cwd):

 
     cmd = command[:]
     cmd.insert(1, '-###')
-    logging.debug('exec command in %s: %s', cwd, ' '.join(cmd))
 
-    output = subprocess.check_output(cmd, cwd=cwd, stderr=subprocess.STDOUT)
+    output = run_command(cmd, cwd=cwd)
     # The relevant information is in the last line of the output.
     # Don't check if finding last line fails, would throw exception anyway.
-    last_line = output.decode('utf-8').splitlines()[-1]
+    last_line = output[-1]
     if re.search(r'clang(.*): error:', last_line):
         raise Exception(last_line)
     return decode(last_line)

     load = [elem for plugin in plugins for elem in ['-load', plugin]]
     cmd = [clang, '-cc1'] + load + ['-analyzer-checker-help']
 
-    logging.debug('exec command: %s', ' '.join(cmd))
-    output = subprocess.check_output(cmd, stderr=subprocess.STDOUT)
-    lines = output.decode('utf-8').splitlines()
+    lines = run_command(cmd)
 
     is_active_checker = is_active(get_active_checkers(clang, plugins))
 

tools/scan-build-py/libscanbuild/intercept.py

 import logging
 import subprocess
 from libear import build_libear, TemporaryDirectory
-from libscanbuild import command_entry_point
+from libscanbuild import command_entry_point, run_command
 from libscanbuild import duplicate_check, tempdir, initialize_logging
 from libscanbuild.compilation import split_command
 from libscanbuild.shell import encode, decode

 
 COMPILER_WRAPPER_CC = 'intercept-cc'
 COMPILER_WRAPPER_CXX = 'intercept-c++'
+WRAPPER_ONLY_PLATFORMS = frozenset({'win32', 'cygwin'})
 
 
 @command_entry_point

     the path and, if so, (2) whether the output of executing 'csrutil status'
     contains 'System Integrity Protection status: enabled'.
 
-    Same problem on linux when SELinux is enabled. The status query program
-    'sestatus' and the output when it's enabled 'SELinux status: enabled'. """
+    :param platform: name of the platform (returned by sys.platform),
+    :return: True if library preload will fail by the dynamic linker. """
 
-    if platform == 'darwin':
-        pattern = re.compile(r'System Integrity Protection status:\s+enabled')
+    if platform in WRAPPER_ONLY_PLATFORMS:
+        return True
+    elif platform == 'darwin':
         command = ['csrutil', 'status']
-    elif platform in {'linux', 'linux2'}:
-        pattern = re.compile(r'SELinux status:\s+enabled')
-        command = ['sestatus']
+        pattern = re.compile(r'System Integrity Protection status:\s+enabled')
+        try:
+            return any(pattern.match(line) for line in run_command(command))
+        except:
+            return False
     else:
         return False
 
-    try:
-        lines = subprocess.check_output(command).decode('utf-8')
-        return any((pattern.match(line) for line in lines.splitlines()))
-    except:
-        return False
-
 
 def entry_hash(entry):
     """ Implement unique hash method for compilation database entries. """

tools/scan-build-py/libscanbuild/runner.py

 import functools
 import subprocess
 import logging
+from libscanbuild import run_command
 from libscanbuild.compilation import classify_source, compiler_language
 from libscanbuild.clang import get_version, get_arguments
 from libscanbuild.shell import decode

 
 
 @require(['clang', 'directory', 'flags', 'file', 'output_dir', 'language',
-          'error_type', 'error_output', 'exit_code'])
+          'error_output', 'exit_code'])
 def report_failure(opts):
     """ Create report when analyzer failed.
 

     randomly. The compiler output also captured into '.stderr.txt' file.
     And some more execution context also saved into '.info.txt' file. """
 
-    def extension(opts):
+    def extension():
         """ Generate preprocessor file extension. """
 
         mapping = {'objective-c++': '.mii', 'objective-c': '.mi', 'c++': '.ii'}
         return mapping.get(opts['language'], '.i')
 
-    def destination(opts):
+    def destination():
         """ Creates failures directory if not exits yet. """
 
-        name = os.path.join(opts['output_dir'], 'failures')
-        if not os.path.isdir(name):
-            os.makedirs(name)
-        return name
-
-    error = opts['error_type']
-    (handle, name) = tempfile.mkstemp(suffix=extension(opts),
+        failures_dir = os.path.join(opts['output_dir'], 'failures')
+        if not os.path.isdir(failures_dir):
+            os.makedirs(failures_dir)
+        return failures_dir
+
+    # Classify error type: when Clang terminated by a signal it's a 'Crash'.
+    # (python subprocess Popen.returncode is negative when child terminated
+    # by signal.) Everything else is 'Other Error'.
+    error = 'crash' if opts['exit_code'] < 0 else 'other_error'
+    # Create preprocessor output file name. (This is blindly following the
+    # Perl implementation.)
+    (handle, name) = tempfile.mkstemp(suffix=extension(),
                                       prefix='clang_' + error + '_',
-                                      dir=destination(opts))
+                                      dir=destination())
     os.close(handle)
+    # Execute Clang again, but run the syntax check only.
     cwd = opts['directory']
-    cmd = get_arguments([opts['clang'], '-fsyntax-only', '-E'] +
-                        opts['flags'] + [opts['file'], '-o', name], cwd)
-    logging.debug('exec command in %s: %s', cwd, ' '.join(cmd))
-    subprocess.call(cmd, cwd=cwd)
+    cmd = get_arguments(
+        [opts['clang'], '-fsyntax-only', '-E'
+         ] + opts['flags'] + [opts['file'], '-o', name], cwd)
+    run_command(cmd, cwd=cwd)
     # write general information about the crash
     with open(name + '.info.txt', 'w') as handle:
         handle.write(opts['file'] + os.linesep)

     with open(name + '.stderr.txt', 'w') as handle:
         handle.writelines(opts['error_output'])
         handle.close()
-    # return with the previous step exit code and output
-    return {
-        'error_output': opts['error_output'],
-        'exit_code': opts['exit_code']
-    }
 
 
 @require(['clang', 'directory', 'flags', 'direct_args', 'file', 'output_dir',

     output of the analysis and returns with it. If failure reports are
     requested, it calls the continuation to generate it. """
 
-    def output():
+    def target():
         """ Creates output file name for reports. """
         if opts['output_format'] in {'plist', 'plist-html'}:
             (handle, name) = tempfile.mkstemp(prefix='report-',

             return name
         return opts['output_dir']
 
-    cwd = opts['directory']
-    cmd = get_arguments([opts['clang'], '--analyze'] + opts['direct_args'] +
-                        opts['flags'] + [opts['file'], '-o', output()],
-                        cwd)
-    logging.debug('exec command in %s: %s', cwd, ' '.join(cmd))
-    child = subprocess.Popen(cmd,
-                             cwd=cwd,
-                             universal_newlines=True,
-                             stdout=subprocess.PIPE,
-                             stderr=subprocess.STDOUT)
-    output = child.stdout.readlines()
-    child.stdout.close()
-    # do report details if it were asked
-    child.wait()
-    if opts.get('output_failures', False) and child.returncode:
-        error_type = 'crash' if child.returncode & 127 else 'other_error'
-        opts.update({
-            'error_type': error_type,
-            'error_output': output,
-            'exit_code': child.returncode
-        })
-        return continuation(opts)
-    # return the output for logging and exit code for testing
-    return {'error_output': output, 'exit_code': child.returncode}
+    try:
+        cwd = opts['directory']
+        cmd = get_arguments([opts['clang'], '--analyze'] +
+                            opts['direct_args'] + opts['flags'] +
+                            [opts['file'], '-o', target()],
+                            cwd)
+        output = run_command(cmd, cwd=cwd)
+        return {'error_output': output, 'exit_code': 0}
+    except subprocess.CalledProcessError as ex:
+        result = {'error_output': ex.output, 'exit_code': ex.returncode}
+        if opts.get('output_failures', False):
+            opts.update(result)
+            continuation(opts)
+        return result
 
 
 @require(['flags', 'force_debug'])

tools/scan-build-py/tests/unit/test_intercept.py

         DISABLED = 'disabled'
 
         OSX = 'darwin'
-        LINUX = 'linux'
 
         with libear.TemporaryDirectory() as tmpdir:
+            saved = os.environ['PATH']
             try:
-                saved = os.environ['PATH']
                 os.environ['PATH'] = tmpdir + ':' + saved
 
                 create_csrutil(tmpdir, ENABLED)

 
                 create_csrutil(tmpdir, DISABLED)
                 self.assertFalse(sut.is_preload_disabled(OSX))
-
-                create_sestatus(tmpdir, ENABLED)
-                self.assertTrue(sut.is_preload_disabled(LINUX))
-
-                create_sestatus(tmpdir, DISABLED)
-                self.assertFalse(sut.is_preload_disabled(LINUX))
             finally:
                 os.environ['PATH'] = saved
 
+        saved = os.environ['PATH']
         try:
-            saved = os.environ['PATH']
             os.environ['PATH'] = ''
             # shall be false when it's not in the path
             self.assertFalse(sut.is_preload_disabled(OSX))
-            self.assertFalse(sut.is_preload_disabled(LINUX))
 
             self.assertFalse(sut.is_preload_disabled('unix'))
         finally:

tools/scan-build-py/tests/unit/test_runner.py

     def test_run_analyzer_crash_and_forwarded(self):
         content = "int div(int n, int d) { return n / d }"
         (_, fwds) = RunAnalyzerTest.run_analyzer(content, True)
-        self.assertEqual('crash', fwds['error_type'])
         self.assertEqual(1, fwds['exit_code'])
         self.assertTrue(len(fwds['error_output']) > 0)