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: Value.Addr().Elem() loses flagEmbedRO in origin value #32772

Open
kezhuw opened this issue Jun 25, 2019 · 3 comments

Comments

Projects
None yet
3 participants
@kezhuw
Copy link

commented Jun 25, 2019

This makes exported fields in unexported anonymous field not settable

What version of Go are you using (go version)?

$ go version
go version go1.12.6 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12.6/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12.6/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/b6/81wsy1517sxdpkd09pwj98_w0000gn/T/go-build799570696=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://play.golang.org/p/mKRa1-3j2He

package main

import (
	"fmt"
	"reflect"
)

type embed struct{ X int }

type S1 struct {
	embed
}

func printCanSet(value reflect.Value, index []int) {
	current := value
	for _, i := range index {
		if i == -1 {
		        // Addr() decays flagRO, which is a combination of flagEmbedRO and flagStickyRO, to flagStickyRO which is propagated to
			// all nested fields, thus making exported fields in unexported anonymous field not settable.
			//
			// See for implementation details:
			// * https://github.com/golang/go/blob/master/src/reflect/value.go#L83
			// * https://github.com/golang/go/blob/master/src/reflect/value.go#L256
			// * https://github.com/golang/go/blob/master/src/reflect/value.go#L829
			current = current.Addr().Elem()
		} else {
			current = current.Field(i)
		}
	}
	fmt.Printf("Value %+v lookup index %+v: CanSet ? %t\n", value, index, current.CanSet())
}

func main() {
	value := reflect.ValueOf(&S1{}).Elem()
	printCanSet(value, []int{0, 0})
	printCanSet(value, []int{0, -1, 0})
}

What did you expect to see?

The second should prints true for CanSet.

What did you see instead?

The second prints false for CanSet.

See also

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

I don't really see how to fix this safely: ensuring that the Addr can not be used in some way to set the unexported fields. If you think that your changes are safe, do you want to send a pull request? Thanks.

@ianlancetaylor ianlancetaylor added this to the Go1.14 milestone Jun 25, 2019

@kezhuw

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

@ianlancetaylor I thought after your response, I think it is safe for reasons:

  • Result of Value.Addr() is not CanAddr and hence not CanSet, so the field itself can't be modified later, this does not interfere with flagRO.
  • Addr().Elem() get flagRO same as origin value, so it should behave same as before in CanSet.
  • Value.Field() inherits flagStickyRO but not flagEmbedRO from wrapped value, so
    • if the value itself is flagEmbedRO, Value.Field()'s flagRO depends only on that field but not its wrapped value;
    • if the value itself is flagStickyRO, all its fields are unexported.

I have update commits with more test cases in trial fix branch to (kezhuw/go@306e9c9 kezhuw/go@a16093d), I plan to send a pull request later after job (at least 10 hours late) if above sounds good to you.

See Value.Field().

kezhuw added a commit to kezhuw/go that referenced this issue Jun 26, 2019

reflect: Inherits all bits of flagRO in Value.Addr()
Currently, `Value.Addr()` collapses `flagRO`, which is a combination of
`flagEmbedRO` and `flagStickyRO`, to `flagStickyRO`. This causes exported
fields of unexported anonymous field from `Value.Addr().Elem()` read only.

This commit fix this by inheriting all bits of `flagRO` from origin
value in `Value.Addr()`. This should be safe due to following reasons:
* Result of `Value.Addr()` is not `CanSet()` because of it is not `CanAddr()`,
  but not `flagRO`.
* `Addr().Elem()` get same `flagRO` as origin, so it should behave same as
  origin in `CanSet()`.

Fix golang#32772.

kezhuw added a commit to kezhuw/go that referenced this issue Jun 26, 2019

reflect: inherit all bits of flagRO in Value.Addr()
Currently, Value.Addr() collapses flagRO, which is a combination of
flagEmbedRO and flagStickyRO, to flagStickyRO. This causes exported
fields of unexported anonymous field from Value.Addr().Elem() read only.

This commit fix this by inheriting all bits of flagRO from origin
value in Value.Addr(). This should be safe due to following reasons:
* Result of Value.Addr() is not CanSet() because of it is not CanAddr()
  but not flagRO.
* Addr().Elem() get same flagRO as origin, so it should behave same as
  origin in CanSet().

Fixes golang#32772.

@kezhuw kezhuw referenced a pull request that will close this issue Jun 26, 2019

Open

reflect: keep RO flags unchanged in Value.Addr #32787

@gopherbot

This comment has been minimized.

Copy link

commented Jun 26, 2019

Change https://golang.org/cl/183937 mentions this issue: Fix lost flagEmbedRO in reflect.Value.Addr().Elem()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.