Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add bitwise operators after bits.. #2160

Merged
merged 1 commit into from
Mar 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 69 additions & 2 deletions ast/builtins.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,14 @@ var DefaultBuiltins = [...]*Builtin{
Abs,
Rem,

// Bitwise Arithmetic
BitsOr,
BitsAnd,
BitsNegate,
BitsXOr,
BitsShiftLeft,
BitsShiftRight,

// Binary
And,
Or,
Expand Down Expand Up @@ -383,10 +391,69 @@ var Rem = &Builtin{
}

/**
* Binary
* Bitwise
*/

// TODO(tsandall): update binary operators to support integers.
// BitsOr returns the bitwise "or" of two integers.
var BitsOr = &Builtin{
Name: "bits.or",
Decl: types.NewFunction(
types.Args(types.N, types.N),
types.N,
),
}

// BitsAnd returns the bitwise "and" of two integers.
var BitsAnd = &Builtin{
Name: "bits.and",
Decl: types.NewFunction(
types.Args(types.N, types.N),
types.N,
),
}

// BitsNegate returns the bitwise "negation" of an integer (i.e. flips each
// bit).
var BitsNegate = &Builtin{
Name: "bits.negate",
Decl: types.NewFunction(
types.Args(types.N),
types.N,
),
}

// BitsXOr returns the bitwise "exclusive-or" of two integers.
var BitsXOr = &Builtin{
Name: "bits.xor",
Decl: types.NewFunction(
types.Args(types.N, types.N),
types.N,
),
}

// BitsShiftLeft returns a new integer with its bits shifted some value to the
// left.
var BitsShiftLeft = &Builtin{
Name: "bits.lsh",
Decl: types.NewFunction(
types.Args(types.N, types.N),
types.N,
),
}

// BitsShiftRight returns a new integer with its bits shifted some value to the
// right.
var BitsShiftRight = &Builtin{
Name: "bits.rsh",
Decl: types.NewFunction(
types.Args(types.N, types.N),
types.N,
),
}

/**
* Sets
*/

// And performs an intersection operation on sets.
var And = &Builtin{
Expand Down
11 changes: 11 additions & 0 deletions docs/content/policy-reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,17 @@ The following table shows examples of how ``glob.match`` works:
| ``output := glob.match("{cat,bat,[fr]at}", [], "rat")`` | ``true`` | A glob with pattern-alternatives matchers. |
| ``output := glob.match("{cat,bat,[fr]at}", [], "at")`` | ``false`` | A glob with pattern-alternatives matchers. |

### Bitwise

| Built-in | Description |
| --- | --- |
| <span class="opa-keep-it-together">``z := bits.or(x, y)``</span> | ``z`` is the bitwise or of integers ``x`` and ``y`` |
| <span class="opa-keep-it-together">``z := bits.and(x, y)``</span> | ``z`` is the bitwise and of integers ``x`` and ``y`` |
| <span class="opa-keep-it-together">``z := bits.negate(x)``</span> | ``z`` is the bitwise negation (flip) of integer ``x`` |
| <span class="opa-keep-it-together">``z := bits.xor(x, y)``</span> | ``z`` is the bitwise exclusive-or of integers ``x`` and ``y`` |
| <span class="opa-keep-it-together">``z := bits.lsh(x, s)``</span> | ``z`` is the bitshift of integer ``x`` by ``s`` bits to the left |
| <span class="opa-keep-it-together">``z := bits.rsh(x, s)``</span> | ``z`` is the bitshift of integer ``x`` by ``s`` bits to the right |

### Conversions

| Built-in | Description |
Expand Down
88 changes: 88 additions & 0 deletions topdown/bits.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
// Copyright 2020 The OPA Authors. All rights reserved.
// Use of this source code is governed by an Apache2
// license that can be found in the LICENSE file.

package topdown

import (
"math/big"

"github.com/open-policy-agent/opa/ast"
"github.com/open-policy-agent/opa/topdown/builtins"
)

type bitsArity1 func(a *big.Int) (*big.Int, error)
type bitsArity2 func(a, b *big.Int) (*big.Int, error)

func bitsOr(a, b *big.Int) (*big.Int, error) {
return new(big.Int).Or(a, b), nil
}

func bitsAnd(a, b *big.Int) (*big.Int, error) {
return new(big.Int).And(a, b), nil
}

func bitsNegate(a *big.Int) (*big.Int, error) {
return new(big.Int).Not(a), nil
}

func bitsXOr(a, b *big.Int) (*big.Int, error) {
return new(big.Int).Xor(a, b), nil
}

func bitsShiftLeft(a, b *big.Int) (*big.Int, error) {
if b.Sign() == -1 {
return nil, builtins.NewOperandErr(2, "must be an unsigned integer number but got a negative integer")
}
shift := uint(b.Uint64())
return new(big.Int).Lsh(a, shift), nil
}

func bitsShiftRight(a, b *big.Int) (*big.Int, error) {
if b.Sign() == -1 {
return nil, builtins.NewOperandErr(2, "must be an unsigned integer number but got a negative integer")
}
shift := uint(b.Uint64())
return new(big.Int).Rsh(a, shift), nil
}

func builtinBitsArity1(fn bitsArity1) BuiltinFunc {
return func(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error {
i, err := builtins.BigIntOperand(operands[0].Value, 1)
if err != nil {
return err
}
iOut, err := fn(i)
if err != nil {
return err
}
return iter(ast.NewTerm(builtins.IntToNumber(iOut)))
}
}

func builtinBitsArity2(fn bitsArity2) BuiltinFunc {
return func(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error {
i1, err := builtins.BigIntOperand(operands[0].Value, 1)
if err != nil {
return err
}
i2, err := builtins.BigIntOperand(operands[1].Value, 2)
if err != nil {
return err
}
iOut, err := fn(i1, i2)
if err != nil {
return err
}
return iter(ast.NewTerm(builtins.IntToNumber(iOut)))
}
}

func init() {
RegisterBuiltinFunc(ast.BitsOr.Name, builtinBitsArity2(bitsOr))
RegisterBuiltinFunc(ast.BitsAnd.Name, builtinBitsArity2(bitsAnd))
RegisterBuiltinFunc(ast.BitsNegate.Name, builtinBitsArity1(bitsNegate))
RegisterBuiltinFunc(ast.BitsXOr.Name, builtinBitsArity2(bitsXOr))
RegisterBuiltinFunc(ast.BitsShiftLeft.Name, builtinBitsArity2(bitsShiftLeft))
RegisterBuiltinFunc(ast.BitsShiftRight.Name, builtinBitsArity2(bitsShiftRight))
}
148 changes: 148 additions & 0 deletions topdown/bits_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
// Copyright 2020 The OPA Authors. All rights reserved.
// Use of this source code is governed by an Apache2
// license that can be found in the LICENSE file.

package topdown

import (
"fmt"
"math"
"testing"

"github.com/open-policy-agent/opa/ast"
)

func TestBuiltinBitsOr(t *testing.T) {
tests := []struct {
note string
rules []string
expected interface{}
}{
{"basic bitwise-or", []string{`p[x] { x := bits.or(7, 9) }`}, `[15]`},
{"or with zero is value", []string{`p[x] { x := bits.or(50, 0) }`}, `[50]`},
Copy link
Contributor

@patrick-east patrick-east Feb 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some negative tests for the builtins with invalid types passed in both for the function signature check like passing in a string or something and then for passing in a valid ast.Number but a float that fails to convert to an integer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

{"lhs (float) error", []string{`p = x { x := bits.or(7.2, 42) }`}, &Error{Code: TypeErr, Message: "bits.or: operand 1 must be integer number but got floating-point number"}},
{
"rhs (wrong type-type) error",
[]string{`p = x { x := bits.or(7, "hi") }`},
ast.Errors{ast.NewError(ast.TypeErr, nil, "bits.or: invalid argument(s)")},
},
}

for _, tc := range tests {
runTopDownTestCase(t, map[string]interface{}{}, tc.note, tc.rules, tc.expected)
}
}

func TestBuiltinBitsAnd(t *testing.T) {
tests := []struct {
note string
rules []string
expected interface{}
}{
{"basic bitwise-and", []string{`p[x] { x := bits.and(7, 9) }`}, `[1]`},
{"and with zero is and", []string{`p[x] { x := bits.and(50, 0) }`}, `[0]`},
{"lhs (float) error", []string{`p = x { x := bits.and(7.2, 42) }`}, &Error{Code: TypeErr, Message: "bits.and: operand 1 must be integer number but got floating-point number"}},
{
"rhs (wrong type-type) error",
[]string{`p = x { x := bits.and(7, "hi") }`},
ast.Errors{ast.NewError(ast.TypeErr, nil, "bits.and: invalid argument(s)")},
},
}

for _, tc := range tests {
runTopDownTestCase(t, map[string]interface{}{}, tc.note, tc.rules, tc.expected)
}
}

func TestBuiltinBitsNegate(t *testing.T) {
tests := []struct {
note string
rules []string
expected interface{}
}{
{"basic bitwise-negate", []string{`p[x] { x := bits.negate(42) }`}, `[-43]`},
{"float error", []string{`p = x { x := bits.negate(7.2) }`}, &Error{Code: TypeErr, Message: "bits.negate: operand 1 must be integer number but got floating-point number"}},
{
"type error",
[]string{`p = x { x := bits.negate("hi") }`},
ast.Errors{ast.NewError(ast.TypeErr, nil, "bits.negate: invalid argument(s)")},
},
}

for _, tc := range tests {
runTopDownTestCase(t, map[string]interface{}{}, tc.note, tc.rules, tc.expected)
}
}

func TestBuiltinBitsXOr(t *testing.T) {
tests := []struct {
note string
rules []string
expected interface{}
}{
{"basic bitwise-xor", []string{`p[x] { x := bits.xor(42, 3) }`}, `[41]`},
{"xor same is 0", []string{`p[x] { x := bits.xor(42, 42) }`}, `[0]`},
{"lhs (float) error", []string{`p = x { x := bits.xor(7.2, 42) }`}, &Error{Code: TypeErr, Message: "bits.xor: operand 1 must be integer number but got floating-point number"}},
{
"rhs (wrong type-type) error",
[]string{`p = x { x := bits.xor(7, "hi") }`},
ast.Errors{ast.NewError(ast.TypeErr, nil, "bits.xor: invalid argument(s)")},
},
}

for _, tc := range tests {
runTopDownTestCase(t, map[string]interface{}{}, tc.note, tc.rules, tc.expected)
}
}

func TestBuiltinBitsShiftLeft(t *testing.T) {
tests := []struct {
note string
rules []string
expected interface{}
}{
{"basic shift-left", []string{`p[x] { x := bits.lsh(1, 3) }`}, `[8]`},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worthwhile to add a test case that would cause something smaller than big.int's to overflow to ensure that we don't regress anywhere in the future by transitioning through something smaller (like golang 32 or 64 bit int) and lose bits.

Copy link
Contributor Author

@mjgpy3 mjgpy3 Mar 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrick-east, I'm at a bit of an impasse here and could use assistance, if you have any recommendations.

I tested MaxInt32 shifted left by one (which would cause issues if we were just using int32s) and everything worked okay.

I went to try MaxInt64 using a test like the following

{
	"shift of max int64 doesn't overflow",
	[]string{fmt.Sprintf(`p = x { x := bits.lsh(%d, 1) }`, math.MaxInt64)},
	`18446744073709551614`,
},

However, since the ast's using json.Number under the hood (seems to just be a type definition wrapping string), math.MaxInt64 gets stored as "9.223372037e+18". Then later, when I try to parse it using NumberToInt which uses the following under-the hood

new(big.Int).SetString(string(n), 10)

big.Int can't parse it.

Any ideas? Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, yea so I guess other places we always use big.Float's which has a Parse helper https://golang.org/pkg/math/big/#Float.Parse so it "just works". What we might need to do is something like:

  • Get ast.Number operand (ie json.Number)
  • Get big.Float by parsing the number
  • Get a big.Int from the big.Float via https://golang.org/pkg/math/big/#Float.Int and validate that the returned accuracy is Exact (otherwise we didn't get a valid int to start with).

So maybe if we update NumberToInt to be more like:

func NumberToInt(n ast.Number) (*big.Int, error) {
	f := NumberToFloat(n)
	r, accuracy := f.Int(nil)
	if accuracy != big.Exact {
		return nil, fmt.Errorf("illegal value")
	}
	return r, nil
}

Does that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrick-east that almost works.

The test I posted is outputting the following

            Got: 18446744074000000000
            Exp: 18446744073709551614

I believe that something odd is happening with big integers in general here. I totally could be wrong but I think go's JSON library is actually lossy in that it's converting big integers to exponent-notation.

For example, here's another test (just using modulus to try to recover a digit that I believe is being lost)

		{
			"big numbers JSON?",
			[]string{fmt.Sprintf(`p = x { x := [%d %% 10, %d] }`, math.MaxInt64, math.MaxInt64)},
			fmt.Sprintf("[%d, %d]", math.MaxInt64%10, math.MaxInt64),
		},

It outputs a failure of

            Got: [0 9.223372037e+18]
            Exp:
            [7 9223372036854775807]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yea for sure we do lossy things when we transition between them in the builtin helpers, for example:

// FloatToNumber converts f to a number.
func FloatToNumber(f *big.Float) ast.Number {
return ast.Number(f.String())
}
uses the .String() method that defaults to limit the precision to 10 in https://golang.org/pkg/math/big/#Float.String when it calls https://golang.org/pkg/math/big/#Float.Text (similar for the int's)

I think the golang JSON library just keeps numbers as strings so it might be ok if we fix how we convert them and just give longer strings.

This reminded me that we have #501 which has been open for a long time.

I guess for now maybe we just add a note to document this behavior with large numbers and add test cases for the truncated results (they'll need to be updated when #501 is addressed). I don't want to block this going in for this kind of systematic problem with large numbers in opa.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrick-east, that sounds good. I added that test.

{"lhs (float) error", []string{`p = x { x := bits.lsh(7.2, 42) }`}, &Error{Code: TypeErr, Message: "bits.lsh: operand 1 must be integer number but got floating-point number"}},
{
"rhs (wrong type-type) error",
[]string{`p = x { x := bits.lsh(7, "hi") }`},
ast.Errors{ast.NewError(ast.TypeErr, nil, "bits.lsh: invalid argument(s)")},
},
{"rhs must be unsigned", []string{`p = x { x := bits.lsh(7, -1) }`}, &Error{Code: TypeErr, Message: "bits.lsh: operand 2 must be an unsigned integer number but got a negative integer"}},
{
"shift of max int32 doesn't overflow",
[]string{fmt.Sprintf(`p = x { x := bits.lsh(%d, 1) }`, math.MaxInt32)},
`4294967294`,
},
{
"shift of max int64 doesn't overflow, but it's lossy do to conversion to exponent type (see discussion in #2160)",
[]string{fmt.Sprintf(`p = x { x := bits.lsh(%d, 1) }`, math.MaxInt64)},
`18446744074000000000`,
},
}

for _, tc := range tests {
runTopDownTestCase(t, map[string]interface{}{}, tc.note, tc.rules, tc.expected)
}
}

func TestBuiltinBitsShiftRight(t *testing.T) {
tests := []struct {
note string
rules []string
expected interface{}
}{
{"basic shift-right", []string{`p[x] { x := bits.rsh(8, 3) }`}, `[1]`},
{"lhs (float) error", []string{`p = x { x := bits.rsh(7.2, 42) }`}, &Error{Code: TypeErr, Message: "bits.rsh: operand 1 must be integer number but got floating-point number"}},
{
"rhs (wrong type-type) error",
[]string{`p = x { x := bits.rsh(7, "hi") }`},
ast.Errors{ast.NewError(ast.TypeErr, nil, "bits.rsh: invalid argument(s)")},
},
{"rhs must be unsigned", []string{`p = x { x := bits.rsh(7, -1) }`}, &Error{Code: TypeErr, Message: "bits.rsh: operand 2 must be an unsigned integer number but got a negative integer"}},
}

for _, tc := range tests {
runTopDownTestCase(t, map[string]interface{}{}, tc.note, tc.rules, tc.expected)
}
}
20 changes: 18 additions & 2 deletions topdown/builtins/builtins.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,21 @@ func IntOperand(x ast.Value, pos int) (int, error) {
return i, nil
}

// BigIntOperand converts x to a big int. If the cast fails, a descriptive error
// is returned.
func BigIntOperand(x ast.Value, pos int) (*big.Int, error) {
n, err := NumberOperand(x, 1)
if err != nil {
return nil, NewOperandTypeErr(pos, x, "integer")
}
bi, err := NumberToInt(n)
if err != nil {
return nil, NewOperandErr(pos, "must be integer number but got floating-point number")
}

return bi, nil
}

// NumberOperand converts x to a number. If the cast fails, a descriptive error is
// returned.
func NumberOperand(x ast.Value, pos int) (ast.Number, error) {
Expand Down Expand Up @@ -159,8 +174,9 @@ func FloatToNumber(f *big.Float) ast.Number {
// NumberToInt converts n to a big int.
// If n cannot be converted to an big int, an error is returned.
func NumberToInt(n ast.Number) (*big.Int, error) {
r, ok := new(big.Int).SetString(string(n), 10)
if !ok {
f := NumberToFloat(n)
r, accuracy := f.Int(nil)
if accuracy != big.Exact {
return nil, fmt.Errorf("illegal value")
}
return r, nil
Expand Down