Commit 9d64636c authored by Nathan VanBenschoten's avatar Nathan VanBenschoten

sql: properly handle nil byte slices in golangFillQueryArguments

Before this, nil byte slices were being treated as empty byte slices,
which resulted in confusing behavior where a nil byte slice passed to an
InternalExecutor would not be NULL.

Also, fix TestGolangQueryArgs, which was completely broken and asserting
that `reflect.Type(*types.T) == reflect.Type(*types.T)`.

This change is what caught the bug fixed by the previous change.
parent 7901304f
......@@ -59,7 +59,7 @@ FROM (
version + 1 AS new_version,
num_records + 1 AS new_num_records,
num_spans + $3 AS new_num_spans,
total_bytes + length($9) + length($6) + length($7) AS new_total_bytes
total_bytes + length($9) + length($6) + coalesce(length($7:::BYTES),0) AS new_total_bytes
FROM
current_meta
)
......@@ -135,7 +135,7 @@ RETURNING
SELECT
id,
num_spans AS record_spans,
length(spans) + length(meta_type) + length(meta) AS record_bytes
length(spans) + length(meta_type) + coalesce(length(meta),0) AS record_bytes
FROM
system.protected_ts_records
WHERE
......
......@@ -67,13 +67,22 @@ func (p *storage) Protect(ctx context.Context, txn *kv.Txn, r *ptpb.Record) erro
if err != nil { // how can this possibly fail?
return errors.Wrap(err, "failed to marshal spans")
}
meta := r.Meta
if meta == nil {
// v20.1 crashes in rowToRecord and storage.Release if it finds a NULL
// value in system.protected_ts_records.meta. v20.2 and above handle
// this correctly, but we need to maintain mixed version compatibility
// for at least one release.
// TODO(nvanbenschoten): remove this for v21.1.
meta = []byte{}
}
s := makeSettings(p.settings)
rows, err := p.ex.QueryEx(ctx, "protectedts-protect", txn,
sqlbase.InternalExecutorSessionDataOverride{User: security.NodeUser},
protectQuery,
s.maxSpans, s.maxBytes, len(r.Spans),
r.ID.GetBytesMut(), r.Timestamp.AsOfSystemTime(),
r.MetaType, r.Meta,
r.MetaType, meta,
len(r.Spans), encodedSpans)
if err != nil {
return errors.Wrapf(err, "failed to write record %v", r.ID)
......@@ -218,8 +227,10 @@ func rowToRecord(ctx context.Context, row tree.Datums, r *ptpb.Record) error {
r.Timestamp = ts
r.MetaType = string(*row[2].(*tree.DString))
if meta := row[3].(*tree.DBytes); meta != nil && len(*meta) > 0 {
r.Meta = []byte(*meta)
if row[3] != tree.DNull {
if meta := row[3].(*tree.DBytes); len(*meta) > 0 {
r.Meta = []byte(*meta)
}
}
var spans Spans
if err := protoutil.Unmarshal([]byte(*row[4].(*tree.DBytes)), &spans); err != nil {
......
......@@ -146,6 +146,15 @@ func (n *alterRoleNode) startExec(params runParams) error {
"setting or updating a password is not supported in insecure mode")
}
if hashedPassword == nil {
// v20.1 and below crash during authentication if they find a NULL value
// in system.users.hashedPassword. v20.2 and above handle this correctly,
// but we need to maintain mixed version compatibility for at least one
// release.
// TODO(nvanbenschoten): remove this for v21.1.
hashedPassword = []byte{}
}
// Updating PASSWORD is a special case since PASSWORD lives in system.users
// while the rest of the role options lives in system.role_options.
_, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
......
......@@ -125,6 +125,13 @@ func (n *CreateRoleNode) startExec(params runParams) error {
return pgerror.New(pgcode.InvalidPassword,
"setting or updating a password is not supported in insecure mode")
}
} else {
// v20.1 and below crash during authentication if they find a NULL value
// in system.users.hashedPassword. v20.2 and above handle this correctly,
// but we need to maintain mixed version compatibility for at least one
// release.
// TODO(nvanbenschoten): remove this for v21.1.
hashedPassword = []byte{}
}
// Reject the "public" role. It does not have an entry in the users table but is reserved.
......
......@@ -905,8 +905,10 @@ func golangFillQueryArguments(args ...interface{}) (tree.Datums, error) {
case reflect.String:
d = tree.NewDString(val.String())
case reflect.Slice:
// Handle byte slices.
if val.Type().Elem().Kind() == reflect.Uint8 {
switch {
case val.IsNil():
d = tree.DNull
case val.Type().Elem().Kind() == reflect.Uint8:
d = tree.NewDBytes(tree.DBytes(val.Bytes()))
}
}
......
......@@ -182,56 +182,57 @@ func TestGolangQueryArgs(t *testing.T) {
// type
testCases := []struct {
value interface{}
expectedType reflect.Type
expectedType *types.T
}{
// Null type.
{nil, reflect.TypeOf(types.Unknown)},
{nil, types.Unknown},
{[]byte(nil), types.Unknown},
// Bool type.
{true, reflect.TypeOf(types.Bool)},
{true, types.Bool},
// Primitive Integer types.
{int(1), reflect.TypeOf(types.Int)},
{int8(1), reflect.TypeOf(types.Int)},
{int16(1), reflect.TypeOf(types.Int)},
{int32(1), reflect.TypeOf(types.Int)},
{int64(1), reflect.TypeOf(types.Int)},
{uint(1), reflect.TypeOf(types.Int)},
{uint8(1), reflect.TypeOf(types.Int)},
{uint16(1), reflect.TypeOf(types.Int)},
{uint32(1), reflect.TypeOf(types.Int)},
{uint64(1), reflect.TypeOf(types.Int)},
{int(1), types.Int},
{int8(1), types.Int},
{int16(1), types.Int},
{int32(1), types.Int},
{int64(1), types.Int},
{uint(1), types.Int},
{uint8(1), types.Int},
{uint16(1), types.Int},
{uint32(1), types.Int},
{uint64(1), types.Int},
// Primitive Float types.
{float32(1.0), reflect.TypeOf(types.Float)},
{float64(1.0), reflect.TypeOf(types.Float)},
{float32(1.0), types.Float},
{float64(1.0), types.Float},
// Decimal type.
{apd.New(55, 1), reflect.TypeOf(types.Decimal)},
{apd.New(55, 1), types.Decimal},
// String type.
{"test", reflect.TypeOf(types.String)},
{"test", types.String},
// Bytes type.
{[]byte("abc"), reflect.TypeOf(types.Bytes)},
{[]byte("abc"), types.Bytes},
// Interval and timestamp.
{time.Duration(1), reflect.TypeOf(types.Interval)},
{timeutil.Now(), reflect.TypeOf(types.Timestamp)},
{time.Duration(1), types.Interval},
{timeutil.Now(), types.Timestamp},
// Primitive type aliases.
{roachpb.NodeID(1), reflect.TypeOf(types.Int)},
{sqlbase.ID(1), reflect.TypeOf(types.Int)},
{floatAlias(1), reflect.TypeOf(types.Float)},
{boolAlias(true), reflect.TypeOf(types.Bool)},
{stringAlias("string"), reflect.TypeOf(types.String)},
{roachpb.NodeID(1), types.Int},
{sqlbase.ID(1), types.Int},
{floatAlias(1), types.Float},
{boolAlias(true), types.Bool},
{stringAlias("string"), types.String},
// Byte slice aliases.
{roachpb.Key("key"), reflect.TypeOf(types.Bytes)},
{roachpb.RKey("key"), reflect.TypeOf(types.Bytes)},
{roachpb.Key("key"), types.Bytes},
{roachpb.RKey("key"), types.Bytes},
// Bit array.
{bitarray.MakeBitArrayFromInt64(8, 58, 7), reflect.TypeOf(types.VarBit)},
{bitarray.MakeBitArrayFromInt64(8, 58, 7), types.VarBit},
}
for i, tcase := range testCases {
......@@ -241,7 +242,7 @@ func TestGolangQueryArgs(t *testing.T) {
t.Fatalf("expected 1 datum, got: %d", len(datums))
}
d := datums[0]
if a, e := reflect.TypeOf(d.ResolvedType()), tcase.expectedType; a != e {
if a, e := d.ResolvedType(), tcase.expectedType; !a.Equal(e) {
t.Errorf("case %d failed: expected type %s, got %s", i, e.String(), a.String())
}
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment