From d5a068d5abfefce8bb513f46d4f5ff824be5d551 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Mon, 25 Mar 2019 16:51:38 -0700 Subject: [PATCH] distsqlrun: fix data race on evalCtx.iVarContainerStack Previously, it was possible to run into a data race on the slice evalCtx.iVarContainerStack because we were simply using a copy by dereference to create a new eval context. However, this is not sufficient since the slices still point to the same underlying memory, so now we make a deeper copy of the slice. Release note: None --- pkg/sql/apply_join.go | 11 ++++------- pkg/sql/distsqlrun/flow.go | 3 +-- pkg/sql/explain_distsql.go | 9 +++------ pkg/sql/insert.go | 2 +- pkg/sql/planner.go | 12 ++++++++++++ pkg/sql/sem/tree/eval.go | 8 ++++++++ pkg/sql/sem/tree/eval_internal_test.go | 16 ++++++++++++++++ pkg/sql/upsert.go | 2 +- 8 files changed, 46 insertions(+), 17 deletions(-) diff --git a/pkg/sql/apply_join.go b/pkg/sql/apply_join.go index bebfda52bd60..2178e82cce95 100644 --- a/pkg/sql/apply_join.go +++ b/pkg/sql/apply_join.go @@ -306,10 +306,7 @@ func (a *applyJoinNode) runRightSidePlan(params runParams, plan *planTop) error if !params.p.extendedEvalCtx.ExecCfg.DistSQLPlanner.PlanAndRunSubqueries( params.ctx, params.p, - func() *extendedEvalContext { - ret := *params.extendedEvalCtx - return &ret - }, + params.extendedEvalCtx.copy, plan.subqueryPlans, recv, true, @@ -321,8 +318,8 @@ func (a *applyJoinNode) runRightSidePlan(params runParams, plan *planTop) error } // Make a copy of the EvalContext so it can be safely modified. - evalCtx := *params.p.ExtendedEvalContext() - planCtx := params.p.extendedEvalCtx.ExecCfg.DistSQLPlanner.newLocalPlanningCtx(params.ctx, &evalCtx) + evalCtx := params.p.ExtendedEvalContextCopy() + planCtx := params.p.extendedEvalCtx.ExecCfg.DistSQLPlanner.newLocalPlanningCtx(params.ctx, evalCtx) // Always plan local. planCtx.isLocal = true plannerCopy := *params.p @@ -332,7 +329,7 @@ func (a *applyJoinNode) runRightSidePlan(params runParams, plan *planTop) error planCtx.stmtType = recv.stmtType params.p.extendedEvalCtx.ExecCfg.DistSQLPlanner.PlanAndRun( - params.ctx, &evalCtx, planCtx, params.p.Txn(), plan.plan, recv) + params.ctx, evalCtx, planCtx, params.p.Txn(), plan.plan, recv) if recv.commErr != nil { return recv.commErr } diff --git a/pkg/sql/distsqlrun/flow.go b/pkg/sql/distsqlrun/flow.go index 41d6e597fe81..a468bbb16e4f 100644 --- a/pkg/sql/distsqlrun/flow.go +++ b/pkg/sql/distsqlrun/flow.go @@ -124,8 +124,7 @@ type FlowCtx struct { // them at runtime to ensure expressions are evaluated with the correct indexed // var context. func (ctx *FlowCtx) NewEvalCtx() *tree.EvalContext { - evalCtx := *ctx.EvalCtx - return &evalCtx + return ctx.EvalCtx.Copy() } // TestingKnobs returns the distsql testing knobs for this flow context. diff --git a/pkg/sql/explain_distsql.go b/pkg/sql/explain_distsql.go index af94421de7da..d18cb5ec2d37 100644 --- a/pkg/sql/explain_distsql.go +++ b/pkg/sql/explain_distsql.go @@ -130,10 +130,7 @@ func (n *explainDistSQLNode) startExec(params runParams) error { if !distSQLPlanner.PlanAndRunSubqueries( planCtx.ctx, params.p, - func() *extendedEvalContext { - ret := *params.extendedEvalCtx - return &ret - }, + params.extendedEvalCtx.copy, n.subqueryPlans, recv, true, @@ -181,10 +178,10 @@ func (n *explainDistSQLNode) startExec(params runParams) error { planCtx.ctx = ctx // Make a copy of the evalContext with the recording span in it; we can't // change the original. - newEvalCtx := *params.extendedEvalCtx + newEvalCtx := params.extendedEvalCtx.copy() newEvalCtx.Context = ctx newParams := params - newParams.extendedEvalCtx = &newEvalCtx + newParams.extendedEvalCtx = newEvalCtx // Discard rows that are returned. rw := newCallbackResultWriter(func(ctx context.Context, row tree.Datums) error { diff --git a/pkg/sql/insert.go b/pkg/sql/insert.go index 199aed7d4ed7..6cfa20d6dd22 100644 --- a/pkg/sql/insert.go +++ b/pkg/sql/insert.go @@ -529,7 +529,7 @@ func (n *insertNode) processSourceRow(params runParams, sourceVals tree.Datums) n.run.computeExprs, n.run.insertCols, n.run.computedCols, - *params.EvalContext(), + *params.EvalContext().Copy(), n.run.ti.tableDesc(), sourceVals, &n.run.iVarContainerForComputedCols, diff --git a/pkg/sql/planner.go b/pkg/sql/planner.go index b67e994c8bb4..da0c9871cf39 100644 --- a/pkg/sql/planner.go +++ b/pkg/sql/planner.go @@ -76,6 +76,13 @@ type extendedEvalContext struct { schemaAccessors *schemaInterface } +// copy returns a deep copy of ctx. +func (ctx *extendedEvalContext) copy() *extendedEvalContext { + cpy := *ctx + cpy.EvalContext = *ctx.EvalContext.Copy() + return &cpy +} + // schemaInterface provides access to the database and table descriptors. // See schema_accessors.go. type schemaInterface struct { @@ -332,10 +339,15 @@ func (p *planner) LogicalSchemaAccessor() SchemaAccessor { return p.extendedEvalCtx.schemaAccessors.logical } +// Note: if the context will be modified, use ExtendedEvalContextCopy instead. func (p *planner) ExtendedEvalContext() *extendedEvalContext { return &p.extendedEvalCtx } +func (p *planner) ExtendedEvalContextCopy() *extendedEvalContext { + return p.extendedEvalCtx.copy() +} + func (p *planner) CurrentDatabase() string { return p.SessionData().Database } diff --git a/pkg/sql/sem/tree/eval.go b/pkg/sql/sem/tree/eval.go index a34a1b978b6e..febecad1f560 100644 --- a/pkg/sql/sem/tree/eval.go +++ b/pkg/sql/sem/tree/eval.go @@ -2618,6 +2618,14 @@ func MakeTestingEvalContextWithMon(st *cluster.Settings, monitor *mon.BytesMonit return ctx } +// Copy returns a deep copy of ctx. +func (ctx *EvalContext) Copy() *EvalContext { + ctxCopy := *ctx + ctxCopy.iVarContainerStack = make([]IndexedVarContainer, len(ctx.iVarContainerStack), cap(ctx.iVarContainerStack)) + copy(ctxCopy.iVarContainerStack, ctx.iVarContainerStack) + return &ctxCopy +} + // PushIVarContainer replaces the current IVarContainer with a different one - // pushing the current one onto a stack to be replaced later once // PopIVarContainer is called. diff --git a/pkg/sql/sem/tree/eval_internal_test.go b/pkg/sql/sem/tree/eval_internal_test.go index d2baaa3bc0a2..349025261bc5 100644 --- a/pkg/sql/sem/tree/eval_internal_test.go +++ b/pkg/sql/sem/tree/eval_internal_test.go @@ -17,6 +17,8 @@ package tree import ( "fmt" "testing" + + "github.com/cockroachdb/cockroach/pkg/util/leaktest" ) func TestUnescapePattern(t *testing.T) { @@ -119,3 +121,17 @@ func TestReplaceUnescaped(t *testing.T) { }) } } + +// TestEvalContextCopy verifies that EvalContext.Copy() produces a deep copy of +// EvalContext. +func TestEvalContextCopy(t *testing.T) { + defer leaktest.AfterTest(t)() + // Note: the test relies on "parent" EvalContext having non-nil and non-empty + // iVarContainerStack. + ctx := EvalContext{iVarContainerStack: make([]IndexedVarContainer, 1)} + + cpy := ctx.Copy() + if &ctx.iVarContainerStack[0] == &cpy.iVarContainerStack[0] { + t.Fatal("iVarContainerStacks are the same") + } +} diff --git a/pkg/sql/upsert.go b/pkg/sql/upsert.go index 49424cf9c09a..203c64cc0351 100644 --- a/pkg/sql/upsert.go +++ b/pkg/sql/upsert.go @@ -371,7 +371,7 @@ func (n *upsertNode) processSourceRow(params runParams, sourceVals tree.Datums) n.run.computeExprs, n.run.insertCols, n.run.computedCols, - *params.EvalContext(), + *params.EvalContext().Copy(), n.run.tw.tableDesc(), sourceVals, &n.run.iVarContainerForComputedCols,