From 62964a694ce67c5425344e2b6473724808a93f74 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Fri, 29 Aug 2014 15:42:59 -0700 Subject: sandbox: style cleanup Based on readability review by Dean Berris at Google. R=jln@chromium.org Review URL: https://codereview.chromium.org/511993005 Cr-Commit-Position: refs/heads/master@{#292698} --- sandbox/linux/bpf_dsl/bpf_dsl.cc | 101 +++++++++++++++++++----------- sandbox/linux/bpf_dsl/bpf_dsl.h | 43 ++++++++----- sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc | 4 +- 3 files changed, 96 insertions(+), 52 deletions(-) (limited to 'sandbox') diff --git a/sandbox/linux/bpf_dsl/bpf_dsl.cc b/sandbox/linux/bpf_dsl/bpf_dsl.cc index e9057ae..ac16051 100644 --- a/sandbox/linux/bpf_dsl/bpf_dsl.cc +++ b/sandbox/linux/bpf_dsl/bpf_dsl.cc @@ -6,67 +6,75 @@ #include +#include + #include "base/logging.h" #include "base/memory/ref_counted.h" #include "sandbox/linux/seccomp-bpf/errorcode.h" #include "sandbox/linux/seccomp-bpf/sandbox_bpf.h" -using namespace sandbox::bpf_dsl::internal; -typedef ::sandbox::Trap::TrapFnc TrapFnc; - namespace sandbox { namespace bpf_dsl { namespace { -class AllowResultExprImpl : public ResultExprImpl { +class AllowResultExprImpl : public internal::ResultExprImpl { public: AllowResultExprImpl() {} + virtual ErrorCode Compile(SandboxBPF* sb) const OVERRIDE { return ErrorCode(ErrorCode::ERR_ALLOWED); } private: virtual ~AllowResultExprImpl() {} + DISALLOW_COPY_AND_ASSIGN(AllowResultExprImpl); }; -class ErrorResultExprImpl : public ResultExprImpl { +class ErrorResultExprImpl : public internal::ResultExprImpl { public: explicit ErrorResultExprImpl(int err) : err_(err) { CHECK(err_ >= ErrorCode::ERR_MIN_ERRNO && err_ <= ErrorCode::ERR_MAX_ERRNO); } + virtual ErrorCode Compile(SandboxBPF* sb) const OVERRIDE { return ErrorCode(err_); } private: virtual ~ErrorResultExprImpl() {} + int err_; + DISALLOW_COPY_AND_ASSIGN(ErrorResultExprImpl); }; -class TrapResultExprImpl : public ResultExprImpl { +class TrapResultExprImpl : public internal::ResultExprImpl { public: - TrapResultExprImpl(TrapFnc func, void* arg) : func_(func), arg_(arg) { + TrapResultExprImpl(Trap::TrapFnc func, void* arg) : func_(func), arg_(arg) { DCHECK(func_); } + virtual ErrorCode Compile(SandboxBPF* sb) const OVERRIDE { return sb->Trap(func_, arg_); } private: virtual ~TrapResultExprImpl() {} - TrapFnc func_; + + Trap::TrapFnc func_; void* arg_; + DISALLOW_COPY_AND_ASSIGN(TrapResultExprImpl); }; -class IfThenResultExprImpl : public ResultExprImpl { +class IfThenResultExprImpl : public internal::ResultExprImpl { public: - IfThenResultExprImpl(BoolExpr cond, - ResultExpr then_result, - ResultExpr else_result) + IfThenResultExprImpl(const BoolExpr& cond, + const ResultExpr& then_result, + const ResultExpr& else_result) : cond_(cond), then_result_(then_result), else_result_(else_result) {} + virtual ErrorCode Compile(SandboxBPF* sb) const OVERRIDE { return cond_->Compile( sb, then_result_->Compile(sb), else_result_->Compile(sb)); @@ -74,19 +82,22 @@ class IfThenResultExprImpl : public ResultExprImpl { private: virtual ~IfThenResultExprImpl() {} + BoolExpr cond_; ResultExpr then_result_; ResultExpr else_result_; + DISALLOW_COPY_AND_ASSIGN(IfThenResultExprImpl); }; -class PrimitiveBoolExprImpl : public BoolExprImpl { +class PrimitiveBoolExprImpl : public internal::BoolExprImpl { public: PrimitiveBoolExprImpl(int argno, ErrorCode::ArgType is_32bit, ErrorCode::Operation op, uint64_t value) : argno_(argno), is_32bit_(is_32bit), op_(op), value_(value) {} + virtual ErrorCode Compile(SandboxBPF* sb, ErrorCode true_ec, ErrorCode false_ec) const OVERRIDE { @@ -95,16 +106,19 @@ class PrimitiveBoolExprImpl : public BoolExprImpl { private: virtual ~PrimitiveBoolExprImpl() {} + int argno_; ErrorCode::ArgType is_32bit_; ErrorCode::Operation op_; uint64_t value_; + DISALLOW_COPY_AND_ASSIGN(PrimitiveBoolExprImpl); }; -class NegateBoolExprImpl : public BoolExprImpl { +class NegateBoolExprImpl : public internal::BoolExprImpl { public: - explicit NegateBoolExprImpl(BoolExpr cond) : cond_(cond) {} + explicit NegateBoolExprImpl(const BoolExpr& cond) : cond_(cond) {} + virtual ErrorCode Compile(SandboxBPF* sb, ErrorCode true_ec, ErrorCode false_ec) const OVERRIDE { @@ -113,13 +127,17 @@ class NegateBoolExprImpl : public BoolExprImpl { private: virtual ~NegateBoolExprImpl() {} + BoolExpr cond_; + DISALLOW_COPY_AND_ASSIGN(NegateBoolExprImpl); }; -class AndBoolExprImpl : public BoolExprImpl { +class AndBoolExprImpl : public internal::BoolExprImpl { public: - AndBoolExprImpl(BoolExpr lhs, BoolExpr rhs) : lhs_(lhs), rhs_(rhs) {} + AndBoolExprImpl(const BoolExpr& lhs, const BoolExpr& rhs) + : lhs_(lhs), rhs_(rhs) {} + virtual ErrorCode Compile(SandboxBPF* sb, ErrorCode true_ec, ErrorCode false_ec) const OVERRIDE { @@ -128,13 +146,18 @@ class AndBoolExprImpl : public BoolExprImpl { private: virtual ~AndBoolExprImpl() {} - BoolExpr lhs_, rhs_; + + BoolExpr lhs_; + BoolExpr rhs_; + DISALLOW_COPY_AND_ASSIGN(AndBoolExprImpl); }; -class OrBoolExprImpl : public BoolExprImpl { +class OrBoolExprImpl : public internal::BoolExprImpl { public: - OrBoolExprImpl(BoolExpr lhs, BoolExpr rhs) : lhs_(lhs), rhs_(rhs) {} + OrBoolExprImpl(const BoolExpr& lhs, const BoolExpr& rhs) + : lhs_(lhs), rhs_(rhs) {} + virtual ErrorCode Compile(SandboxBPF* sb, ErrorCode true_ec, ErrorCode false_ec) const OVERRIDE { @@ -143,7 +166,10 @@ class OrBoolExprImpl : public BoolExprImpl { private: virtual ~OrBoolExprImpl() {} - BoolExpr lhs_, rhs_; + + BoolExpr lhs_; + BoolExpr rhs_; + DISALLOW_COPY_AND_ASSIGN(OrBoolExprImpl); }; @@ -161,23 +187,27 @@ BoolExpr ArgEq(int num, size_t size, uint64_t mask, uint64_t val) { const ErrorCode::ArgType arg_type = (size <= 4) ? ErrorCode::TP_32BIT : ErrorCode::TP_64BIT; - if (mask == static_cast(-1)) { + if (mask == std::numeric_limits::max()) { // Arg == Val return BoolExpr(new const PrimitiveBoolExprImpl( num, arg_type, ErrorCode::OP_EQUAL, val)); - } else if (mask == val) { + } + + if (mask == val) { // (Arg & Mask) == Mask return BoolExpr(new const PrimitiveBoolExprImpl( num, arg_type, ErrorCode::OP_HAS_ALL_BITS, mask)); - } else if (val == 0) { + } + + if (val == 0) { // (Arg & Mask) == 0, which is semantically equivalent to !((arg & mask) != // 0). return !BoolExpr(new const PrimitiveBoolExprImpl( num, arg_type, ErrorCode::OP_HAS_ANY_BITS, mask)); - } else { - CHECK(false) << "Unimplemented ArgEq case"; - return BoolExpr(); } + + CHECK(false) << "Unimplemented ArgEq case"; + return BoolExpr(); } } // namespace internal @@ -190,23 +220,23 @@ ResultExpr Error(int err) { return ResultExpr(new const ErrorResultExprImpl(err)); } -ResultExpr Trap(TrapFnc trap_func, void* aux) { +ResultExpr Trap(Trap::TrapFnc trap_func, void* aux) { return ResultExpr(new const TrapResultExprImpl(trap_func, aux)); } -BoolExpr operator!(BoolExpr cond) { +BoolExpr operator!(const BoolExpr& cond) { return BoolExpr(new const NegateBoolExprImpl(cond)); } -BoolExpr operator&&(BoolExpr lhs, BoolExpr rhs) { +BoolExpr operator&&(const BoolExpr& lhs, const BoolExpr& rhs) { return BoolExpr(new const AndBoolExprImpl(lhs, rhs)); } -BoolExpr operator||(BoolExpr lhs, BoolExpr rhs) { +BoolExpr operator||(const BoolExpr& lhs, const BoolExpr& rhs) { return BoolExpr(new const OrBoolExprImpl(lhs, rhs)); } -Elser If(BoolExpr cond, ResultExpr then_result) { +Elser If(const BoolExpr& cond, const ResultExpr& then_result) { return Elser(Cons::List()).ElseIf(cond, then_result); } @@ -219,12 +249,12 @@ Elser::Elser(const Elser& elser) : clause_list_(elser.clause_list_) { Elser::~Elser() { } -Elser Elser::ElseIf(BoolExpr cond, ResultExpr then_result) const { +Elser Elser::ElseIf(const BoolExpr& cond, const ResultExpr& then_result) const { return Elser( Cons::Make(std::make_pair(cond, then_result), clause_list_)); } -ResultExpr Elser::Else(ResultExpr else_result) const { +ResultExpr Elser::Else(const ResultExpr& else_result) const { // We finally have the default result expression for this // if/then/else sequence. Also, we've already accumulated all // if/then pairs into a list of reverse order (i.e., lower priority @@ -269,8 +299,7 @@ ErrorCode SandboxBPFDSLPolicy::InvalidSyscall(SandboxBPF* sb) const { return InvalidSyscall()->Compile(sb); } -ResultExpr SandboxBPFDSLPolicy::Trap(::sandbox::Trap::TrapFnc trap_func, - void* aux) { +ResultExpr SandboxBPFDSLPolicy::Trap(Trap::TrapFnc trap_func, void* aux) { return bpf_dsl::Trap(trap_func, aux); } diff --git a/sandbox/linux/bpf_dsl/bpf_dsl.h b/sandbox/linux/bpf_dsl/bpf_dsl.h index d46102a..da90efd 100644 --- a/sandbox/linux/bpf_dsl/bpf_dsl.h +++ b/sandbox/linux/bpf_dsl/bpf_dsl.h @@ -7,6 +7,7 @@ #include +#include #include #include "base/macros.h" @@ -108,7 +109,7 @@ class SANDBOX_EXPORT SandboxBPFDSLPolicy : public SandboxBPFPolicy { virtual ErrorCode InvalidSyscall(SandboxBPF* sb) const OVERRIDE FINAL; // Helper method so policies can just write Trap(func, aux). - static ResultExpr Trap(::sandbox::Trap::TrapFnc trap_func, void* aux); + static ResultExpr Trap(Trap::TrapFnc trap_func, void* aux); private: DISALLOW_COPY_AND_ASSIGN(SandboxBPFDSLPolicy); @@ -127,41 +128,48 @@ SANDBOX_EXPORT ResultExpr Error(int err); // Trap specifies a result that the system call should be handled by // trapping back into userspace and invoking |trap_func|, passing // |aux| as the second parameter. -SANDBOX_EXPORT ResultExpr Trap(::sandbox::Trap::TrapFnc trap_func, void* aux); +SANDBOX_EXPORT ResultExpr Trap(Trap::TrapFnc trap_func, void* aux); template class SANDBOX_EXPORT Arg { public: // Initializes the Arg to represent the |num|th system call // argument (indexed from 0), which is of type |T|. - explicit Arg(int num) : num_(num), mask_(-1) {} + explicit Arg(int num) + : num_(num), mask_(std::numeric_limits::max()) {} Arg(const Arg& arg) : num_(arg.num_), mask_(arg.mask_) {} // Returns an Arg representing the current argument, but after // bitwise-and'ing it with |rhs|. - Arg operator&(uint64_t rhs) const { return Arg(num_, mask_ & rhs); } + friend Arg operator&(const Arg& lhs, uint64_t rhs) { + return Arg(lhs.num_, lhs.mask_ & rhs); + } // Returns a boolean expression comparing whether the system call // argument (after applying any bitmasks, if appropriate) equals |rhs|. - BoolExpr operator==(T rhs) const; + friend BoolExpr operator==(const Arg& lhs, T rhs) { return lhs.EqualTo(rhs); } private: Arg(int num, uint64_t mask) : num_(num), mask_(mask) {} + + BoolExpr EqualTo(T val) const; + int num_; uint64_t mask_; + DISALLOW_ASSIGN(Arg); }; // Various ways to combine boolean expressions into more complex expressions. // They follow standard boolean algebra laws. -SANDBOX_EXPORT BoolExpr operator!(BoolExpr cond); -SANDBOX_EXPORT BoolExpr operator&&(BoolExpr lhs, BoolExpr rhs); -SANDBOX_EXPORT BoolExpr operator||(BoolExpr lhs, BoolExpr rhs); +SANDBOX_EXPORT BoolExpr operator!(const BoolExpr& cond); +SANDBOX_EXPORT BoolExpr operator&&(const BoolExpr& lhs, const BoolExpr& rhs); +SANDBOX_EXPORT BoolExpr operator||(const BoolExpr& lhs, const BoolExpr& rhs); // If begins a conditional result expression predicated on the // specified boolean expression. -SANDBOX_EXPORT Elser If(BoolExpr cond, ResultExpr then_result); +SANDBOX_EXPORT Elser If(const BoolExpr& cond, const ResultExpr& then_result); class SANDBOX_EXPORT Elser { public: @@ -170,17 +178,20 @@ class SANDBOX_EXPORT Elser { // ElseIf extends the conditional result expression with another // "if then" clause, predicated on the specified boolean expression. - Elser ElseIf(BoolExpr cond, ResultExpr then_result) const; + Elser ElseIf(const BoolExpr& cond, const ResultExpr& then_result) const; // Else terminates a conditional result expression using |else_result| as // the default fallback result expression. - ResultExpr Else(ResultExpr else_result) const; + ResultExpr Else(const ResultExpr& else_result) const; private: typedef std::pair Clause; + explicit Elser(Cons::List clause_list); + Cons::List clause_list_; - friend Elser If(BoolExpr, ResultExpr); + + friend Elser If(const BoolExpr&, const ResultExpr&); DISALLOW_ASSIGN(Elser); }; @@ -235,9 +246,13 @@ class SANDBOX_EXPORT ResultExprImpl : public base::RefCounted { // Definition requires ArgEq to have been declared. Moved out-of-line // to minimize how much internal clutter users have to ignore while // reading the header documentation. +// +// Additionally, we use this helper member function to avoid linker errors +// caused by defining operator== out-of-line. For a more detailed explanation, +// see http://www.parashift.com/c++-faq-lite/template-friends.html. template -BoolExpr Arg::operator==(T rhs) const { - return internal::ArgEq(num_, sizeof(T), mask_, static_cast(rhs)); +BoolExpr Arg::EqualTo(T val) const { + return internal::ArgEq(num_, sizeof(T), mask_, static_cast(val)); } } // namespace bpf_dsl diff --git a/sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc b/sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc index 560d94c..d975d64 100644 --- a/sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc +++ b/sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc @@ -16,8 +16,6 @@ #include "sandbox/linux/seccomp-bpf/sandbox_bpf_policy.h" #include "sandbox/linux/seccomp-bpf/syscall.h" -using namespace sandbox::bpf_dsl; - // Helper macro to assert that invoking system call |sys| directly via // Syscall::Call with arguments |...| returns |res|. // Errors can be asserted by specifying a value like "-EINVAL". @@ -25,6 +23,7 @@ using namespace sandbox::bpf_dsl; BPF_ASSERT_EQ(res, Stubs::sys(__VA_ARGS__)) namespace sandbox { +namespace bpf_dsl { namespace { // Type safe stubs for tested system calls. @@ -265,4 +264,5 @@ BPF_TEST_C(BPFDSL, ElseIfTest, ElseIfPolicy) { } } // namespace +} // namespace bpf_dsl } // namespace sandbox -- cgit v1.1