Merge #156949

156949: sql: prevent mutation of synthetic privilege cache entries r=fqazi a=fqazi

Previously, when GRANT / REVOKE were modifying synthetic privileges, they requested a copy of the privilege descriptor from the synthetic privilege cache, which would make a shallow copy. This meant that returned entries were not safe for mutations, since slices would be shared between copies. This could lead to unexpected behavior for concurrent transactions or schema change retries, since slices could get modified in unexpected ways. To address this, this patch marks the existing method as immutable and makes a variant aimed at modification that is labeled as mutable.

Fixes: #156503
Fixes: #155822
Fixes: #155858
Fixes: #156393
Fixes: #142992

Release note (bug fix): Transactions running concurrently with a GRANT / REVOKE on virtual tables / external connections could observe modifications incorrectly.

Co-authored-by: Faizan Qazi <[email protected]>

Author: craig[bot]

pkg/sql/authorization.go

@@ -156,7 +156,7 @@ func (p *planner) HasPrivilege(
 	// permission check).
 	p.maybeAuditSensitiveTableAccessEvent(privilegeObject, privilegeKind)
 
-	privs, err := p.getPrivilegeDescriptor(ctx, privilegeObject)
+	privs, err := p.getImmutablePrivilegeDescriptor(ctx, privilegeObject)
 	if err != nil {
 		return false, err
 	}
@@ -209,7 +209,7 @@ func (p *planner) HasAnyPrivilege(
 		return true, nil
 	}
 
-	privs, err := p.getPrivilegeDescriptor(ctx, privilegeObject)
+	privs, err := p.getImmutablePrivilegeDescriptor(ctx, privilegeObject)
 	if err != nil {
 		return false, err
 	}
@@ -373,7 +373,7 @@ func (p *planner) getOwnerOfPrivilegeObject(
 	if d, ok := privilegeObject.(catalog.TableDescriptor); ok && d.IsVirtualTable() {
 		return username.NodeUserName(), nil
 	}
-	privDesc, err := p.getPrivilegeDescriptor(ctx, privilegeObject)
+	privDesc, err := p.getImmutablePrivilegeDescriptor(ctx, privilegeObject)
 	if err != nil {
 		return username.SQLUsername{}, err
 	}

pkg/sql/grant_revoke_system.go

@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
+	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/errors"
 )
 
@@ -48,7 +49,7 @@ func (n *changeNonDescriptorBackedPrivilegesNode) startExec(params runParams) er
 		if err := catprivilege.ValidateSyntheticPrivilegeObject(systemPrivilegeObject); err != nil {
 			return err
 		}
-		syntheticPrivDesc, err := params.p.getPrivilegeDescriptor(params.ctx, systemPrivilegeObject)
+		syntheticPrivDesc, err := params.p.getMutablePrivilegeDescriptor(params.ctx, systemPrivilegeObject)
 		if err != nil {
 			return err
 		}
@@ -255,10 +256,11 @@ func (n *changeNonDescriptorBackedPrivilegesNode) makeSystemPrivilegeObject(
 	}
 }
 
-// getPrivilegeDescriptor returns the privilege descriptor for the
+// getImmutablePrivilegeDescriptor returns the privilege descriptor for the
 // object. Note that for non-descriptor backed objects, we query the
-// system.privileges table to synthesize a PrivilegeDescriptor.
-func (p *planner) getPrivilegeDescriptor(
+// system.privileges table to synthesize a PrivilegeDescriptor. The returned
+// privilege descriptor is not safe for modification.
+func (p *planner) getImmutablePrivilegeDescriptor(
 	ctx context.Context, po privilege.Object,
 ) (*catpb.PrivilegeDescriptor, error) {
 	switch d := po.(type) {
@@ -294,3 +296,26 @@ func (p *planner) getPrivilegeDescriptor(
 	}
 	return nil, errors.AssertionFailedf("unknown privilege.Object type %T", po)
 }
+
+// getMutablePrivilegeDescriptor returns the privilege descriptor for the
+// object. Note that for non-descriptor backed objects, we query the
+// system.privileges table to synthesize a PrivilegeDescriptor. The returned
+// copy can be safely modified.
+func (p *planner) getMutablePrivilegeDescriptor(
+	ctx context.Context, po privilege.Object,
+) (*catpb.PrivilegeDescriptor, error) {
+	pd, err := p.getImmutablePrivilegeDescriptor(ctx, po)
+	if err != nil {
+		return nil, err
+	}
+	// No copy is needed for non-synthetic descriptors.
+	switch d := po.(type) {
+	case catalog.TableDescriptor:
+		if !d.IsVirtualTable() {
+			return pd, nil
+		}
+	case catalog.Descriptor:
+		return pd, nil
+	}
+	return protoutil.Clone(pd).(*catpb.PrivilegeDescriptor), nil
+}

pkg/sql/information_schema.go

@@ -408,7 +408,7 @@ https://www.postgresql.org/docs/9.5/infoschema-column-privileges.html`,
 			dbNameStr := tree.NewDString(db.GetName())
 			scNameStr := tree.NewDString(sc.GetName())
 			columndata := privilege.List{privilege.SELECT, privilege.INSERT, privilege.UPDATE} // privileges for column level granularity
-			privDesc, err := p.getPrivilegeDescriptor(ctx, table)
+			privDesc, err := p.getImmutablePrivilegeDescriptor(ctx, table)
 			if err != nil {
 				return err
 			}
@@ -1610,7 +1610,7 @@ func populateTablePrivileges(
 			// TODO(knz): This should filter for the current user, see
 			// https://github.com/cockroachdb/cockroach/issues/35572
 			tableType := table.GetObjectType()
-			desc, err := p.getPrivilegeDescriptor(ctx, table)
+			desc, err := p.getImmutablePrivilegeDescriptor(ctx, table)
 			if err != nil {
 				return err
 			}
@@ -1623,7 +1623,7 @@ func populateTablePrivileges(
 				for _, priv := range u.Privileges {
 					// We use this function to check for the grant option so that the
 					// object owner also gets is_grantable=true.
-					privs, err := p.getPrivilegeDescriptor(ctx, table)
+					privs, err := p.getImmutablePrivilegeDescriptor(ctx, table)
 					if err != nil {
 						return err
 					}

pkg/sql/pg_catalog.go

@@ -3246,7 +3246,7 @@ https://www.postgresql.org/docs/9.6/catalog-pg-shdepend.html`,
 		if err = forEachTableDesc(ctx, p, dbContext, opts,
 			func(ctx context.Context, descCtx tableDescContext) error {
 				db, table := descCtx.database, descCtx.table
-				privDesc, err := p.getPrivilegeDescriptor(ctx, table)
+				privDesc, err := p.getImmutablePrivilegeDescriptor(ctx, table)
 				if err != nil {
 					return err
 				}

pkg/sql/resolver.go

@@ -136,7 +136,7 @@ func (p *planner) HasAnyPrivilegeForSpecifier(
 		}
 
 		if priv.GrantOption {
-			privDesc, err := p.getPrivilegeDescriptor(ctx, privObject)
+			privDesc, err := p.getImmutablePrivilegeDescriptor(ctx, privObject)
 			if err != nil {
 				return eval.HasNoPrivilege, err
 			}