Skip to content

Commit

Permalink
Fix panic in sort.go (#39)
Browse files Browse the repository at this point in the history
Avoid using reflect.Value.Interface to de-duplicate keys since
some keys may have source from an unexported field,
causing the logic to panic. Instead, just rely on the isLess function,
which is already safe to use on unexported values.

Fixes #38
  • Loading branch information
dsnet authored Sep 1, 2017
1 parent 8099a97 commit d5735f7
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 6 deletions.
2 changes: 1 addition & 1 deletion cmp/internal/value/sort.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func SortKeys(vs []reflect.Value) []reflect.Value {
// Deduplicate keys (fails for NaNs).
vs2 := vs[:1]
for _, v := range vs[1:] {
if v.Interface() != vs2[len(vs2)-1].Interface() {
if isLess(vs2[len(vs2)-1], v) {
vs2 = append(vs2, v)
}
}
Expand Down
17 changes: 12 additions & 5 deletions cmp/internal/value/sort_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,15 +132,22 @@ func TestSortKeys(t *testing.T) {
complex(math.NaN(), math.NaN()): true,
},
want: []interface{}{
math.NaN(), math.NaN(), math.NaN(), math.NaN(),
complex(math.NaN(), math.NaN()), complex(math.NaN(), math.NaN()),
complex(math.NaN(), 0), complex(math.NaN(), 0), complex(math.NaN(), 0), complex(math.NaN(), 0),
complex(0, math.NaN()), complex(0, math.NaN()), complex(0, math.NaN()), complex(0, math.NaN()),
math.NaN(),
complex(math.NaN(), math.NaN()),
complex(math.NaN(), 0),
complex(0, math.NaN()),
},
}}

for i, tt := range tests {
keys := append(reflect.ValueOf(tt.in).MapKeys(), reflect.ValueOf(tt.in).MapKeys()...)
// Intentionally pass the map via an unexported field to detect panics.
// Unfortunately, we cannot actually test the keys without using unsafe.
v := reflect.ValueOf(struct{ x map[interface{}]bool }{tt.in}).Field(0)
value.SortKeys(append(v.MapKeys(), v.MapKeys()...))

// Try again, with keys that have read-write access in reflect.
v = reflect.ValueOf(tt.in)
keys := append(v.MapKeys(), v.MapKeys()...)
var got []interface{}
for _, k := range value.SortKeys(keys) {
got = append(got, k.Interface())
Expand Down

0 comments on commit d5735f7

Please sign in to comment.