sql/catalog/typedesc: validate OID range before converting it to ID

[sajjadrizvi]

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

[cockroach-teamcity]

This change is 

[pbardea]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @RichardJCai, and @sajjadrizvi)

---

pkg/ccl/backupccl/restore_planning.go, line 1045 at r1 (raw file):

		return
	}
	tid, _ := typedesc.GetTypeDescID(typ)

Why is it okay to not return an error if it's not valid in this case?

---

pkg/ccl/changefeedccl/schemafeed/schema_feed.go, line 183 at r1 (raw file):

func (t *typeDependencyTracker) purgeTable(tbl catalog.TableDescriptor) {
	for _, col := range tbl.UserDefinedTypeColumns() {
		// Not validating the type as the columns are supposed to have a valid id

If it's not valid that would be noteworthy, right? Perhaps it's at least worth logging or perhaps panicing in non-release builds? Adding cdc team for these changes.

[sajjadrizvi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @pbardea, and @RichardJCai)

---

pkg/ccl/backupccl/restore_planning.go, line 1045 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Why is it okay to not return an error if it's not valid in this case?

I thought the passed types are already validated in this case and they must be within the correct range. As the function does not returns an error, I will log an error message if the ID conversion is not OK.

---

pkg/ccl/changefeedccl/schemafeed/schema_feed.go, line 183 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

If it's not valid that would be noteworthy, right? Perhaps it's at least worth logging or perhaps panicing in non-release builds? Adding cdc team for these changes.

Yeah, right. I should at least log an error.

[miretskiy]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @pbardea, and @RichardJCai)

---

pkg/ccl/backupccl/restore_planning.go, line 1045 at r1 (raw file):

Previously, sajjadrizvi (Sajjad Rizvi) wrote…

I thought the passed types are already validated in this case and they must be within the correct range. As the function does not returns an error, I will log an error message if the ID conversion is not OK.

I'm uncomfortable seeing log fatal calls. I think validation aside, "things" could happen.
For example, we know what type of data we store in the jobs table, but we still check
and return error if we failed to deserialize proto. This feels very similar to me.
I think we should return an error.

[fqazi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @pbardea, @RichardJCai, and @sajjadrizvi)

---

pkg/sql/type_change.go, line 820 at r2 (raw file):

			}

			id, _ := typedesc.GetTypeDescID(col.GetType())

Should we check if the conversion is okay?

---

pkg/sql/type_change.go, line 928 at r2 (raw file):

		firstClause := true
		for _, col := range desc.PublicColumns() {
			id, _ := typedesc.GetTypeDescID(col.GetType())

Similar any danger of not checking?

---

pkg/sql/catalog/typedesc/type_desc.go, line 119 at r2 (raw file):

// descriptor ID. OID of a user-defined type must be greater than
// CockraochPredefinedOIDMax.
func MaybeUserDefinedTypeOIDToID(oid oid.Oid) (descpb.ID, bool) {

Minor: You don't have to do this, but would it be cleaner to just have a function that returns an error on failure to convert? Disregard, if that was something you discussed/considered.

[stevendanna]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @pbardea, @RichardJCai, and @sajjadrizvi)

---

pkg/ccl/changefeedccl/schemafeed/schema_feed.go, line 183 at r1 (raw file):

Previously, sajjadrizvi (Sajjad Rizvi) wrote…

Yeah, right. I should at least log an error.

The only caller of this function is from a function that returns an error and has its own context. Given that, I'd prefer that we return the error up the callstack both here and at the callsite in validateDescriptor and then log at the Error level rather than Fatal. An error from validateDescriptor will result in a failed changefeed job AFAIK and we'd like to limit the number of places where changefeeds can bring down the database.

If this is the kind of error where we really don't want to continue once we've seen it, even inside changefeeds, perhaps a more descriptive error would be useful so that we know what table and column this happened on?

[sajjadrizvi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @fqazi, @pbardea, and @RichardJCai)

---

pkg/ccl/backupccl/restore_planning.go, line 1045 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

I'm uncomfortable seeing log fatal calls. I think validation aside, "things" could happen.
For example, we know what type of data we store in the jobs table, but we still check
and return error if we failed to deserialize proto. This feels very similar to me.
I think we should return an error.

OK. I am modifying the code to return an error. It makes more sense to return an error.

I will avoid log fatal calls.

---

pkg/ccl/changefeedccl/schemafeed/schema_feed.go, line 183 at r1 (raw file):

and then log at the Error level rather than Fatal.
Do you mean to log an error in the caller of validateDescriptor() instead of returning the error further up the callstack? The current caller of validateDescriptor(), which is ingestDescriptors(), eventually returns the error further up in the stack.

perhaps a more descriptive error would be useful ...
Thanks, I will keep that in mind.

---

pkg/sql/type_change.go, line 820 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Should we check if the conversion is okay?

Yes, thanks for pointing out. I have modified it to check and return an error if the conversion fails.

---

pkg/sql/type_change.go, line 928 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Similar any danger of not checking?

Yeah, fixed that as well. Thanks.

---

pkg/sql/catalog/typedesc/type_desc.go, line 119 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Minor: You don't have to do this, but would it be cleaner to just have a function that returns an error on failure to convert? Disregard, if that was something you discussed/considered.

I think it is cleaner to just return an error whenever we return false, we create an error message in the caller function and return an error. Essentially, we are just returning an error whenever conversion fails. I was planning to do it.

[sajjadrizvi]

This is a work-in-progress (WIP), not ready for the thorough review yet.

[sajjadrizvi]

This commit is now ready for another review (RFAR).

[pbardea]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @fqazi, @pbardea, @RichardJCai, and @sajjadrizvi)

---

pkg/ccl/backupccl/restore_planning.go, line 123 at r3 (raw file):

	ctx.SetIndexedTypeFormat(func(ctx *tree.FmtCtx, ref *tree.OIDTypeReference) {
		newRef := ref
		id, err := typedesc.UserDefinedTypeOIDToID(ref.OID)

I don't think this will set the right err. This should define a new err scoped to this inner func and won't set the err that this function closes over. To do this, I think we need something like:

var id descpb.ID
id, err = typedesc.UserDefinedTypeOIDToID(ref.OID)

[sajjadrizvi]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @fqazi, @pbardea, and @RichardJCai)

---

pkg/ccl/backupccl/restore_planning.go, line 123 at r3 (raw file):

Previously, pbardea (Paul Bardea) wrote…

I don't think this will set the right err. This should define a new err scoped to this inner func and won't set the err that this function closes over. To do this, I think we need something like:

var id descpb.ID
id, err = typedesc.UserDefinedTypeOIDToID(ref.OID)

Good catch! I have now fixed it.

[pbardea]

Bulk IO changes LGTM

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @fqazi, @pbardea, and @RichardJCai)

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @fqazi, @pbardea, and @sajjadrizvi)

---

pkg/sql/type_change.go, line 663 at r4 (raw file):

			id, err := typedesc.UserDefinedTypeOIDToID(typeOid.OID)
			if err != nil || id != typeID {
				return true, expr, nil

We should return the err if it's not nil

---

pkg/sql/type_change.go, line 687 at r4 (raw file):

			id = id - 1
			if err != nil || id != typeID {
				return true, expr, nil

Ditto

---

pkg/sql/type_change.go, line 732 at r4 (raw file):

		id, err := typedesc.UserDefinedTypeOIDToID(typeOid.OID)
		if err != nil || id != typeID {
			return true, expr, nil

Ditto

---

pkg/sql/catalog/tabledesc/validate.go, line 94 at r4 (raw file):

		id, err := typedesc.UserDefinedTypeOIDToID(oid)
		if err != nil {
			panic(err)

I think we should change this function to error as well instead of panicking, it looks like this is called by Validate

Don't think we wanna panic while validating.

// either as a slice or combined as one.```

---

pkg/sql/catalog/typedesc/type_desc.go, line 953 at r4 (raw file):

	id, err := GetUserDefinedTypeDescID(typ)
	if err != nil {
		panic(err)

I prefer returning an error here instead of panicking even if the only caller of this function will panic if theres an error.

If we add any more callers, they may not want a function it's calling to panic.

[ajwerner]

This is getting very close.

Reviewed 1 of 14 files at r3, 2 of 8 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @fqazi, @pbardea, and @sajjadrizvi)

---

pkg/sql/catalog/typedesc/type_desc_test.go, line 810 at r7 (raw file):

Quoted 5 lines of code…

			t.Fatalf("%s test: expected error, but succeeded", test.name)
		} else if test.ok && err != nil {
			t.Fatalf("%s test: expected success, but got error: %s", test.name, err.Error())
		}

this is totally fine, however, I want to make sure you're aware of require

I would write this:

for _, test := range tests {
    t.Run(fmt.Sprint(test.oid), func(t *testing.T) {
        _, err := typedesc.UserDefinedTypeOIDToID(test.oid)
        if test.ok {
            require.NoError(t, err)
        } else {
            require.Regexp(t, "user-defined oid \d+.*", err) // could make a clearer regexp
        }
    })
}

It's the same size but it carries the metadata in a more structured way and generally for a lot of use cases can be a good bit more terse. require will fatal and its sister assert will just error.

---

pkg/sql/opt/optbuilder/builder.go, line 429 at r7 (raw file):

}

func (b *Builder) maybeTrackUserDefinedTypeDepsForViews(texpr tree.TypedExpr) error {

A caller to this needs to use this error, however, in this file, we panic errors because the builder always catches them. So, tl;dr, panic this error with confidence that it'll be caught.

---

pkg/sql/sem/tree/casts.go, line 1307 at r6 (raw file):

		} else if typ, err := ctx.Planner.ResolveTypeByOID(ctx.Context, oid.Oid(v)); err == nil {
			ret.name = typ.PGName()
		} // It seems that the error above is not checked and returned intentionally.

How about:

		} else if typedesc.IsOIDUserDefinedType(oid.Oid(v)) {
			typ, err := ctx.Planner.ResolveTypeByOID(ctx.Context, oid.Oid(v))
                        if err != nil {
                            return nil, err
                        }
                        ret.name = typ.PGName()
		}

[ajwerner]

mod the one nit and removing WIP from the title

Reviewed 1 of 8 files at r2, 3 of 14 files at r3, 2 of 6 files at r5, 2 of 8 files at r6, 4 of 6 files at r7, 1 of 3 files at r8, 4 of 4 files at r9.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @fqazi, @pbardea, and @sajjadrizvi)

---

pkg/sql/catalog/typedesc/type_desc.go, line 118 at r9 (raw file):

// UserDefinedTypeOIDToID converts a user defined type OID into a
// descriptor ID. OID of a user-defined type must be greater than
// CockroachPredefinedOIDMax. The function throws an error if the

nit: returns rather than throws

[sajjadrizvi]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @fqazi, @pbardea, and @RichardJCai)

---

pkg/ccl/changefeedccl/schemafeed/schema_feed.go, line 183 at r1 (raw file):

Previously, sajjadrizvi (Sajjad Rizvi) wrote…

and then log at the Error level rather than Fatal.
Do you mean to log an error in the caller of validateDescriptor() instead of returning the error further up the callstack? The current caller of validateDescriptor(), which is ingestDescriptors(), eventually returns the error further up in the stack.

perhaps a more descriptive error would be useful ...
Thanks, I will keep that in mind.

Done.

---

pkg/sql/type_change.go, line 820 at r2 (raw file):

Previously, sajjadrizvi (Sajjad Rizvi) wrote…

Yes, thanks for pointing out. I have modified it to check and return an error if the conversion fails.

Done.

---

pkg/sql/type_change.go, line 928 at r2 (raw file):

Previously, sajjadrizvi (Sajjad Rizvi) wrote…

Yeah, fixed that as well. Thanks.

Done.

---

pkg/sql/type_change.go, line 663 at r4 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

We should return the err if it's not nil

Done.

---

pkg/sql/type_change.go, line 687 at r4 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Ditto

Done.

---

pkg/sql/type_change.go, line 732 at r4 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Ditto

Done.

---

pkg/sql/catalog/tabledesc/validate.go, line 94 at r4 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think we should change this function to error as well instead of panicking, it looks like this is called by Validate

Don't think we wanna panic while validating.

// either as a slice or combined as one.```
</blockquote></details>

Done.
___
*[pkg/sql/catalog/typedesc/type_desc.go, line 953 at r4](https://reviewable.io/reviews/cockroachdb/cockroach/65352#-MaEcq8-BeiHV745zv79:-MaiD321CEM-_grtZRro:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/93362929db1371fec81332ce7196fde16ab477dd/pkg/sql/catalog/typedesc/type_desc.go#L953)):*
<details><summary><i>Previously, RichardJCai (Richard Cai) wrote…</i></summary><blockquote>

I prefer returning an error here instead of panicking even if the only caller of this function will panic if theres an error.

If we add any more callers, they may not want a function it's calling to panic.
</blockquote></details>

Done.
___
*[pkg/sql/catalog/typedesc/type_desc_test.go, line 810 at r7](https://reviewable.io/reviews/cockroachdb/cockroach/65352#-MaiMSpj7iOnsR3RqXfZ:-MaivZy91D0B1zui39e6:br1bocw) ([raw file](https://github.com/cockroachdb/cockroach/blob/db3231e2098af01a40bd0de42c844a91b7594291/pkg/sql/catalog/typedesc/type_desc_test.go#L810)):*
<details><summary><i>Previously, ajwerner wrote…</i></summary><blockquote>

> ```
> 
> 			t.Fatalf("%s test: expected error, but succeeded", test.name)
> 		} else if test.ok && err != nil {
> 			t.Fatalf("%s test: expected success, but got error: %s", test.name, err.Error())
> 		}
> ```
this is totally fine, however, I want to make sure you're aware of `require` 

I would write this:

```go
for _, test := range tests {
    t.Run(fmt.Sprint(test.oid), func(t *testing.T) {
        _, err := typedesc.UserDefinedTypeOIDToID(test.oid)
        if test.ok {
            require.NoError(t, err)
        } else {
            require.Regexp(t, "user-defined oid \d+.*", err) // could make a clearer regexp
        }
    })
}

It's the same size but it carries the metadata in a more structured way and generally for a lot of use cases can be a good bit more terse. require will fatal and its sister assert will just error.

Done. Thanks for the pointer to the require package.

---

pkg/sql/opt/optbuilder/builder.go, line 429 at r7 (raw file):

Previously, ajwerner wrote…

A caller to this needs to use this error, however, in this file, we panic errors because the builder always catches them. So, tl;dr, panic this error with confidence that it'll be caught.

I called panic in the parent function. I have now changed it.

---

pkg/sql/sem/tree/casts.go, line 1307 at r6 (raw file):

Previously, ajwerner wrote…

How about:

		} else if typedesc.IsOIDUserDefinedType(oid.Oid(v)) {
			typ, err := ctx.Planner.ResolveTypeByOID(ctx.Context, oid.Oid(v))
                        if err != nil {
                            return nil, err
                        }
                        ret.name = typ.PGName()
		}

Done.

[ajwerner]

Super nit, don't to change anything, in the future I think the header I'd use is sql/catalog/typedesc. Commas are when the bulk of the change impacts multiple packages.

Give it a bors when CI passes.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @fqazi, @pbardea, and @RichardJCai)

[sajjadrizvi]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)