Skip to content

Commit

Permalink
reflect: keep RO flags unchanged in Value.Addr
Browse files Browse the repository at this point in the history
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 keeping 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 #32772.

Change-Id: I79e086628c0fb6569a50ce63f3b95916f997eda1
GitHub-Last-Rev: 78e280e
GitHub-Pull-Request: #32787
Reviewed-on: https://go-review.googlesource.com/c/go/+/183937
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
  • Loading branch information
kezhuw authored and mdempsky committed May 4, 2020
1 parent a1ffbe9 commit 4c003f6
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 2 deletions.
37 changes: 36 additions & 1 deletion src/reflect/all_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ func TestCanSetField(t *testing.T) {
}

type testCase struct {
// -1 means Addr().Elem() of current value
index []int
canSet bool
}
Expand All @@ -360,35 +361,65 @@ func TestCanSetField(t *testing.T) {
val: ValueOf(&S1{}),
cases: []testCase{
{[]int{0}, false},
{[]int{0, -1}, false},
{[]int{0, 0}, false},
{[]int{0, 0, -1}, false},
{[]int{0, -1, 0}, false},
{[]int{0, -1, 0, -1}, false},
{[]int{0, 1}, true},
{[]int{0, 1, -1}, true},
{[]int{0, -1, 1}, true},
{[]int{0, -1, 1, -1}, true},
{[]int{1}, false},
{[]int{1, -1}, false},
{[]int{2}, true},
{[]int{2, -1}, true},
},
}, {
val: ValueOf(&S2{embed: &embed{}}),
cases: []testCase{
{[]int{0}, false},
{[]int{0, -1}, false},
{[]int{0, 0}, false},
{[]int{0, 0, -1}, false},
{[]int{0, -1, 0}, false},
{[]int{0, -1, 0, -1}, false},
{[]int{0, 1}, true},
{[]int{0, 1, -1}, true},
{[]int{0, -1, 1}, true},
{[]int{0, -1, 1, -1}, true},
{[]int{1}, false},
{[]int{2}, true},
},
}, {
val: ValueOf(&S3{}),
cases: []testCase{
{[]int{0}, true},
{[]int{0, -1}, true},
{[]int{0, 0}, false},
{[]int{0, 0, -1}, false},
{[]int{0, -1, 0}, false},
{[]int{0, -1, 0, -1}, false},
{[]int{0, 1}, true},
{[]int{0, 1, -1}, true},
{[]int{0, -1, 1}, true},
{[]int{0, -1, 1, -1}, true},
{[]int{1}, false},
{[]int{2}, true},
},
}, {
val: ValueOf(&S4{Embed: &Embed{}}),
cases: []testCase{
{[]int{0}, true},
{[]int{0, -1}, true},
{[]int{0, 0}, false},
{[]int{0, 0, -1}, false},
{[]int{0, -1, 0}, false},
{[]int{0, -1, 0, -1}, false},
{[]int{0, 1}, true},
{[]int{0, 1, -1}, true},
{[]int{0, -1, 1}, true},
{[]int{0, -1, 1, -1}, true},
{[]int{1}, false},
{[]int{2}, true},
},
Expand All @@ -402,7 +433,11 @@ func TestCanSetField(t *testing.T) {
if f.Kind() == Ptr {
f = f.Elem()
}
f = f.Field(i)
if i == -1 {
f = f.Addr().Elem()
} else {
f = f.Field(i)
}
}
if got := f.CanSet(); got != tc.canSet {
t.Errorf("CanSet() = %v, want %v", got, tc.canSet)
Expand Down
5 changes: 4 additions & 1 deletion src/reflect/value.go
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,10 @@ func (v Value) Addr() Value {
if v.flag&flagAddr == 0 {
panic("reflect.Value.Addr of unaddressable value")
}
return Value{v.typ.ptrTo(), v.ptr, v.flag.ro() | flag(Ptr)}
// Preserve flagRO instead of using v.flag.ro() so that
// v.Addr().Elem() is equivalent to v (#32772)
fl := v.flag & flagRO
return Value{v.typ.ptrTo(), v.ptr, fl | flag(Ptr)}
}

// Bool returns v's underlying value.
Expand Down

0 comments on commit 4c003f6

Please sign in to comment.