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.

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

reflect: Bytes allows conversion of invariant slices #24746

Closed
dsnet opened this issue Apr 7, 2018 · 21 comments
Closed

reflect: Bytes allows conversion of invariant slices #24746

dsnet opened this issue Apr 7, 2018 · 21 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Apr 7, 2018

(This is a very minor issue), but consider the following:

type MyByte uint8
type Bytes1 = []byte
type Bytes2 []byte
type Bytes3 []MyByte

According to the Go specification, Bytes1 and Bytes2 are convertible to each other, but Bytes3 is not convertible to any of the other since the underlying element type differs (even if byte and MyByte are convertible to each other). In other words Bytes3 is not convertible to a []byte.

The Value.Bytes is a convenience method that retrieves the []byte if convertible to such. However, this is possible:

reflect.ValueOf(Bytes3{1, 2, 3}).Bytes()

This allows us to use reflect to convert Bytes3 to Bytes1, when we couldn't do so in the language.

This applies to Value.Bytes, Value.Runes, Value.SetBytes, and Value.SetRunes.

@dotaheor
Copy link

dotaheor commented Apr 7, 2018

cool finding.

package main

import "fmt"
import "reflect"

type MyByte uint8
type Bytes1 = []byte
type Bytes2 []byte
type Bytes3 []MyByte

func main() {
	var b3 = Bytes3{1, 2, 3}
	b1 := reflect.ValueOf(b3).Bytes()
	b1[1] = 9
	fmt.Println(b3) // [1 9 3]
}

@dotaheor
Copy link

dotaheor commented Apr 7, 2018

A little related: #23536

@bcmills
Copy link
Contributor

bcmills commented Apr 7, 2018

Very related: #19778.

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 9, 2018
@bcmills bcmills added this to the Go1.11 milestone Apr 9, 2018
@gopherbot
Copy link

Change https://golang.org/cl/105937 mentions this issue: reflect: prevent conversion of invariant slices

@ianlancetaylor
Copy link
Contributor

I'm not sure this is a real problem. It would be a minor problem if the reflect package let you convert a reflect.Value from Bytes3 to Bytes1. But (as far as I know) it doesn't. It's just that the Bytes method words on anything whose underlying value looks like a []byte, or, to put it another way, the reflect package lets you pull a []byte out of something whose type is not []byte. That is a type error, but it's not a very interesting one, as it's a type error when crossing the boundary from a reflect.Value to a real type. We should consider deprecating this method for Go 2, but there isn't much point to making the method a duplicate of v.Interface().([]byte).

@dsnet
Copy link
Member Author

dsnet commented Apr 9, 2018

That is a type error, but it's not a very interesting one, as it's a type error when crossing the boundary from a reflect.Value to a real type.

Interesting or not, usages of reflect start relying on behavior like this. I came upon this because I was wondering if I was allowed to do this for something else I was working on. Even though reflect allows this today, I decided to avoid relying on this "feature" since it's not something the language allows.

As an example of bad usages, the json package relies on this and some users of json implicitly rely on this without realizing it.

@ianlancetaylor
Copy link
Contributor

Sure, I'm not surprised that people rely on it. What I'm trying to say is that I think it's OK to permit this kind of type violation. It's not the kind of type violation that the reflect package is not supposed to permit.

@dotaheor
Copy link

The hole doesn't do bad, pls don't fix it. :)

@bcmills
Copy link
Contributor

bcmills commented Apr 10, 2018

One surprising side-effect it is that it allows the program to bypass the usual restrictions on setting unexported fields (and fields of unexported types), without even importing unsafe.

https://play.golang.org/p/7MwAH2x6Uto

@ianlancetaylor
Copy link
Contributor

Yep, it's a risky method all around.

@dotaheor
Copy link

@bcmills

..., without even importing unsafe.

This is why this hole is so useful.

@dsnet
Copy link
Member Author

dsnet commented Apr 20, 2018

This is why this hole is so useful.

This type of sentiment is why this hole is dangerous. Needing to import unsafe is supposed to signal that you are deliberately sidestepping the type system.

@dotaheor
Copy link

dotaheor commented Apr 22, 2018

I don't see any dangerousness here.
Could you show an example to prove it is dangerous?

I don't think Go type system forbids this conversion for security reason.

@dotaheor
Copy link

And if it is dangerous, the proposal, #19778, should have been rejected already now.

@bcmills
Copy link
Contributor

bcmills commented Apr 23, 2018

The problem here is arguably not the conversion at all. Rather, it is that (reflect.Value).Bytes is the only reflect.Value method that returns a mutable value without enforcing CanSet.

If you try to write the equivalent code by copying the slice value out to a different variable, you get panic: reflect: reflect.Value.Set using value obtained using unexported field (https://play.golang.org/p/-jlmAYDY4IE ). A similar panic occurs with reflect.Copy (https://play.golang.org/p/V7n2EXkRn6Y).

#19778 is not relevant to this problem because ordinary Go code enforces unexported fields and types at compile-time instead of run-time. (It is already impossible to write these examples using ordinary Go code, because outside packages cannot name the types and/or fields involved.)

@bcmills
Copy link
Contributor

bcmills commented Apr 23, 2018

Could you show an example to prove it is dangerous?

See the numerous Playground examples above. It's not “dangerous” in the “undefined behavior” sense, but rather in the “allows a program to violate API restrictions” sense.

@bcmills
Copy link
Contributor

bcmills commented Apr 23, 2018

Read-only slices (#22876) could address this problem: they would allow the Bytes method to return a read-only slice instead of a read-write one.

Programs that need to obtain a read-write slice can already do so today, subject to the usual restrictions on unexported fields and types, by calling (reflect.Value).Interface and type-asserting the result. (And, of course, programs today can also use the (reflect.Value).SetBytes method to set the raw bytes directly.)

@dotaheor
Copy link

dotaheor commented Apr 24, 2018

@bcmills
#19778 has nothing related to unexported fields.
As in the 3rd comment you have made, it is very related to this issue.

Agree with you that your play examples are not "dangerous",
and yes, it “allows a program to violate API restrictions”.
But the violation doesn't do any harm.
On the contrary, it has many benefits.
For example, now we can NOT use the functions in bytes standard packages for []MyByte values,
but we can with the help of this violation.

@dotaheor
Copy link

fixed missing a NOT in the last comment.

And I don't think this violation will affect the possible read-only slice feature later.

@ianlancetaylor
Copy link
Contributor

I'm going to close this because I really don't think there is anything to be done.

I suggest that you write a Go 2 proposal to remove the reflect.Value.Bytes method.

@bcmills
Copy link
Contributor

bcmills commented Sep 18, 2018

I suggest that you write a Go 2 proposal to remove the reflect.Value.Bytes method.

Done: #27727.

@golang golang locked and limited conversation to collaborators Sep 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants