reflect: Bytes allows conversion of invariant slices

[dsnet]

(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]

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]

A little related: #23536

[bcmills]

Very related: #19778.

[gopherbot]

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

[ianlancetaylor]

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]

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]

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]

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

[bcmills]

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]

Yep, it's a risky method all around.

[dotaheor]

@bcmills

..., without even importing unsafe.

This is why this hole is so useful.

[dsnet]

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]

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]

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

[bcmills]

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]

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]

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]

@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]

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]

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]

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

Done: #27727.