From 0a5558d2323d3d91b6042116437ccb8a9dd37023 Mon Sep 17 00:00:00 2001 From: Benjamin DENEUX Date: Tue, 23 Jul 2024 08:01:17 +0200 Subject: [PATCH 1/5] feat: add a maxVariable limit and check if its reached --- engine/builtin_test.go | 2 +- engine/text_test.go | 4 ++-- engine/variable.go | 26 +++++++++++++++++++++----- engine/vm.go | 2 +- engine/vm_test.go | 4 ++-- 5 files changed, 27 insertions(+), 11 deletions(-) diff --git a/engine/builtin_test.go b/engine/builtin_test.go index d747fce..6546f49 100644 --- a/engine/builtin_test.go +++ b/engine/builtin_test.go @@ -952,7 +952,7 @@ func TestTermVariables(t *testing.T) { func TestOp(t *testing.T) { t.Run("insert", func(t *testing.T) { t.Run("atom", func(t *testing.T) { - varCounter = 1 + varCounter.count = 1 vm := VM{_operators: newOperators()} vm.getOperators().define(900, operatorSpecifierXFX, NewAtom(`+++`)) diff --git a/engine/text_test.go b/engine/text_test.go index 2eb3a31..5f2da3b 100644 --- a/engine/text_test.go +++ b/engine/text_test.go @@ -24,7 +24,7 @@ func mustOpen(fs fs.FS, name string) fs.File { } func TestVM_Compile(t *testing.T) { - varCounter = 1 + varCounter.count = 1 tests := []struct { title string @@ -483,7 +483,7 @@ bar(b). for _, tt := range tests { t.Run(tt.title, func(t *testing.T) { var vm VM - varCounter = 1 // Global var cause issues in testing environment that call in randomly order for checking equality between procedure clause args + varCounter.count = 1 // Global var cause issues in testing environment that call in randomly order for checking equality between procedure clause args vm.getOperators().define(1200, operatorSpecifierXFX, atomIf) vm.getOperators().define(1200, operatorSpecifierXFX, atomArrow) diff --git a/engine/variable.go b/engine/variable.go index a93380e..89142a7 100644 --- a/engine/variable.go +++ b/engine/variable.go @@ -1,15 +1,26 @@ package engine import ( + "errors" "fmt" "io" - "sync/atomic" + "sync" ) -var varCounter int64 +var errMaxVariables = errors.New("maximum number of variables reached") + +var maxVariables uint64 +var varCounter = struct { + sync.Mutex + count uint64 +}{ + count: 0, +} func lastVariable() Variable { - return Variable(varCounter) + defer varCounter.Unlock() + varCounter.Lock() + return Variable(varCounter.count) } // Variable is a prolog variable. @@ -17,8 +28,13 @@ type Variable int64 // NewVariable creates a new anonymous variable. func NewVariable() Variable { - n := atomic.AddInt64(&varCounter, 1) - return Variable(n) + defer varCounter.Unlock() + varCounter.Lock() + if maxVariables != 0 && varCounter.count >= maxVariables { + panic(errMaxVariables) + } + varCounter.count++ + return Variable(varCounter.count) } func (v Variable) WriteTerm(w io.Writer, opts *WriteOptions, env *Env) error { diff --git a/engine/vm.go b/engine/vm.go index c77ad61..eb82727 100644 --- a/engine/vm.go +++ b/engine/vm.go @@ -278,7 +278,7 @@ func (vm *VM) SetUserOutput(s *Stream) { // ResetEnv is used to reset all global variable func (vm *VM) ResetEnv() { - varCounter = 0 + varCounter.count = 0 varContext = NewVariable() rootContext = NewAtom("root") rootEnv = &Env{ diff --git a/engine/vm_test.go b/engine/vm_test.go index 1fb733d..e8c8e4b 100644 --- a/engine/vm_test.go +++ b/engine/vm_test.go @@ -289,7 +289,7 @@ func TestProcedureIndicator_Apply(t *testing.T) { func TestVM_ResetEnv(t *testing.T) { var vm VM - varCounter = 10 + varCounter.count = 10 varContext = NewVariable() rootContext = NewAtom("non-root") rootEnv = &Env{ @@ -302,7 +302,7 @@ func TestVM_ResetEnv(t *testing.T) { t.Run("Reset environment", func(t *testing.T) { vm.ResetEnv() - assert.Equal(t, int64(1), varCounter) // 1 because NewVariable() is called in ResetEnv() + assert.Equal(t, uint64(1), varCounter.count) // 1 because NewVariable() is called in ResetEnv() assert.Equal(t, "root", rootContext.String()) assert.Equal(t, newEnvKey(varContext), rootEnv.binding.key) assert.Equal(t, NewAtom("root"), rootEnv.binding.value) From fc26b9d85ca0a739f22c62fe1b8c4be24dcd2629 Mon Sep 17 00:00:00 2001 From: Benjamin DENEUX Date: Tue, 23 Jul 2024 08:13:15 +0200 Subject: [PATCH 2/5] feat: add vm paramter to set MaxVariables --- engine/vm.go | 10 ++++++++++ engine/vm_test.go | 4 ++++ 2 files changed, 14 insertions(+) diff --git a/engine/vm.go b/engine/vm.go index eb82727..35e2a47 100644 --- a/engine/vm.go +++ b/engine/vm.go @@ -70,6 +70,9 @@ type VM struct { streams streams input, output *Stream + // Limits + maxVariables uint64 + // Misc debug bool } @@ -276,6 +279,12 @@ func (vm *VM) SetUserOutput(s *Stream) { vm.output = s } +// SetMaxVariables sets the maximum number of variables that the VM can create. +func (vm *VM) SetMaxVariables(n uint64) { + vm.maxVariables = n + maxVariables = n +} + // ResetEnv is used to reset all global variable func (vm *VM) ResetEnv() { varCounter.count = 0 @@ -287,6 +296,7 @@ func (vm *VM) ResetEnv() { value: rootContext, }, } + maxVariables = vm.maxVariables } func (vm *VM) getProcedure(p procedureIndicator) (procedure, bool) { diff --git a/engine/vm_test.go b/engine/vm_test.go index e8c8e4b..23913ba 100644 --- a/engine/vm_test.go +++ b/engine/vm_test.go @@ -289,6 +289,8 @@ func TestProcedureIndicator_Apply(t *testing.T) { func TestVM_ResetEnv(t *testing.T) { var vm VM + vm.maxVariables = 10 + varCounter.count = 10 varContext = NewVariable() rootContext = NewAtom("non-root") @@ -298,6 +300,7 @@ func TestVM_ResetEnv(t *testing.T) { value: NewAtom("non-root"), }, } + maxVariables = 20 t.Run("Reset environment", func(t *testing.T) { vm.ResetEnv() @@ -306,5 +309,6 @@ func TestVM_ResetEnv(t *testing.T) { assert.Equal(t, "root", rootContext.String()) assert.Equal(t, newEnvKey(varContext), rootEnv.binding.key) assert.Equal(t, NewAtom("root"), rootEnv.binding.value) + assert.Equal(t, uint64(10), maxVariables) }) } From a32f8909c1ff6a7bec90f23ffe45d4af4dfee942 Mon Sep 17 00:00:00 2001 From: Benjamin DENEUX Date: Tue, 23 Jul 2024 08:18:16 +0200 Subject: [PATCH 3/5] test: check setting max variables at VM level --- engine/vm.go | 1 + engine/vm_test.go | 15 ++++++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/engine/vm.go b/engine/vm.go index 35e2a47..acfae9d 100644 --- a/engine/vm.go +++ b/engine/vm.go @@ -280,6 +280,7 @@ func (vm *VM) SetUserOutput(s *Stream) { } // SetMaxVariables sets the maximum number of variables that the VM can create. +// Zero value mean no limits func (vm *VM) SetMaxVariables(n uint64) { vm.maxVariables = n maxVariables = n diff --git a/engine/vm_test.go b/engine/vm_test.go index 23913ba..aeba7ec 100644 --- a/engine/vm_test.go +++ b/engine/vm_test.go @@ -270,6 +270,15 @@ func TestVM_SetUserOutput(t *testing.T) { }) } +func TestVM_SetMaxVariables(t *testing.T) { + t.Run("ok", func(t *testing.T) { + var vm VM + vm.SetMaxVariables(10) + assert.Equal(t, uint64(10), maxVariables) + assert.Equal(t, uint64(10), vm.maxVariables) + }) +} + func TestProcedureIndicator_Apply(t *testing.T) { t.Run("ok", func(t *testing.T) { c, err := procedureIndicator{name: NewAtom("foo"), arity: 2}.Apply(NewAtom("a"), NewAtom("b")) @@ -289,7 +298,7 @@ func TestProcedureIndicator_Apply(t *testing.T) { func TestVM_ResetEnv(t *testing.T) { var vm VM - vm.maxVariables = 10 + vm.SetMaxVariables(20) varCounter.count = 10 varContext = NewVariable() @@ -300,7 +309,7 @@ func TestVM_ResetEnv(t *testing.T) { value: NewAtom("non-root"), }, } - maxVariables = 20 + maxVariables = 30 t.Run("Reset environment", func(t *testing.T) { vm.ResetEnv() @@ -309,6 +318,6 @@ func TestVM_ResetEnv(t *testing.T) { assert.Equal(t, "root", rootContext.String()) assert.Equal(t, newEnvKey(varContext), rootEnv.binding.key) assert.Equal(t, NewAtom("root"), rootEnv.binding.value) - assert.Equal(t, uint64(10), maxVariables) + assert.Equal(t, uint64(20), maxVariables) }) } From 0358f8fb7dbecf1a53c4b05196df9f7f96f138e2 Mon Sep 17 00:00:00 2001 From: Benjamin DENEUX Date: Tue, 23 Jul 2024 10:51:29 +0200 Subject: [PATCH 4/5] test: check max variables panics on new variable --- engine/variable_test.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/engine/variable_test.go b/engine/variable_test.go index efe4b8e..73f7161 100644 --- a/engine/variable_test.go +++ b/engine/variable_test.go @@ -141,3 +141,39 @@ func Test_freeVariablesSet(t *testing.T) { assert.Equal(t, tt.fv, newFreeVariablesSet(tt.t, tt.v, nil)) } } + +func Test_maxVariables(t *testing.T) { + tests := []struct { + title string + init func() + max uint64 + expectedCount uint64 + shouldPanic bool + }{ + {title: "no limits", init: func() { + NewVariable() + NewVariable() + }, max: 0, expectedCount: 2}, + {title: "limit", init: func() { + NewVariable() + NewVariable() + }, max: 2, expectedCount: 2}, + {title: "limit reached", init: func() { + NewVariable() + NewVariable() + }, max: 1, expectedCount: 1, shouldPanic: true}, + } + + for _, tt := range tests { + varCounter.count = 0 // reset at each test + + maxVariables = tt.max + + if tt.shouldPanic { + assert.Panics(t, tt.init) + } else { + tt.init() + } + assert.Equal(t, varCounter.count, tt.expectedCount) + } +} From 5392a0f1a86062fd185e9b6346ed8cc7ce2ed5dd Mon Sep 17 00:00:00 2001 From: Benjamin DENEUX Date: Tue, 23 Jul 2024 11:35:27 +0200 Subject: [PATCH 5/5] test: improve SetMaxVariables test coverage --- engine/vm_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/engine/vm_test.go b/engine/vm_test.go index aeba7ec..285da4d 100644 --- a/engine/vm_test.go +++ b/engine/vm_test.go @@ -271,12 +271,19 @@ func TestVM_SetUserOutput(t *testing.T) { } func TestVM_SetMaxVariables(t *testing.T) { - t.Run("ok", func(t *testing.T) { + t.Run("limits", func(t *testing.T) { var vm VM vm.SetMaxVariables(10) assert.Equal(t, uint64(10), maxVariables) assert.Equal(t, uint64(10), vm.maxVariables) }) + + t.Run("no limit", func(t *testing.T) { + var vm VM + vm.SetMaxVariables(0) + assert.Equal(t, uint64(0), maxVariables) + assert.Equal(t, uint64(0), vm.maxVariables) + }) } func TestProcedureIndicator_Apply(t *testing.T) {