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

proposal: Go 2: reflect: remove Value.Bytes #27727

Open
bcmills opened this issue Sep 18, 2018 · 9 comments
Open

proposal: Go 2: reflect: remove Value.Bytes #27727

bcmills opened this issue Sep 18, 2018 · 9 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 18, 2018

Background

As noted in #24746 (comment), (reflect.Value).Bytes is the only reflect.Value methods that return mutable values without enforcing CanSet or CanInterface.

That makes dangerous mutations through []byte (and slices of defined types with byte as the underlying type) more difficult to detect, since they do not involve unsafe or syscall in the way that other dangerous mutations must.

This is a proposal to close that mutability loophole and make the handling of byte slice types more consistent with other slices.

Proposal

This proposal consists of two parts:

  1. Remove Value.Bytes, making it accessible only to Go 1 code.
  2. Relax reflect.Copy, reflect.Append, and reflect.AppendSlice to allow the source argument (but not the destination) to be obtained through unexported fields, provided that the element type of the slice does not contain any pointers or interfaces.
    • Today, those functions panic if any argument was obtained through unexported fields.

Users could replace a call to Bytes on an exported field of known element type with a call to Interface and a type-assertion (https://play.golang.org/p/9LJIDW7ftZx).

To examine slices obtained via unexported fields (and slices of defined types with byte or as their underlying type), users could employ any of four strategies:

Note that the latter three are already possible today: part (2) of this proposal only affects the clarity of the code, not what is possible to express.

Alternatives

We could keep the Bytes method, but make it panic if CanInterface returns false.

  • That would allow more existing code to continue to compile, but at the risk of introducing a run-time error. I prefer to make it a complie-time error in order to prompt the user to consider whether to introduce a run-time check, make a copy, or use an unsafe operation.

We could go even further and remove SetBytes, since it is redundant with Copy.

  • That would make the treatment of []byte more like every other slice type.
  • Since that method doesn't introduce any loopholes in the usual mutability rules, removing it does not seem worth the code churn.

If we had read-only slices (#22876 and associated issues), we could make Bytes return a read-only slice.

  • In contrast, this proposal does not rely on any new language features.
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 16, 2018

The current Bytes and Runes method are a general type safety hole, since they permit changing the value of an unexported field. We should do this.

@go101
Copy link

@go101 go101 commented Mar 4, 2019

Is there a Value.Runes method?

@bcmills
Copy link
Member Author

@bcmills bcmills commented Mar 4, 2019

@go101 apparently not! 😅

I was just riffing on #24746, but I'm not sure why @dsnet mentioned it there...

@bcmills bcmills changed the title proposal: Go 2: reflect: remove Value.Bytes and Value.Runes proposal: Go 2: reflect: remove Value.Bytes Mar 4, 2019
@bcmills
Copy link
Member Author

@bcmills bcmills commented Mar 4, 2019

Updated proposal to remove references to the nonexistent Runes method.

@go101
Copy link

@go101 go101 commented Mar 4, 2019

To avoid breaking compatibility, maybe let the Bytes method return a deep copy of bytes is better.

@go101
Copy link

@go101 go101 commented Mar 4, 2019

Aha, sorry, then it still breaks the compatibility.

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Feb 11, 2020

Remove Value.Bytes, making it accessible only to Go 1 code.

How would that work, given that it's always possible to access the Bytes method via reflection? Is it possible for reflect code to know whether it's being called by Go 1 or Go 2 code?

Relax reflect.Copy, reflect.Append, and reflect.AppendSlice to allow the source argument (but not the destination) to be obtained through unexported fields, provided that the element type of the slice does not contain any pointers or interfaces.

I suppose that Append and AppendSlice would also have to slightly alter semantics if the source was obtained through an unexported field - it would always need to copy the result, rather than using extra capacity when available.

Users could replace a call to Bytes on an exported field of known element type with a call to Interface and a type-assertion (https://play.golang.org/p/9LJIDW7ftZx).

That won't work if the slice is a named type with underlying type []byte which is currently allowed by Bytes. (https://play.golang.org/p/QbdhYJMZ_8u)

The workaround is possible, but a bit more awkward and with some performance implications. https://play.golang.org/p/JWXNzh238Ju

@bcmills
Copy link
Member Author

@bcmills bcmills commented Feb 11, 2020

How would that work, given that it's always possible to access the Bytes method via reflection? Is it possible for reflect code to know whether it's being called by Go 1 or Go 2 code?

Good question! That could happen at the point where the Value is packed into an interface: if that point is in Go 1 code, then the concrete type indicated in the interface value would include the Bytes method, and otherwise it would not.

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Feb 11, 2020

That could happen at the point where the Value is packed into an interface: if that point is in Go 1 code, then the concrete type indicated in the interface value would include the Bytes method, and otherwise it would not.

I'm not sure I can see how that could work. AFAICS it means that reflect.TypeOf(reflect.Value{}) would have different values throughout the system, breaking one of the fundamental reflect invariants.
Also:

  • the Value might never be explicitly packed into an interface. https://play.golang.org/p/9USK58xRZah
  • the Value is actually packed into an interface inside the reflect package (during the return from the Interface method), and that's always inside Go 2 code, so the heuristic would have to be more specific than that, I think. You could special-case reflect.Interface, but that wouldn't cover the fact that the Interface method could be assigned to function value.

I suspect that in this particular case, doing a Go 1/2 split is quite hard.

I wonder: would it be so bad if Bytes made a copy of the slice when the value isn't Interface-able, perhaps combined with a runtime warning that could be enabled to flag when this happens?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants