Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
josharian opened this issue Jun 13, 2021 · 9 comments
Closed

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

josharian opened this issue Jun 13, 2021 · 9 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone
Go1.17

Comments

@josharian
Copy link
Contributor

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
@josharian josharian added this to the Go1.17 milestone Jun 13, 2021
@tgulacsi
Copy link
Contributor

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
Copy link
Contributor

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

@OneOfOne
Copy link
Contributor

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

@josharian
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link

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

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 14, 2021
@ianlancetaylor ianlancetaylor mentioned this issue Jun 14, 2021
Closed
@ianlancetaylor
Copy link
Contributor

I filed #46746.

@ianlancetaylor
Copy link
Contributor

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

@josharian
Copy link
Contributor Author

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!).

@golang golang locked and limited conversation to collaborators Jun 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
7 participants
@josharian @cagedmantis @tgulacsi @OneOfOne @ianlancetaylor @gopherbot @renthraysk