SyscallIterator: support C++11 range-based for loops

Also remove the superfluous "to" field in PolicyCompiler::Range,
and fix the next-syscall-number calculation so we can remove the
loop in SyscallIterator::operator++() (nee SyscallIterator::Next()).

BUG=414363

Review URL: https://codereview.chromium.org/659723002

Cr-Commit-Position: refs/heads/master@{#300633}

sandbox/linux/bpf_dsl/policy_compiler.cc

@@ -76,9 +76,8 @@ intptr_t BPFFailure(const struct arch_seccomp_data&, void* aux) {
 }
 
 bool HasUnsafeTraps(const SandboxBPFDSLPolicy* policy) {
-  for (SyscallIterator iter(false); !iter.Done();) {
-    uint32_t sysnum = iter.Next();
-    if (SyscallIterator::IsValid(sysnum) &&
+  for (uint32_t sysnum : SyscallSet::All()) {
+    if (SyscallSet::IsValid(sysnum) &&
         policy->EvaluateSyscall(sysnum)->HasUnsafeTraps()) {
       return true;
     }
@@ -89,8 +88,8 @@ bool HasUnsafeTraps(const SandboxBPFDSLPolicy* policy) {
 }  // namespace
 
 struct PolicyCompiler::Range {
-  Range(uint32_t f, uint32_t t, const ErrorCode& e) : from(f), to(t), err(e) {}
-  uint32_t from, to;
+  Range(uint32_t f, const ErrorCode& e) : from(f), err(e) {}
+  uint32_t from;
   ErrorCode err;
 };
 
@@ -252,22 +251,22 @@ void PolicyCompiler::FindRanges(Ranges* ranges) {
   // negative) all return the same ErrorCode.
   const ErrorCode invalid_err = policy_->InvalidSyscall()->Compile(this);
   uint32_t old_sysnum = 0;
-  ErrorCode old_err = SyscallIterator::IsValid(old_sysnum)
+  ErrorCode old_err = SyscallSet::IsValid(old_sysnum)
                           ? policy_->EvaluateSyscall(old_sysnum)->Compile(this)
                           : invalid_err;
 
-  for (SyscallIterator iter(false); !iter.Done();) {
-    uint32_t sysnum = iter.Next();
+  for (uint32_t sysnum : SyscallSet::All()) {
     ErrorCode err =
-        SyscallIterator::IsValid(sysnum)
+        SyscallSet::IsValid(sysnum)
             ? policy_->EvaluateSyscall(static_cast<int>(sysnum))->Compile(this)
             : invalid_err;
-    if (!err.Equals(old_err) || iter.Done()) {
-      ranges->push_back(Range(old_sysnum, sysnum - 1, old_err));
+    if (!err.Equals(old_err)) {
+      ranges->push_back(Range(old_sysnum, old_err));
       old_sysnum = sysnum;
       old_err = err;
     }
   }
+  ranges->push_back(Range(old_sysnum, old_err));
 }
 
 Instruction* PolicyCompiler::AssembleJumpTable(Ranges::const_iterator start,

sandbox/linux/seccomp-bpf/sandbox_bpf.cc

@@ -150,7 +150,7 @@ SandboxBPF::~SandboxBPF() {
 }
 
 bool SandboxBPF::IsValidSyscallNumber(int sysnum) {
-  return SyscallIterator::IsValid(sysnum);
+  return SyscallSet::IsValid(sysnum);
 }
 
 bool SandboxBPF::RunFunctionInPolicy(

sandbox/linux/seccomp-bpf/syscall_iterator.cc

@@ -4,6 +4,7 @@
 
 #include "sandbox/linux/seccomp-bpf/syscall_iterator.h"
 
+#include "base/logging.h"
 #include "base/macros.h"
 #include "sandbox/linux/seccomp-bpf/linux_seccomp.h"
 
@@ -38,14 +39,16 @@ const SyscallRange kValidSyscallRanges[] = {
 #endif
 };
 
+// NextSyscall returns the next system call in the specified system
+// call set after |cur|, or 0 if no such system call exists.
 uint32_t NextSyscall(uint32_t cur, bool invalid_only) {
   for (const SyscallRange& range : kValidSyscallRanges) {
     if (range.first > 0 && cur < range.first - 1) {
       return range.first - 1;
     }
     if (cur <= range.last) {
-      if (invalid_only && cur < range.last) {
-        return range.last;
+      if (invalid_only) {
+        return range.last + 1;
       }
       return cur + 1;
     }
@@ -62,27 +65,22 @@ uint32_t NextSyscall(uint32_t cur, bool invalid_only) {
   if (cur < 0x80000000u)
     return 0x80000000u;
 
-  return 0xFFFFFFFFu;
+  if (cur < 0xFFFFFFFFu)
+    return 0xFFFFFFFFu;
+  return 0;
 }
 
 }  // namespace
 
-uint32_t SyscallIterator::Next() {
-  if (done_) {
-    return num_;
-  }
-
-  uint32_t val;
-  do {
-    val = num_;
-    num_ = NextSyscall(num_, invalid_only_);
-  } while (invalid_only_ && IsValid(val));
+SyscallSet::Iterator SyscallSet::begin() const {
+  return Iterator(set_, false);
+}
 
-  done_ |= val == 0xFFFFFFFFu;
-  return val;
+SyscallSet::Iterator SyscallSet::end() const {
+  return Iterator(set_, true);
 }
 
-bool SyscallIterator::IsValid(uint32_t num) {
+bool SyscallSet::IsValid(uint32_t num) {
   for (const SyscallRange& range : kValidSyscallRanges) {
     if (num >= range.first && num <= range.last) {
       return true;
@@ -91,4 +89,42 @@ bool SyscallIterator::IsValid(uint32_t num) {
   return false;
 }
 
+bool operator==(const SyscallSet& lhs, const SyscallSet& rhs) {
+  return (lhs.set_ == rhs.set_);
+}
+
+SyscallSet::Iterator::Iterator(Set set, bool done)
+    : set_(set), done_(done), num_(0) {
+  if (set_ == Set::INVALID_ONLY && !done_ && IsValid(num_)) {
+    ++*this;
+  }
+}
+
+uint32_t SyscallSet::Iterator::operator*() const {
+  DCHECK(!done_);
+  return num_;
+}
+
+SyscallSet::Iterator& SyscallSet::Iterator::operator++() {
+  DCHECK(!done_);
+
+  num_ = NextSyscall(num_, set_ == Set::INVALID_ONLY);
+  if (num_ == 0) {
+    done_ = true;
+  }
+
+  return *this;
+}
+
+bool operator==(const SyscallSet::Iterator& lhs,
+                const SyscallSet::Iterator& rhs) {
+  DCHECK(lhs.set_ == rhs.set_);
+  return (lhs.done_ == rhs.done_) && (lhs.num_ == rhs.num_);
+}
+
+bool operator!=(const SyscallSet::Iterator& lhs,
+                const SyscallSet::Iterator& rhs) {
+  return !(lhs == rhs);
+}
+
 }  // namespace sandbox

sandbox/linux/seccomp-bpf/syscall_iterator.h

@@ -12,6 +12,8 @@
 
 namespace sandbox {
 
+// TODO(mdempsky): Rename this header to syscall_set.h.
+
 // Iterates over the entire system call range from 0..0xFFFFFFFFu. This
 // iterator is aware of how system calls look like and will skip quickly
 // over ranges that can't contain system calls. It iterates more slowly
@@ -20,35 +22,75 @@ namespace sandbox {
 // first invalid value after a valid range of syscalls. It iterates over
 // individual values whenever it is in the normal range for system calls
 // (typically MIN_SYSCALL..MAX_SYSCALL).
-// If |invalid_only| is true, this iterator will only return invalid
-// syscall numbers, but will still skip quickly over invalid ranges,
-// returning the first invalid value in the range and then skipping
-// to the last invalid value in the range.
 //
 // Example usage:
-//   for (SyscallIterator iter(false); !iter.Done(); ) {
-//     uint32_t sysnum = iter.Next();
+//   for (uint32_t sysnum : SyscallSet::All()) {
 //     // Do something with sysnum.
 //   }
-//
-// TODO(markus): Make this a classic C++ iterator.
-class SANDBOX_EXPORT SyscallIterator {
+class SANDBOX_EXPORT SyscallSet {
  public:
-  explicit SyscallIterator(bool invalid_only)
-      : invalid_only_(invalid_only), done_(false), num_(0) {}
+  class Iterator;
+
+  SyscallSet(const SyscallSet& ss) : set_(ss.set_) {}
+  ~SyscallSet() {}
+
+  Iterator begin() const;
+  Iterator end() const;
+
+  // All returns a SyscallSet that contains both valid and invalid
+  // system call numbers.
+  static SyscallSet All() { return SyscallSet(Set::ALL); }
 
-  bool Done() const { return done_; }
-  uint32_t Next();
+  // InvalidOnly returns a SyscallSet that contains only invalid
+  // system call numbers, but still omits numbers in the middle of a
+  // range of invalid system call numbers.
+  static SyscallSet InvalidOnly() { return SyscallSet(Set::INVALID_ONLY); }
+
+  // IsValid returns whether |num| specifies a valid system call
+  // number.
   static bool IsValid(uint32_t num);
 
  private:
-  bool invalid_only_;
+  enum class Set { ALL, INVALID_ONLY };
+
+  explicit SyscallSet(Set set) : set_(set) {}
+
+  Set set_;
+
+  friend bool operator==(const SyscallSet&, const SyscallSet&);
+  DISALLOW_ASSIGN(SyscallSet);
+};
+
+SANDBOX_EXPORT bool operator==(const SyscallSet& lhs, const SyscallSet& rhs);
+
+// Iterator provides C++ input iterator semantics for traversing a
+// SyscallSet.
+class SyscallSet::Iterator {
+ public:
+  Iterator(const Iterator& it)
+      : set_(it.set_), done_(it.done_), num_(it.num_) {}
+  ~Iterator() {}
+
+  uint32_t operator*() const;
+  Iterator& operator++();
+
+ private:
+  Iterator(Set set, bool done);
+
+  Set set_;
   bool done_;
   uint32_t num_;
 
-  DISALLOW_IMPLICIT_CONSTRUCTORS(SyscallIterator);
+  friend SyscallSet;
+  friend bool operator==(const Iterator&, const Iterator&);
+  DISALLOW_ASSIGN(Iterator);
 };
 
+SANDBOX_EXPORT bool operator==(const SyscallSet::Iterator& lhs,
+                               const SyscallSet::Iterator& rhs);
+SANDBOX_EXPORT bool operator!=(const SyscallSet::Iterator& lhs,
+                               const SyscallSet::Iterator& rhs);
+
 }  // namespace sandbox
 
 #endif  // SANDBOX_LINUX_SECCOMP_BPF_SYSCALL_ITERATOR_H__

sandbox/linux/seccomp-bpf/syscall_iterator_unittest.cc

@@ -13,18 +13,19 @@ namespace sandbox {
 
 namespace {
 
-const bool kFalseTrue[] = {false, true};
+const SyscallSet kSyscallSets[] = {
+    SyscallSet::All(),
+    SyscallSet::InvalidOnly(),
+};
 
 SANDBOX_TEST(SyscallIterator, Monotonous) {
-  for (bool invalid_only : kFalseTrue) {
+  for (const SyscallSet& set : kSyscallSets) {
     uint32_t prev = 0;
     bool have_prev = false;
-    for (SyscallIterator iter(invalid_only); !iter.Done();) {
-      uint32_t sysnum = iter.Next();
-
+    for (uint32_t sysnum : set) {
       if (have_prev) {
         SANDBOX_ASSERT(sysnum > prev);
-      } else if (!invalid_only) {
+      } else if (set == SyscallSet::All()) {
         // The iterator should start at 0.
         SANDBOX_ASSERT(sysnum == 0);
       }
@@ -44,8 +45,7 @@ SANDBOX_TEST(SyscallIterator, Monotonous) {
 void AssertRange(uint32_t min, uint32_t max) {
   SANDBOX_ASSERT(min < max);
   uint32_t prev = min - 1;
-  for (SyscallIterator iter(false); !iter.Done();) {
-    uint32_t sysnum = iter.Next();
+  for (uint32_t sysnum : SyscallSet::All()) {
     if (sysnum >= min && sysnum <= max) {
       SANDBOX_ASSERT(prev == sysnum - 1);
       prev = sysnum;
@@ -80,11 +80,10 @@ SANDBOX_TEST(SyscallIterator, InvalidSyscalls) {
     0xFFFFFFFFu,
   };
 
-  for (bool invalid_only : kFalseTrue) {
+  for (const SyscallSet& set : kSyscallSets) {
     size_t i = 0;
-    for (SyscallIterator iter(invalid_only); !iter.Done();) {
-      uint32_t sysnum = iter.Next();
-      if (!SyscallIterator::IsValid(sysnum)) {
+    for (uint32_t sysnum : set) {
+      if (!SyscallSet::IsValid(sysnum)) {
         SANDBOX_ASSERT(i < arraysize(kExpected));
         SANDBOX_ASSERT(kExpected[i] == sysnum);
         ++i;
@@ -94,6 +93,12 @@ SANDBOX_TEST(SyscallIterator, InvalidSyscalls) {
   }
 }
 
+SANDBOX_TEST(SyscallIterator, InvalidOnlyIsOnlyInvalid) {
+  for (uint32_t sysnum : SyscallSet::InvalidOnly()) {
+    SANDBOX_ASSERT(!SyscallSet::IsValid(sysnum));
+  }
+}
+
 }  // namespace
 
 }  // namespace sandbox

sandbox/linux/seccomp-bpf/verifier.cc

@@ -318,8 +318,7 @@ bool Verifier::VerifyBPF(bpf_dsl::PolicyCompiler* compiler,
                          const bpf_dsl::SandboxBPFDSLPolicy& policy,
                          const char** err) {
   *err = NULL;
-  for (SyscallIterator iter(false); !iter.Done();) {
-    uint32_t sysnum = iter.Next();
+  for (uint32_t sysnum : SyscallSet::All()) {
     // We ideally want to iterate over the full system call range and values
     // just above and just below this range. This gives us the full result set
     // of the "evaluators".
@@ -340,7 +339,7 @@ bool Verifier::VerifyBPF(bpf_dsl::PolicyCompiler* compiler,
     }
 #endif
 #endif
-    ErrorCode code = iter.IsValid(sysnum)
+    ErrorCode code = SyscallSet::IsValid(sysnum)
                          ? policy.EvaluateSyscall(sysnum)->Compile(compiler)
                          : policy.InvalidSyscall()->Compile(compiler);
     if (!VerifyErrorCode(compiler, program, &data, code, code, err)) {