reflect: inconsistent handling of embed/sticky read-only flag

[ianlancetaylor]

In the following program, the first call to v.Close() works fine. The second panics saying

panic: reflect: reflect.Value.Close using value obtained using unexported field

The difference is that one value is reached via an embedded unexported field, and the other is reached via a normal unexported field. It makes sense for the handling of a field of an unexported embedded struct to be treated as exported, but in an example like this the special handling is carrying through a map index operation. That does not make sense, and does not correspond to how the language behaves.

CC @mpvl

package main

import (
	"reflect"
)

type s1 struct { C chan int }
type S2 struct { s1 }
type m map[int]S2
type S3 struct { m }

type S4 struct { f m }

func main() {
	// This works.
	s3 := S3{map[int]S2{0: S2{s1{make(chan int)}}}}
	v := reflect.ValueOf(s3).Field(0).MapIndex(reflect.ValueOf(0)).Field(0).Field(0)
	v.Close()

	// This panics.
	s4 := S4{map[int]S2{0: S2{s1{make(chan int)}}}}
	v = reflect.ValueOf(s4).Field(0).MapIndex(reflect.ValueOf(0)).Field(0).Field(0)
	v.Close()
}

[mdempsky]

/cc @dsnet

Reminds me of #21122 et al.

Edit: Nevermind, appears unrelated.

[mdempsky]

I think the issue has to do with the distinction between flagEmbedRO and flagStickyRO and reflect.Value.MapIndex.

When we access an unexported non-embedded field, we set flagStickyRO because it's never valid to perform any mutation operations on the derived values. This flag is then always preserved.

When we access an unexported embedded field though, we set flagEmbedRO because if we later access an exported field or method, then we assume that's a promoted field. (Aside: this technically seems to subvert the security model because it allows accessing shadowed fields that couldn't normally be accessed via Go code.)

The issue is when we call MapIndex, we simply pass through these RO flags into the resulting reflect.Value. When we later access the exported C field, that erroneously clears away the flagEmbedRO field, but leaves the flagStickyRO in place.

I think operations like MapIndex need to promote flagEmbedRO to flagStickyRO and both calls to Close should panic.

[ianlancetaylor]

Yes, that is exactly the problem. Various places in reflect/value.go need to change from fl = v.flag&flagRO to

if v.flag&flagRO != 0 {
    fl = flagStickyRO
}

[mdempsky]

Yeah, I think Field and Elem are the only operations that should preserve the flagRO bits as is. Everything else should promote flagEmbedRO to flagStickyRO.

[gopherbot]

Change https://golang.org/cl/66331 mentions this issue: reflect: fix mutability of non-exported embedded fields