reflect: can set map elem with string key of a different string type #52379
Comments
|
Replacing the kind equality with an assignment check doesn't completely wipe out the gains from the previous change. diff --git a/src/reflect/all_test.go b/src/reflect/all_test.go
index 5a35d98b51..87caca3300 100644
--- a/src/reflect/all_test.go
+++ b/src/reflect/all_test.go
@@ -1496,6 +1496,10 @@ func TestMap(t *testing.T) {
if m != nil {
t.Errorf("mv.Set(nil) failed")
}
+
+ type S string
+ shouldPanic("not assignable", func() { mv.MapIndex(ValueOf(S("key"))) })
+ shouldPanic("not assignable", func() { mv.SetMapIndex(ValueOf(S("key")), ValueOf(0)) })
}
func TestNilMap(t *testing.T) {
diff --git a/src/reflect/value.go b/src/reflect/value.go
index 2496cbe463..5608bcd7ad 100644
--- a/src/reflect/value.go
+++ b/src/reflect/value.go
@@ -1616,7 +1616,7 @@ func (v Value) MapIndex(key Value) Value {
// of unexported fields.
var e unsafe.Pointer
- if key.kind() == String && tt.key.Kind() == String && tt.elem.size <= maxValSize {
+ if key.kind() == String && directlyAssignable(key.typ, tt.key) && tt.elem.size <= maxValSize {
k := *(*string)(key.ptr)
e = mapaccess_faststr(v.typ, v.pointer(), k)
} else {
@@ -2232,7 +2232,7 @@ func (v Value) SetMapIndex(key, elem Value) {
key.mustBeExported()
tt := (*mapType)(unsafe.Pointer(v.typ))
- if key.kind() == String && tt.key.Kind() == String && tt.elem.size <= maxValSize {
+ if key.kind() == String && directlyAssignable(key.typ, tt.key) && tt.elem.size <= maxValSize {
k := *(*string)(key.ptr)
if elem.typ == nil {
mapdelete_faststr(v.typ, v.pointer(), k)I'm happy to send this change unless there are better options. |
|
Change https://go.dev/cl/400635 mentions this issue: |
|
@gopherbot Please open backport to 1.18. The reflect package incorrectly accepts this case. This should be fixed so that we don't have programs that work with the current release and fail with the next release. Earlier releases correctly panic. |
ianlancetaylor
added
NeedsFix
|
Backport issue(s) opened: #52386 (for 1.18). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases. |
|
@kortschak , would you mind preparing a back-port CL to apply to the release-branch.go.1.18 branch? |
|
No worries. Happy to do that. |
|
Change https://go.dev/cl/404000 mentions this issue: |
…pIndex and SetMapIndex name old time/op new time/op delta Map/StringKeys/MapIndex-8 2.36µs ± 5% 2.55µs ±11% +7.98% (p=0.006 n=10+9) Map/StringKeys/SetMapIndex-8 4.86µs ± 7% 4.77µs ± 1% ~ (p=0.211 n=10+9) Map/StringKindKeys/MapIndex-8 2.29µs ± 3% 2.28µs ± 4% ~ (p=0.631 n=10+10) Map/StringKindKeys/SetMapIndex-8 4.44µs ± 3% 4.61µs ± 1% +3.78% (p=0.000 n=10+10) Map/Uint64Keys/MapIndex-8 3.42µs ± 9% 3.11µs ± 2% -9.20% (p=0.000 n=10+9) Map/Uint64Keys/SetMapIndex-8 5.17µs ± 3% 5.00µs ± 1% -3.23% (p=0.000 n=9+10) For #52379 Fixes #52386 Change-Id: I545c71ea3145280828ca4186aad036a6c02016ed Reviewed-on: https://go-review.googlesource.com/c/go/+/400635 Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> Reviewed-by: Joseph Tsai <joetsai@digital-static.net> Reviewed-by: Ian Lance Taylor <iant@google.com> Run-TryBot: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Ian Lance Taylor <iant@google.com> (cherry picked from commit 11a650b) Reviewed-on: https://go-review.googlesource.com/c/go/+/404000 Reviewed-by: David Chase <drchase@google.com>
Original report here.
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
Run https://go.dev/play/p/b7kheLRQpLL on go1.18
What did you expect to see?
Something like this
What did you see instead?
Additional information
The same behaviour not-surprisingly exists for map access (https://go.dev/play/p/3KtoQGNVvgR).
Bisected to
It looks like this change does not include a check for assignability.
The text was updated successfully, but these errors were encountered: