Skip to content

Commit c353467

Browse files
author
Sajjad Rizvi
committed
sql,catalog,typedesc: validate OID range before converting it to ID
Previously the code to convert an OID to a descpb.ID assumed that the OID is larger than a predefined constant. There were no checks to validate the given OID during conversion. As a result, an out-of-range OID could be converted to an invalid descriptor ID without an error. The changes in this PR validate the range of given user-defined OID before conversion, which encourages the user to check the conversion for potential problems. Release note: None Fixes #58414
1 parent a3a3381 commit c353467

15 files changed

+160
-43
lines changed

‎pkg/ccl/backupccl/restore_planning.go

+26-5
Original file line numberDiff line numberDiff line change
@@ -116,14 +116,23 @@ func rewriteTypesInExpr(expr string, rewrites DescRewriteMap) (string, error) {
116116
if err != nil {
117117
return "", err
118118
}
119+
119120
ctx := tree.NewFmtCtx(tree.FmtSerializable)
120121
ctx.SetIndexedTypeFormat(func(ctx *tree.FmtCtx, ref *tree.OIDTypeReference) {
121122
newRef := ref
122-
if rw, ok := rewrites[typedesc.UserDefinedTypeOIDToID(ref.OID)]; ok {
123+
id, ok := typedesc.MaybeUserDefinedTypeOIDToID(ref.OID)
124+
if !ok {
125+
err = typedesc.MakeOIDRangeError(ref.OID)
126+
return
127+
}
128+
if rw, ok := rewrites[id]; ok {
123129
newRef = &tree.OIDTypeReference{OID: typedesc.TypeIDToOID(rw.ID)}
124130
}
125131
ctx.WriteString(newRef.SQLString())
126132
})
133+
if err != nil {
134+
return "", err
135+
}
127136
ctx.FormatNode(parsed)
128137
return ctx.CloseAndGetString(), nil
129138
}
@@ -348,11 +357,15 @@ func allocateDescriptorRewrites(
348357
// Ensure that all referenced types are present.
349358
if col.Type.UserDefined() {
350359
// TODO (rohany): This can be turned into an option later.
351-
if _, ok := typesByID[typedesc.GetTypeDescID(col.Type)]; !ok {
360+
id, isValid := typedesc.GetTypeDescID(col.Type)
361+
if !isValid {
362+
return nil, typedesc.MakeTypeIDRangeError(col.Type)
363+
}
364+
if _, ok := typesByID[id]; !ok {
352365
return nil, errors.Errorf(
353366
"cannot restore table %q without referenced type %d",
354367
table.Name,
355-
typedesc.GetTypeDescID(col.Type),
368+
id,
356369
)
357370
}
358371
}
@@ -1029,13 +1042,21 @@ func rewriteIDsInTypesT(typ *types.T, descriptorRewrites DescRewriteMap) {
10291042
if !typ.UserDefined() {
10301043
return
10311044
}
1045+
tid, ok := typedesc.GetTypeDescID(typ)
1046+
if !ok {
1047+
log.Fatalf(context.TODO(), "%v", typedesc.MakeTypeIDRangeError(typ))
1048+
}
10321049
// Collect potential new OID values.
10331050
var newOID, newArrayOID oid.Oid
1034-
if rw, ok := descriptorRewrites[typedesc.GetTypeDescID(typ)]; ok {
1051+
if rw, ok := descriptorRewrites[tid]; ok {
10351052
newOID = typedesc.TypeIDToOID(rw.ID)
10361053
}
10371054
if typ.Family() != types.ArrayFamily {
1038-
if rw, ok := descriptorRewrites[typedesc.GetArrayTypeDescID(typ)]; ok {
1055+
tid, ok = typedesc.GetArrayTypeDescID(typ)
1056+
if !ok {
1057+
log.Fatalf(context.TODO(), "%v", typedesc.MakeTypeIDRangeError(typ))
1058+
}
1059+
if rw, ok := descriptorRewrites[tid]; ok {
10391060
newArrayOID = typedesc.TypeIDToOID(rw.ID)
10401061
}
10411062
}

‎pkg/ccl/changefeedccl/schemafeed/schema_feed.go

+8-2
Original file line numberDiff line numberDiff line change
@@ -180,13 +180,19 @@ func (t *typeDependencyTracker) removeDependency(typeID, tableID descpb.ID) {
180180

181181
func (t *typeDependencyTracker) purgeTable(tbl catalog.TableDescriptor) {
182182
for _, col := range tbl.UserDefinedTypeColumns() {
183-
t.removeDependency(typedesc.UserDefinedTypeOIDToID(col.GetType().Oid()), tbl.GetID())
183+
id, ok := typedesc.MaybeUserDefinedTypeOIDToID(col.GetType().Oid())
184+
if !ok {
185+
log.Fatalf(context.TODO(), "%v", typedesc.MakeOIDRangeError(col.GetType().Oid()))
186+
}
187+
t.removeDependency(id, tbl.GetID())
184188
}
185189
}
186190

187191
func (t *typeDependencyTracker) ingestTable(tbl catalog.TableDescriptor) {
188192
for _, col := range tbl.UserDefinedTypeColumns() {
189-
t.addDependency(typedesc.UserDefinedTypeOIDToID(col.GetType().Oid()), tbl.GetID())
193+
// Not validating the type as the columns are supposed to have a valid id
194+
id, _ := typedesc.MaybeUserDefinedTypeOIDToID(col.GetType().Oid())
195+
t.addDependency(id, tbl.GetID())
190196
}
191197
}
192198

‎pkg/sql/BUILD.bazel

-1
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,6 @@ go_library(
302302
"//pkg/sql/inverted",
303303
"//pkg/sql/lex",
304304
"//pkg/sql/mutations",
305-
"//pkg/sql/oidext",
306305
"//pkg/sql/opt",
307306
"//pkg/sql/opt/cat",
308307
"//pkg/sql/opt/constraint",

‎pkg/sql/catalog/descs/collection.go

+10-2
Original file line numberDiff line numberDiff line change
@@ -2180,7 +2180,11 @@ func (dt DistSQLTypeResolver) ResolveType(
21802180

21812181
// ResolveTypeByOID implements the tree.TypeReferenceResolver interface.
21822182
func (dt DistSQLTypeResolver) ResolveTypeByOID(ctx context.Context, oid oid.Oid) (*types.T, error) {
2183-
name, desc, err := dt.GetTypeDescriptor(ctx, typedesc.UserDefinedTypeOIDToID(oid))
2183+
id, ok := typedesc.MaybeUserDefinedTypeOIDToID(oid)
2184+
if !ok {
2185+
return nil, typedesc.MakeOIDRangeError(oid)
2186+
}
2187+
name, desc, err := dt.GetTypeDescriptor(ctx, id)
21842188
if err != nil {
21852189
return nil, err
21862190
}
@@ -2213,7 +2217,11 @@ func (dt DistSQLTypeResolver) GetTypeDescriptor(
22132217
func (dt DistSQLTypeResolver) HydrateTypeSlice(ctx context.Context, typs []*types.T) error {
22142218
for _, t := range typs {
22152219
if t.UserDefined() {
2216-
name, desc, err := dt.GetTypeDescriptor(ctx, typedesc.GetTypeDescID(t))
2220+
id, ok := typedesc.GetTypeDescID(t)
2221+
if !ok {
2222+
return typedesc.MakeTypeIDRangeError(t)
2223+
}
2224+
name, desc, err := dt.GetTypeDescriptor(ctx, id)
22172225
if err != nil {
22182226
return err
22192227
}

‎pkg/sql/catalog/tabledesc/structured.go

+5-1
Original file line numberDiff line numberDiff line change
@@ -496,7 +496,11 @@ func (desc *wrapper) getAllReferencedTypesInTableColumns(
496496
// collect the closure of ID's referenced.
497497
ids := make(map[descpb.ID]struct{})
498498
for id := range visitor.OIDs {
499-
typDesc, err := getType(typedesc.UserDefinedTypeOIDToID(id))
499+
uid, ok := typedesc.MaybeUserDefinedTypeOIDToID(id)
500+
if !ok {
501+
return nil, typedesc.MakeOIDRangeError(id)
502+
}
503+
typDesc, err := getType(uid)
500504
if err != nil {
501505
return nil, err
502506
}

‎pkg/sql/catalog/tabledesc/validate.go

+3-1
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,9 @@ func (desc *wrapper) GetReferencedDescIDs() catalog.DescriptorIDSet {
8989
})
9090
// Add collected Oids to return set.
9191
for oid := range visitor.OIDs {
92-
ids.Add(typedesc.UserDefinedTypeOIDToID(oid))
92+
// Assuming that the id converted from OID is valid
93+
id, _ := typedesc.MaybeUserDefinedTypeOIDToID(oid)
94+
ids.Add(id)
9395
}
9496
// Add view dependencies.
9597
for _, id := range desc.GetDependsOn() {

‎pkg/sql/catalog/typedesc/BUILD.bazel

+2
Original file line numberDiff line numberDiff line change
@@ -46,13 +46,15 @@ go_test(
4646
"//pkg/sql/catalog/dbdesc",
4747
"//pkg/sql/catalog/descpb",
4848
"//pkg/sql/catalog/schemadesc",
49+
"//pkg/sql/oidext",
4950
"//pkg/sql/privilege",
5051
"//pkg/sql/types",
5152
"//pkg/testutils",
5253
"//pkg/testutils/serverutils",
5354
"//pkg/util/leaktest",
5455
"//pkg/util/randutil",
5556
"@com_github_cockroachdb_redact//:redact",
57+
"@com_github_lib_pq//oid",
5658
"@com_github_stretchr_testify//require",
5759
"@in_gopkg_yaml_v2//:yaml_v2",
5860
],

‎pkg/sql/catalog/typedesc/type_desc.go

+41-13
Original file line numberDiff line numberDiff line change
@@ -113,21 +113,36 @@ func TypeIDToOID(id descpb.ID) oid.Oid {
113113
return oid.Oid(id) + oidext.CockroachPredefinedOIDMax
114114
}
115115

116-
// UserDefinedTypeOIDToID converts a user defined type OID into a
117-
// descriptor ID.
118-
func UserDefinedTypeOIDToID(oid oid.Oid) descpb.ID {
119-
return descpb.ID(oid) - oidext.CockroachPredefinedOIDMax
116+
// MaybeUserDefinedTypeOIDToID converts a user defined type OID into a
117+
// descriptor ID. OID of a user-defined type must be greater than
118+
// CockraochPredefinedOIDMax.
119+
func MaybeUserDefinedTypeOIDToID(oid oid.Oid) (descpb.ID, bool) {
120+
if descpb.ID(oid) <= oidext.CockroachPredefinedOIDMax {
121+
return 0, false
122+
}
123+
return descpb.ID(oid) - oidext.CockroachPredefinedOIDMax, true
124+
}
125+
126+
// MakeOIDRangeError is a helper to create an error message for invalid user-defined OID conversion.
127+
func MakeOIDRangeError(oid oid.Oid) error {
128+
return errors.Newf("user-defined OID %d is out of range. It must be greater than Max OID: %d.",
129+
oid, oidext.CockroachPredefinedOIDMax)
120130
}
121131

122132
// GetTypeDescID gets the type descriptor ID from a user defined type.
123-
func GetTypeDescID(t *types.T) descpb.ID {
124-
return UserDefinedTypeOIDToID(t.Oid())
133+
func GetTypeDescID(t *types.T) (descpb.ID, bool) {
134+
return MaybeUserDefinedTypeOIDToID(t.Oid())
125135
}
126136

127137
// GetArrayTypeDescID gets the ID of the array type descriptor from a user
128138
// defined type.
129-
func GetArrayTypeDescID(t *types.T) descpb.ID {
130-
return UserDefinedTypeOIDToID(t.UserDefinedArrayOID())
139+
func GetArrayTypeDescID(t *types.T) (descpb.ID, bool) {
140+
return MaybeUserDefinedTypeOIDToID(t.UserDefinedArrayOID())
141+
}
142+
143+
// MakeTypeIDRangeError is a helper to create an error message for invalid type ID conversion.
144+
func MakeTypeIDRangeError(t *types.T) error {
145+
return MakeOIDRangeError(t.Oid())
131146
}
132147

133148
// TypeDesc implements the Descriptor interface.
@@ -599,10 +614,13 @@ func (desc *immutable) ValidateCrossReferences(
599614
}
600615
case descpb.TypeDescriptor_ALIAS:
601616
if desc.GetAlias().UserDefined() {
602-
aliasedID := UserDefinedTypeOIDToID(desc.GetAlias().Oid())
617+
aliasedID, isValid := MaybeUserDefinedTypeOIDToID(desc.GetAlias().Oid())
603618
if _, err := vdg.GetTypeDescriptor(aliasedID); err != nil {
604619
vea.Report(errors.Wrapf(err, "aliased type %d does not exist", aliasedID))
605620
}
621+
if !isValid {
622+
vea.Report(errors.Wrapf(err, "aliased type %d is out of range", aliasedID))
623+
}
606624
}
607625
}
608626

@@ -724,7 +742,11 @@ func HydrateTypesInTableDescriptor(
724742
hydrateCol := func(col *descpb.ColumnDescriptor) error {
725743
if col.Type.UserDefined() {
726744
// Look up its type descriptor.
727-
name, typDesc, err := res.GetTypeDescriptor(ctx, GetTypeDescID(col.Type))
745+
td, ok := GetTypeDescID(col.Type)
746+
if !ok {
747+
return MakeTypeIDRangeError(col.Type)
748+
}
749+
name, typDesc, err := res.GetTypeDescriptor(ctx, td)
728750
if err != nil {
729751
return err
730752
}
@@ -787,7 +809,11 @@ func (desc *immutable) HydrateTypeInfoWithName(
787809
case types.ArrayFamily:
788810
// Hydrate the element type.
789811
elemType := typ.ArrayContents()
790-
elemTypName, elemTypDesc, err := res.GetTypeDescriptor(ctx, GetTypeDescID(elemType))
812+
id, ok := GetTypeDescID(elemType)
813+
if !ok {
814+
return MakeTypeIDRangeError(elemType)
815+
}
816+
elemTypName, elemTypDesc, err := res.GetTypeDescriptor(ctx, id)
791817
if err != nil {
792818
return err
793819
}
@@ -925,9 +951,10 @@ func GetTypeDescriptorClosure(typ *types.T) map[descpb.ID]struct{} {
925951
if !typ.UserDefined() {
926952
return map[descpb.ID]struct{}{}
927953
}
954+
id, _ := GetTypeDescID(typ)
928955
// Collect the type's descriptor ID.
929956
ret := map[descpb.ID]struct{}{
930-
GetTypeDescID(typ): {},
957+
id: {},
931958
}
932959
if typ.Family() == types.ArrayFamily {
933960
// If we have an array type, then collect all types in the contents.
@@ -937,7 +964,8 @@ func GetTypeDescriptorClosure(typ *types.T) map[descpb.ID]struct{} {
937964
}
938965
} else {
939966
// Otherwise, take the array type ID.
940-
ret[GetArrayTypeDescID(typ)] = struct{}{}
967+
id, _ := GetArrayTypeDescID(typ)
968+
ret[id] = struct{}{}
941969
}
942970
return ret
943971
}

‎pkg/sql/catalog/typedesc/type_desc_test.go

+22
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ package typedesc_test
1313
import (
1414
"context"
1515
"fmt"
16+
"math"
1617
"testing"
1718

1819
"github.com/cockroachdb/cockroach/pkg/keys"
@@ -22,10 +23,12 @@ import (
2223
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
2324
"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc"
2425
"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
26+
"github.com/cockroachdb/cockroach/pkg/sql/oidext"
2527
"github.com/cockroachdb/cockroach/pkg/sql/privilege"
2628
"github.com/cockroachdb/cockroach/pkg/sql/types"
2729
"github.com/cockroachdb/cockroach/pkg/testutils"
2830
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
31+
"github.com/lib/pq/oid"
2932
"github.com/stretchr/testify/require"
3033
)
3134

@@ -784,3 +787,22 @@ func TestValidateTypeDesc(t *testing.T) {
784787
}
785788
}
786789
}
790+
791+
func TestOIDToIDConversion(t *testing.T) {
792+
tests := []struct {
793+
oid oid.Oid
794+
ok bool
795+
tname string
796+
}{
797+
{oid.Oid(0), false, "default OID"},
798+
{oid.Oid(1), false, "PG OID"},
799+
{oid.Oid(oidext.CockroachPredefinedOIDMax), false, "User-defined OID"},
800+
{oid.Oid(oidext.CockroachPredefinedOIDMax + 1), true, "user-defined OID"},
801+
{oid.Oid(math.MaxUint32), true, "max user-defined OID"},
802+
}
803+
804+
for _, test := range tests {
805+
_, ok := typedesc.MaybeUserDefinedTypeOIDToID(test.oid)
806+
require.Equalf(t, test.ok, ok, "%s test: expected: %t, but found: %t", test.tname, test.ok, ok)
807+
}
808+
}

‎pkg/sql/database_region_change_finalizer.go

+8-2
Original file line numberDiff line numberDiff line change
@@ -169,8 +169,14 @@ func (r *databaseRegionChangeFinalizer) repartitionRegionalByRowTables(
169169
// the table descriptor with the new type metadata.
170170
for i := range tableDesc.Columns {
171171
col := &tableDesc.Columns[i]
172-
if col.Type.UserDefined() && typedesc.UserDefinedTypeOIDToID(col.Type.Oid()) == r.typeID {
173-
col.Type.TypeMeta = types.UserDefinedTypeMetadata{}
172+
if col.Type.UserDefined() {
173+
tid, ok := typedesc.MaybeUserDefinedTypeOIDToID(col.Type.Oid())
174+
if !ok {
175+
return typedesc.MakeOIDRangeError(col.Type.Oid())
176+
}
177+
if tid == r.typeID {
178+
col.Type.TypeMeta = types.UserDefinedTypeMetadata{}
179+
}
174180
}
175181
}
176182
if err := typedesc.HydrateTypesInTableDescriptor(

‎pkg/sql/opt/testutils/testcat/test_catalog.go

+5-1
Original file line numberDiff line numberDiff line change
@@ -794,7 +794,11 @@ func (tt *Table) CollectTypes(ord int) (descpb.IDs, error) {
794794

795795
ids := make(descpb.IDs, 0, len(visitor.OIDs))
796796
for collectedOid := range visitor.OIDs {
797-
ids = append(ids, typedesc.UserDefinedTypeOIDToID(collectedOid))
797+
id, ok := typedesc.MaybeUserDefinedTypeOIDToID(collectedOid)
798+
if !ok {
799+
return nil, typedesc.MakeOIDRangeError(collectedOid, id)
800+
}
801+
ids = append(ids, id)
798802
}
799803
return ids, nil
800804
}

‎pkg/sql/opt_catalog.go

+5-1
Original file line numberDiff line numberDiff line change
@@ -2274,7 +2274,11 @@ func collectTypes(col catalog.Column) (descpb.IDs, error) {
22742274

22752275
ids := make(descpb.IDs, 0, len(visitor.OIDs))
22762276
for collectedOid := range visitor.OIDs {
2277-
ids = append(ids, typedesc.UserDefinedTypeOIDToID(collectedOid))
2277+
id, ok := typedesc.MaybeUserDefinedTypeOIDToID(collectedOid)
2278+
if !ok {
2279+
return nil, typedesc.MakeOIDRangeError(collectedOid)
2280+
}
2281+
ids = append(ids, id)
22782282
}
22792283
return ids, nil
22802284
}

‎pkg/sql/pg_catalog.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ import (
3030
"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
3131
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
3232
"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
33-
"github.com/cockroachdb/cockroach/pkg/sql/oidext"
3433
"github.com/cockroachdb/cockroach/pkg/sql/parser"
3534
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
3635
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
@@ -2437,17 +2436,18 @@ https://www.postgresql.org/docs/9.5/catalog-pg-type.html`,
24372436
return true, nil
24382437
}
24392438

2439+
// Check if it is a user defined type.
2440+
id, ok := typedesc.MaybeUserDefinedTypeOIDToID(ooid)
2441+
24402442
// This oid is not a user-defined type and we didn't find it in the
24412443
// map of predefined types, return false. Note that in common usage we
24422444
// only really expect the value 0 here (which cockroach uses internally
24432445
// in the typelem field amongst others). Users, however, may join on
24442446
// this index with any value.
2445-
if ooid <= oidext.CockroachPredefinedOIDMax {
2447+
if !ok {
24462448
return false, nil
24472449
}
24482450

2449-
// Check if it is a user defined type.
2450-
id := typedesc.UserDefinedTypeOIDToID(ooid)
24512451
typDesc, err := p.Descriptors().GetImmutableTypeByID(ctx, p.txn, id, tree.ObjectLookupFlags{})
24522452
if err != nil {
24532453
if errors.Is(err, catalog.ErrDescriptorNotFound) {

0 commit comments

Comments
 (0)