fix: Anonymous field pointers #622
Conversation
stevenh
force-pushed
the
fix-anonymous-nil
branch
8 times, most recently
from
cd30379
to
e1fef94
Compare
|
Nice seems to mostly fix it, all the tests i could find now seem to work perfectly. I was just playing around with edge cases and I found I can get it to blow the stack with this though |
Fix panic using ArgsFlat or ScanStruct on structs with nil Anonymous field pointers. Also: * Remove unused travis-ci link from README. Fixes: #621
Remove the incorrect go1.17 tag so that builds on go versions below v1.18 work as expected.
stevenh
force-pushed
the
fix-anonymous-nil
branch
from
e1fef94
to
fee9988
Compare
I don't believe there's any way to fix that. Even if we added loop detection into I have an implementation of that, see below, but not sure if it's worth the overhead it adds, thoughts? diff --git a/redis/scan.go b/redis/scan.go
index 27809ae..9445ea7 100644
--- a/redis/scan.go
+++ b/redis/scan.go
@@ -355,7 +355,12 @@ func (ss *structSpec) fieldSpec(name []byte) *fieldSpec {
return ss.m[string(name)]
}
-func compileStructSpec(t reflect.Type, depth map[string]int, index []int, ss *structSpec) {
+func compileStructSpec(t reflect.Type, depth map[string]int, index []int, ss *structSpec, seen map[reflect.Type]struct{}) error {
+ if _, ok := seen[t]; ok {
+ return fmt.Errorf("recursive struct definition for %v", t)
+ }
+
+ seen[t] = struct{}{}
LOOP:
for i := 0; i < t.NumField(); i++ {
f := t.Field(i)
@@ -365,11 +370,15 @@ LOOP:
case f.Anonymous:
switch f.Type.Kind() {
case reflect.Struct:
- compileStructSpec(f.Type, depth, append(index, i), ss)
+ if err := compileStructSpec(f.Type, depth, append(index, i), ss, seen); err != nil {
+ return err
+ }
case reflect.Ptr:
// TODO(steve): Protect against infinite recursion.
if f.Type.Elem().Kind() == reflect.Struct {
- compileStructSpec(f.Type.Elem(), depth, append(index, i), ss)
+ if err := compileStructSpec(f.Type.Elem(), depth, append(index, i), ss, seen); err != nil {
+ return err
+ }
}
}
default:
@@ -428,6 +437,8 @@ LOOP:
}
}
}
+
+ return nil
}
var (
@@ -435,25 +446,27 @@ var (
structSpecCache = make(map[reflect.Type]*structSpec)
)
-func structSpecForType(t reflect.Type) *structSpec {
+func structSpecForType(t reflect.Type) (*structSpec, error) {
structSpecMutex.RLock()
ss, found := structSpecCache[t]
structSpecMutex.RUnlock()
if found {
- return ss
+ return ss, nil
}
structSpecMutex.Lock()
defer structSpecMutex.Unlock()
ss, found = structSpecCache[t]
if found {
- return ss
+ return ss, nil
}
ss = &structSpec{m: make(map[string]*fieldSpec)}
- compileStructSpec(t, make(map[string]int), nil, ss)
+ if err := compileStructSpec(t, make(map[string]int), nil, ss, make(map[reflect.Type]struct{})); err != nil {
+ return nil, fmt.Errorf("compile struct: %s: %w", t, err)
+ }
structSpecCache[t] = ss
- return ss
+ return ss, nil
}
var errScanStructValue = errors.New("redigo.ScanStruct: value must be non-nil pointer to a struct")
@@ -489,7 +502,11 @@ func ScanStruct(src []interface{}, dest interface{}) error {
return errors.New("redigo.ScanStruct: number of values not a multiple of 2")
}
- ss := structSpecForType(d.Type())
+ ss, err := structSpecForType(d.Type())
+ if err != nil {
+ return fmt.Errorf("redigo.ScanStruct: %w", err)
+ }
+
for i := 0; i < len(src); i += 2 {
s := src[i+1]
if s == nil {
@@ -558,7 +575,11 @@ func ScanSlice(src []interface{}, dest interface{}, fieldNames ...string) error
return nil
}
- ss := structSpecForType(t)
+ ss, err := structSpecForType(t)
+ if err != nil {
+ return fmt.Errorf("redigo.ScanSlice: %w", err)
+ }
+
fss := ss.l
if len(fieldNames) > 0 {
fss = make([]*fieldSpec, len(fieldNames))
@@ -621,6 +642,7 @@ func (args Args) Add(value ...interface{}) Args {
// for more information on the use of the 'redis' field tag.
//
// Other types are appended to args as is.
+// panics if v includes a recursive anonymous struct.
func (args Args) AddFlat(v interface{}) Args {
rv := reflect.ValueOf(v)
switch rv.Kind() {
@@ -649,7 +671,11 @@ func (args Args) AddFlat(v interface{}) Args {
}
func flattenStruct(args Args, v reflect.Value) Args {
- ss := structSpecForType(v.Type())
+ ss, err := structSpecForType(v.Type())
+ if err != nil {
+ panic(fmt.Errorf("redigo.AddFlat: %w", err))
+ }
+
for _, fs := range ss.l {
fv, err := fieldByIndexErr(v, fs.index)
if err != nil {
diff --git a/redis/scan_test.go b/redis/scan_test.go
index 6154171..bf97ead 100644
--- a/redis/scan_test.go
+++ b/redis/scan_test.go
@@ -763,3 +763,18 @@ func ExampleArgs() {
// {Title:Example Author:Gary Body:Hello}
// {Title:Example2 Author:Steve Body:Map}
}
+
+func TestRecursiveStructToArgs(t *testing.T) {
+ type Outer struct {
+ LocationID int
+ *Outer
+ }
+
+ v := &Outer{
+ LocationID: 1,
+ }
+
+ require.Panics(t, func() {
+ redis.Args{}.AddFlat(v)
+ })
+} |
|
I think that it should be ok to just let the application carry on regardless. My gut feel is that being able to trigger any sort of panic is not ideal If we are actually recursing into an object we have already seen then we can just ignore it, since its a duplicate of the one we have already i don't think it makes sense even if we could parse it I've raised a pr into this branch with a slight change from your one above plus some additional tests. What do you think? #624 For performance i've been playing around with the one below. Running on both the branch linked, and current master (had to define Edp2 on that). Performance effects look negligible, if any. I think that cache may be helping us here Current master branch With the changes on #624 applied |
|
While I agree the panic isn't ideal, isn't unexpected behaviour without any indication worse? What is the developers intent when they embedded?
type Level1 struct {
Name string
Level2
}
type Level2 struct {
Description string
}
val := Level1{
Name: "myname",
Level2: Level2{
Description: "mydesc",
},
}The result is: For the recursive case this makes no sense as the lower levels would just overwrite the higher ones. type Level1 struct {
Name string
*Level1
}
val := Level1{
Name: "myname1",
Level1: &Level1{
Name: "myname2",
},
}What is the expected result? or I would suggest the intent is the latter, but with this protection in place you will actually get the former. |
|
Yeah good point. I think either way is probably fine as long as there is information about the behaviour it does (maybe to addflat? so it shows in the godoc). I'd still probably tend towards not panicking generally, but given these are impossible to properly support, it could be fine I guess the two options for that are something like these.
I'm easy either way |
Catch the case where anonymous struct recursion and prevent it. In the case of ScanStruct an error will be returned, in the case of ArgsFlat it will panic with a nice error.
Fix panic using
ArgsFlatorScanStructon structs withnilAnonymous field pointers.Also: