database/sql: possible new panic doing reflect-based conversions

[josharian]

Go 1.17 adds conversions from []T to *[N]T, including via the reflect package. Such conversions may panic if N is larger than the slice length. Whether reflection-based conversion is possible without panicking is thus now no longer a function only of the types involved. As a result, even if v.Type().ConvertibleTo(w.Type()) returns true, v.Convert(w.Type()) may panic. This is documented in package reflect.

database/sql/convert.go contains code that appears to rely on that now-broken invariant:

func convertAssignRows(dest, src interface{}, rows *Rows) error {
	// ...
	if dv.Kind() == sv.Kind() && sv.Type().ConvertibleTo(dv.Type()) {
		dv.Set(sv.Convert(dv.Type()))
		return nil
	}
	// ...
}

(text/template has similar code, but it is guarded with a check that the types are int-like.)

This sv.Convert call couldn't panic in 1.16; now it can.

What should we do here?

• (a) recover from the panic and return an error saying the conversion failed
• (b) panic
• (c) recover from the panic and continue as if the types weren't convertible

(a) seems like the clearest error output. (b) is simple. It also matches the handling in the standard library of reflect.Type.Comparable: In a few places, we check reflect.Type.Comparable and then blindly use ==, even though that can panic. (c) most closely preserves the 1.16 behavior.

I lean towards (a) but would like to discuss. And if we do (a) or (c), should we give reflect.Type.Comparable/== the same treatment?

Marking as a 1.17 release-blocker, pending a decision about severity and how we want to proceed.

cc @dsnet @ianlancetaylor @mdempsky @OneOfOne

[tgulacsi]

Why does v.Type().ConvertibleTo(w.Type()) return true?
Because of the language change ([]T is convertible to *[N]T). But that spec also says that some conversions will panic.
So this can panic.
database/sql handles panics at some places, this should be another one. (Eliminate choice (b))
Those types are convertible, only the length of the slice causes error -which is a common error- so this sould return an error (sth. like io.ErrShortBuffer, or a new reflect.ErrShortSlice ?).

TL;DR: My 2c for (a)

[ianlancetaylor]

Perhaps we should add in package reflect func (Value) ConvertibleTo(Type) bool and func (Value) Comparable(Value) bool.

[OneOfOne]

I agree with @tgulacsi, the package already handles reflect panics, so I'd vote for a.

[josharian]

@ianlancetaylor that's a great idea. We aren't going to be the only ones having to figure out how to work around this.

Is there time yet to do that for 1.17? Does it need to go through the proposal process?

[josharian]

Note that adding Value-based ConvertibleTo and Comparable will effectively implement choice (c) above, which is in conflict with what appears to be a broad preference for choice (a).

[renthraysk]

Aren't the Kinds different (array & slice) so that sv.Convert() never gets called?

[ianlancetaylor]

I filed #46746.

[ianlancetaylor]

That said I agree with @renthraysk that I don't see a problem with the current code due to the explicit Kind check.

[josharian]

That's embarassing; I combed through all the code above it and failed to read the very code I copy/pasted. Thanks, @renthraysk. Everything remaining to do here is in #46746 (thanks, Ian!).