From ba6d42acbb7fd7af5d9adf250d4599600079067a Mon Sep 17 00:00:00 2001 From: Leon Mika Date: Sun, 25 May 2025 10:08:56 +1000 Subject: [PATCH] Fixed bug which was not isolating procs --- ucl/builtins.go | 2 +- ucl/env.go | 36 +++++++++++---- ucl/inst.go | 6 ++- ucl/inst_test.go | 114 +++++++++++++++++++++++------------------------ 4 files changed, 89 insertions(+), 69 deletions(-) diff --git a/ucl/builtins.go b/ucl/builtins.go index da7ddc4..71bad9b 100644 --- a/ucl/builtins.go +++ b/ucl/builtins.go @@ -1161,7 +1161,7 @@ func procBuiltin(ctx context.Context, args macroArgs) (Object, error) { obj := procObject{args.eval, args.ec, blockObj.block} if procName != "" { - args.ec.addCmd(procName, obj) + args.ec.addUserCmd(procName, obj) } return obj, nil } diff --git a/ucl/env.go b/ucl/env.go index 6d92ffb..7a58cc1 100644 --- a/ucl/env.go +++ b/ucl/env.go @@ -1,18 +1,17 @@ package ucl type evalCtx struct { - root *evalCtx - parent *evalCtx - commands map[string]invokable - macros map[string]macroable - vars map[string]Object - pseudoVars map[string]pseudoVar + root *evalCtx + parent *evalCtx + commands map[string]invokable + macros map[string]macroable + vars map[string]Object + pseudoVars map[string]pseudoVar + userCommandFrame bool // Frame to use for user-defined commands } func (ec *evalCtx) forkAndIsolate() *evalCtx { - newEc := &evalCtx{parent: ec} - newEc.root = newEc - return newEc + return &evalCtx{parent: ec, root: ec.root, userCommandFrame: true} } func (ec *evalCtx) fork() *evalCtx { @@ -27,6 +26,25 @@ func (ec *evalCtx) addCmd(name string, inv invokable) { ec.root.commands[name] = inv } +func (ec *evalCtx) addUserCmd(name string, inv invokable) { + frame := ec + for frame != nil { + if frame.userCommandFrame { + break + } + frame = frame.parent + } + if frame == nil { + panic("no user command frame found") + } + + if frame.commands == nil { + frame.commands = make(map[string]invokable) + } + + frame.commands[name] = inv +} + func (ec *evalCtx) addMacro(name string, inv macroable) { if ec.root.macros == nil { ec.root.macros = make(map[string]macroable) diff --git a/ucl/inst.go b/ucl/inst.go index 791b202..074433e 100644 --- a/ucl/inst.go +++ b/ucl/inst.go @@ -53,7 +53,9 @@ type Module struct { } func New(opts ...InstOption) *Inst { - rootEC := &evalCtx{} + rootEC := &evalCtx{ + userCommandFrame: true, + } rootEC.root = rootEC rootEC.addCmd("echo", invokableFunc(echoBuiltin)) @@ -194,7 +196,7 @@ func (inst *Inst) eval(ctx context.Context, r io.Reader, opts evalOptions) (Obje env := inst.rootEC if opts.forkEnv { - env = env.fork() + env = env.forkAndIsolate() } return eval.evalScript(ctx, env, ast) diff --git a/ucl/inst_test.go b/ucl/inst_test.go index 184a3b0..06c60a0 100644 --- a/ucl/inst_test.go +++ b/ucl/inst_test.go @@ -145,72 +145,72 @@ func TestInst_Eval_WithSubEnv(t *testing.T) { assert.Nil(t, res) }) - t.Run("when using hooks, reading vars are not permitted to leak across environment", func(t *testing.T) { - ctx := t.Context() + t.Run("environments should not leak when using hooks", func(t *testing.T) { + tests := []struct { + descr string + eval1 string + eval2 string + want1 any + want2 any + }{ + { + descr: "reading vars", + eval1: `$a = "hello" ; hook { $a }`, + eval2: `$a = "world" ; hook { $a }`, + want1: "hello", + want2: "world", + }, + { + descr: "modifying vars", + eval1: `$a = "hello" ; hook { $a = "new value" ; $a }`, + eval2: `$a = "world" ; hook { $a }`, + want1: "new value", + want2: "world", + }, + { + descr: "defining procs", + eval1: `proc say_hello { "hello" } ; hook { say_hello }`, + eval2: `proc say_hello { "world" } ; hook { say_hello }`, + want1: "hello", + want2: "world", + }, + } - hooks := make([]ucl.Invokable, 0) + for _, tt := range tests { + t.Run(tt.descr, func(t *testing.T) { + ctx := t.Context() - inst := ucl.New() - inst.SetBuiltin("hook", func(ctx context.Context, args ucl.CallArgs) (any, error) { - var hookProc ucl.Invokable + hooks := make([]ucl.Invokable, 0) - if err := args.Bind(&hookProc); err != nil { - return nil, err - } - hooks = append(hooks, hookProc) + inst := ucl.New() + inst.SetBuiltin("hook", func(ctx context.Context, args ucl.CallArgs) (any, error) { + var hookProc ucl.Invokable - return nil, nil - }) + if err := args.Bind(&hookProc); err != nil { + return nil, err + } + hooks = append(hooks, hookProc) - res, err := inst.Eval(ctx, strings.NewReader(`$a = "hello" ; hook { $a }`), ucl.WithSubEnv()) - assert.NoError(t, err) - assert.Nil(t, res) + return nil, nil + }) - res, err = inst.Eval(ctx, strings.NewReader(`$a = "world" ; hook { $a }`), ucl.WithSubEnv()) - assert.NoError(t, err) - assert.Nil(t, res) + res, err := inst.Eval(ctx, strings.NewReader(tt.eval1), ucl.WithSubEnv()) + assert.NoError(t, err) + assert.Nil(t, res) - h1, err := hooks[0].Invoke(ctx, ucl.CallArgs{}) - assert.NoError(t, err) - assert.Equal(t, "hello", h1) + res, err = inst.Eval(ctx, strings.NewReader(tt.eval2), ucl.WithSubEnv()) + assert.NoError(t, err) + assert.Nil(t, res) - h2, err := hooks[1].Invoke(ctx, ucl.CallArgs{}) - assert.NoError(t, err) - assert.Equal(t, "world", h2) - }) + h1, err := hooks[0].Invoke(ctx, ucl.CallArgs{}) + assert.NoError(t, err) + assert.Equal(t, tt.want1, h1) - t.Run("when using hooks, writing vars are not permitted to leak across environment", func(t *testing.T) { - ctx := t.Context() - - hooks := make([]ucl.Invokable, 0) - - inst := ucl.New() - inst.SetBuiltin("hook", func(ctx context.Context, args ucl.CallArgs) (any, error) { - var hookProc ucl.Invokable - - if err := args.Bind(&hookProc); err != nil { - return nil, err - } - hooks = append(hooks, hookProc) - - return nil, nil - }) - - res, err := inst.Eval(ctx, strings.NewReader(`$a = "hello" ; hook { $a = "new value" ; $a }`), ucl.WithSubEnv()) - assert.NoError(t, err) - assert.Nil(t, res) - - res, err = inst.Eval(ctx, strings.NewReader(`$a = "world" ; hook { $a }`), ucl.WithSubEnv()) - assert.NoError(t, err) - assert.Nil(t, res) - - h1, err := hooks[0].Invoke(ctx, ucl.CallArgs{}) - assert.NoError(t, err) - assert.Equal(t, "new value", h1) - - h2, err := hooks[1].Invoke(ctx, ucl.CallArgs{}) - assert.NoError(t, err) - assert.Equal(t, "world", h2) + h2, err := hooks[1].Invoke(ctx, ucl.CallArgs{}) + assert.NoError(t, err) + assert.Equal(t, tt.want2, h2) + }) + } }) }