Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Anonymous field pointers #622

Merged
merged 3 commits into from Jul 6, 2022
Merged

fix: Anonymous field pointers #622

merged 3 commits into from Jul 6, 2022

Conversation

stevenh
Copy link
Collaborator

@stevenh stevenh commented Jul 1, 2022

Fix panic using ArgsFlat or ScanStruct on structs with nil Anonymous field pointers.

Also:

  • Remove unused travis-ci link from README.

@stevenh stevenh force-pushed the fix-anonymous-nil branch 8 times, most recently from cd30379 to e1fef94 Compare Jul 1, 2022
redis/reflect_go117.go Outdated Show resolved Hide resolved
@lwaddicor
Copy link
Collaborator

lwaddicor commented Jul 4, 2022

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

package main

import (
	"testing"

	"github.com/gomodule/redigo/redis"
)

func TestRecursiveStructToArgs(t *testing.T) {
	type Outer struct {
		LocationID int
		*Outer
	}

	v := &Outer{
		LocationID: 1,
	}

	args := redis.Args{}.AddFlat(v)
	_ = args
}

=== RUN   TestRecursiveStructToArgs
runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0x140201c03a0 stack=[0x140201c0000, 0x140401c0000]
fatal error: stack overflow

runtime stack:
runtime.throw({0x104f12755?, 0x10502c3e0?})
	/Users/lewisw/sdk/go1.18/src/runtime/panic.go:992 +0x50
runtime.newstack()
	/Users/lewisw/sdk/go1.18/src/runtime/stack.go:1101 +0x46c
runtime.morestack()
	/Users/lewisw/sdk/go1.18/src/runtime/asm_arm64.s:310 +0x70

goroutine 35 [running]:
reflect.name.name({0x104f470bf?})
	/Users/lewisw/sdk/go1.18/src/reflect/type.go:524 +0xbc fp=0x140201c03a0 sp=0x140201c03a0 pc=0x104e8a86c
reflect.(*structType).Field(0x104f61020, 0x0)
	/Users/lewisw/sdk/go1.18/src/reflect/type.go:1248 +0x84 fp=0x140201c0440 sp=0x140201c03a0 pc=0x104e8bf34
reflect.(*rtype).Field(0x0?, 0x0?)
	/Users/lewisw/sdk/go1.18/src/reflect/type.go:973 +0x50 fp=0x140201c0550 sp=0x140201c0440 pc=0x104e8b590
github.com/gomodule/redigo/redis.compileStructSpec({0x104f7b848, 0x104f61020}, 0x104f470c1?, {0x14005f00000, 0x12f67e, 0x142400}, 0x1400015e1a0)
	/Users/lewisw/repos/..../vendor/github.com/gomodule/redigo/redis/scan.go:361 +0x84 fp=0x140201c0700 sp=0x140201c0550 pc=0x104f0df44
github.com/gomodule/redigo/redis.compileStructSpec({0x104f7b848, 0x104f61020}, 0x104f470c1?, {0x14005f00000, 0x12f67d, 0x142400}, 0x1400015e1a0)
	/Users/lewisw/repos/.../vendor/github.com/gomodule/redigo/redis/scan.go:372 +0x21c fp=0x140201c08b0 sp=0x140201c0700 pc=0x104f0e0dc
github.com/gomodule/redigo/redis.compileStructSpec({0x104f7b848, 0x104f61020}, 0x104f470c1?, {0x14005f00000, 0x12f67c, 0x142400}, 0x1400015e1a0)
...

stevenh added 2 commits Jul 4, 2022
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
Copy link
Collaborator Author

stevenh commented Jul 4, 2022

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

I don't believe there's any way to fix that. Even if we added loop detection into compileStructSpec AddFlat doesn't return an error so it can't be handled gracefully, the only benefit would to make it nice error.

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)
+       })
+}

@lwaddicor
Copy link
Collaborator

lwaddicor commented Jul 5, 2022

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

func BenchmarkRecursiveStructToArgs(b *testing.B) {
	v := &struct {
		A int
		B *Ed
		C *Edp
		D *Edp2
	}{1, &Ed{EdI: 2}, &Edp{EdpI: 3}, &Edp2{Edp2I: 4}}

	arg := redis.Args{}
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		arg.AddFlat(v)
	}
}

Current master branch

goos: darwin
goarch: arm64
pkg: github.com/gomodule/redigo/redis
BenchmarkRecursiveStructToArgs
BenchmarkRecursiveStructToArgs-10    	 3419240	       342.1 ns/op
PASS

With the changes on #624 applied

goos: darwin
goarch: arm64
pkg: github.com/gomodule/redigo/redis
BenchmarkRecursiveStructToArgs
BenchmarkRecursiveStructToArgs-10    	 3358435	       341.9 ns/op
PASS

@stevenh
Copy link
Collaborator Author

stevenh commented Jul 5, 2022

While I agree the panic isn't ideal, isn't unexpected behaviour without any indication worse?

What is the developers intent when they embedded?

AddFlat combines the values from the embedded struct into the parent so where we have:

type Level1 struct {
      Name string
      Level2
}

type Level2 struct {
      Description string
}

val := Level1{
      Name: "myname",
      Level2: Level2{
            Description: "mydesc",
      },
}

The result is:

HMSET <key> Name myname Description mydesc

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?

HMSET <key> Name myname1

or

HMSET <key> Name myname2

I would suggest the intent is the latter, but with this protection in place you will actually get the former.

@lwaddicor
Copy link
Collaborator

lwaddicor commented Jul 5, 2022

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.

Recursive structures are not supported and will cause a panic.

Recursive structures will not have repeated elements traversed.

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.
@stevenh stevenh requested a review from lwaddicor Jul 5, 2022
Copy link
Collaborator

@lwaddicor lwaddicor left a comment

Looking good to me

@stevenh stevenh merged commit f1e923c into master Jul 6, 2022
9 checks passed
@stevenh stevenh deleted the fix-anonymous-nil branch Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants