Add PRESUBMIT rule to make sure IPC changes are security reviewed.

PRESUBMIT.py

 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Top-level presubmit script for Chromium.
 
 See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts
 for more details about the presubmit API built into depot_tools.
 """
 
@@ skipping 410 matching lines @@
 def _CheckDCHECK_IS_ONHasBraces(input_api, output_api):
   """Checks to make sure DCHECK_IS_ON() does not skip the braces."""
   errors = []
   pattern = input_api.re.compile(r'DCHECK_IS_ON(?!\(\))',
                                  input_api.re.MULTILINE)
   for f in input_api.AffectedSourceFiles(input_api.FilterSourceFile):
     if (not f.LocalPath().endswith(('.cc', '.mm', '.h'))):
       continue
     for lnum, line in f.ChangedContents():
       if input_api.re.search(pattern, line):
-          errors.append(output_api.PresubmitError(
-            ('%s:%d: Use of DCHECK_IS_ON() must be written as "#if ' +
-             'DCHECK_IS_ON()", not forgetting the braces.')
-            % (f.LocalPath(), lnum)))
+        errors.append(output_api.PresubmitError(
+          ('%s:%d: Use of DCHECK_IS_ON() must be written as "#if ' +
+           'DCHECK_IS_ON()", not forgetting the braces.')
+          % (f.LocalPath(), lnum)))
   return errors
 
 
 def _FindHistogramNameInLine(histogram_name, line):
   """Tries to find a histogram name or prefix in a line."""
   if not "affected-histogram" in line:
     return histogram_name in line
   # A histogram_suffixes tag type has an affected-histogram name as a prefix of
   # the histogram_name.
   if not '"' in line:
@@ skipping 138 matching lines @@
         return True
     return False
 
   def CheckForMatch(affected_file, line_num, line, func_name, message, error):
     matched = False
     if func_name[0:1] == '/':
       regex = func_name[1:]
       if input_api.re.search(regex, line):
         matched = True
     elif func_name in line:
-        matched = True
+      matched = True
     if matched:
-      problems = warnings;
+      problems = warnings
       if error:
-        problems = errors;
+        problems = errors
       problems.append('    %s:%d:' % (affected_file.LocalPath(), line_num))
       for message_line in message:
         problems.append('      %s' % message_line)
 
   file_filter = lambda f: f.LocalPath().endswith(('.mm', '.m', '.h'))
   for f in input_api.AffectedFiles(file_filter=file_filter):
     for line_num, line in f.ChangedContents():
       for func_name, message, error in _BANNED_OBJC_FUNCTIONS:
         CheckForMatch(f, line_num, line, func_name, message, error)
 
@@ skipping 784 matching lines @@
     import checkstyle
   finally:
     # Restore sys.path to what it was before.
     sys.path = original_sys_path
 
   return checkstyle.RunCheckstyle(
       input_api, output_api, 'tools/android/checkstyle/chromium-style-5.0.xml',
       black_list=_EXCLUDED_PATHS + input_api.DEFAULT_BLACK_LIST)
 
 
+def _CheckIpcOwners(input_api, output_api):
+  """Checks that affected files involving IPC have an IPC OWNERS rule.
+
+  Whether or not a file affects IPC is determined by a simple whitelist of
+  filename patterns."""
+  file_patterns = [
+      '*_messages.cc',
+      '*_messages*.h',
+      '*_param_traits*.*',
+      '*.mojom',
+      '*_struct_traits*.*',
+      '*_type_converters*.*',
+  ]
+
+  # Dictionary mapping an OWNERS file path to Patterns.
+  # Patterns is a dictionary mapping glob patterns (suitable for use in per-file
+  # rules ) to a PatternEntry.
+  # PatternEntry is a dictionary with two keys:
+  # - 'files': the files that are matched by this pattern
+  # - 'rules': the per-file rules needed for this pattern
+  # For example, if we expect OWNERS file to contain rules for *.mojom and
+  # *_struct_traits*.*, Patterns might look like this:
+  # {
+  #   '*.mojom': {
+  #     'files': ...,
+  #     'rules': [
+  #       'per-file *.mojom=set noparent',
+  #       'per-file *.mojom=file://ipc/SECURITY_OWNERS',
+  #     ],
+  #   },
+  #   '*_struct_traits*.*': {
+  #     'files': ...,
+  #     'rules': [
+  #       'per-file *_struct_traits*.*=set noparent',
+  #       'per-file *_struct_traits*.*=file://ipc/SECURITY_OWNERS',
+  #     ],
+  #   },
+  # }
+  to_check = {}
+
+  # Iterate through the affected files to see what we actually need to check
+  # for. We should only nag patch authors about per-file rules if a file in that
+  # directory would match that pattern. If a directory only contains *.mojom
+  # files and no *_messages*.h files, we should only nag about rules for
+  # *.mojom files.
+  for f in input_api.change.AffectedFiles():
+    for pattern in file_patterns:
+      if input_api.fnmatch.fnmatch(
+          input_api.os_path.basename(f.LocalPath()), pattern):
+        owners_file = input_api.os_path.join(
+            input_api.os_path.dirname(f.LocalPath()), 'OWNERS')
+        if owners_file not in to_check:
+          to_check[owners_file] = {}
+        if pattern not in to_check[owners_file]:
+          to_check[owners_file][pattern] = {
+              'files': [],
+              'rules': [
+                  'per-file %s=set noparent' % pattern,
+                  'per-file %s=file://ipc/SECURITY_OWNERS' % pattern,
+              ]
+          }
+        to_check[owners_file][pattern]['files'].append(f)
+        break
+
+  # Now go through the OWNERS files we collected, filtering out rules that are
+  # already present in that OWNERS file.
+  for owners_file, patterns in to_check.iteritems():
+    try:
+      with file(owners_file) as f:
+        for line in f.read().splitlines():
+          for entry in patterns.itervalues():
+            entry['rules'] = [rule for rule in entry['rules'] if rule != line]

[Dirk Pranke, 2016/06/18 20:49:26]

Maybe

  lines = f.read().splitlines()
  for entry in patterns.itervalues():
    entry['rules'] = [rule for rule in entry['rules'] if rule not in lines]

is clearer and possibly also more efficient?

[dcheng, 2016/06/19 00:03:28]

I tweaked this slightly and put the lines in a set. So now it's more efficient
from a strict algorithmic complexity standpoint… though I'm not sure it will
actually be more efficient in practice =P

+    except IOError:
+      # No OWNERS file, so all the rules are definitely missing.
+      continue
+
+  # All the remaining lines weren't found in OWNERS files, so emit an error.
+  errors = []
+  for owners_file, patterns in to_check.iteritems():
+    missing_lines = []
+    files = []
+    for pattern, entry in patterns.iteritems():
+      missing_lines.extend(entry['rules'])
+      files.extend(['  %s' % f.LocalPath() for f in entry['files']])
+    if missing_lines:
+      errors.append(
+          '%s is missing the following lines:\n\n%s\n\nfor changed files:\n%s' %
+          (owners_file, '\n'.join(missing_lines), '\n'.join(files)))
+
+  results = []
+  if errors:
+    results.append(output_api.PresubmitError(
+        'Found changes to IPC files without a security OWNER!',
+        long_text='\n\n'.join(errors)))
+
+  return results
+
+
 def _CheckAndroidToastUsage(input_api, output_api):
   """Checks that code uses org.chromium.ui.widget.Toast instead of
      android.widget.Toast (Chromium Toast doesn't force hardware
      acceleration on low-end devices, saving memory).
   """
   toast_import_pattern = input_api.re.compile(
       r'^import android\.widget\.Toast;$')
 
   errors = []
 
@@ skipping 443 matching lines @@
   results.extend(_CheckNoDeprecatedCSS(input_api, output_api))
   results.extend(_CheckNoDeprecatedJS(input_api, output_api))
   results.extend(_CheckParseErrors(input_api, output_api))
   results.extend(_CheckForIPCRules(input_api, output_api))
   results.extend(_CheckForCopyrightedCode(input_api, output_api))
   results.extend(_CheckForWindowsLineEndings(input_api, output_api))
   results.extend(_CheckSingletonInHeaders(input_api, output_api))
   results.extend(_CheckNoDeprecatedCompiledResourcesGYP(input_api, output_api))
   results.extend(_CheckPydepsNeedsUpdating(input_api, output_api))
   results.extend(_CheckJavaStyle(input_api, output_api))
+  results.extend(_CheckIpcOwners(input_api, output_api))
 
   if any('PRESUBMIT.py' == f.LocalPath() for f in input_api.AffectedFiles()):
     results.extend(input_api.canned_checks.RunUnitTestsInDirectory(
         input_api, output_api,
         input_api.PresubmitLocalPath(),
         whitelist=[r'^PRESUBMIT_test\.py$']))
   return results
 
 
 def _CheckAuthorizedAuthor(input_api, output_api):
   """For non-googler/chromites committers, verify the author's email address is
   in AUTHORS.
   """
-  # TODO(maruel): Add it to input_api?
-  import fnmatch
-
   author = input_api.change.author_email
   if not author:
     input_api.logging.info('No author, skipping AUTHOR check')
     return []
   authors_path = input_api.os_path.join(
       input_api.PresubmitLocalPath(), 'AUTHORS')
   valid_authors = (
       input_api.re.match(r'[^#]+\s+\<(.+?)\>\s*$', line)
       for line in open(authors_path))
   valid_authors = [item.group(1).lower() for item in valid_authors if item]
-  if not any(fnmatch.fnmatch(author.lower(), valid) for valid in valid_authors):
+  if not any(input_api.fnmatch.fnmatch(author.lower(), valid)
+             for valid in valid_authors):
     input_api.logging.info('Valid authors are %s', ', '.join(valid_authors))
     return [output_api.PresubmitPromptWarning(
         ('%s is not in AUTHORS file. If you are a new contributor, please visit'
         '\n'
         'http://www.chromium.org/developers/contributing-code and read the '
         '"Legal" section\n'
         'If you are a chromite, verify the contributor signed the CLA.') %
         author)]
   return []
 
@@ skipping 220 matching lines @@
   results.extend(input_api.canned_checks.CheckTreeIsOpen(
       input_api,
       output_api,
       json_url='http://chromium-status.appspot.com/current?format=json'))
 
   results.extend(input_api.canned_checks.CheckChangeHasBugField(
       input_api, output_api))
   results.extend(input_api.canned_checks.CheckChangeHasDescription(
       input_api, output_api))
   return results