How to fix shadowing introduced by decodeBasic in v1.3.2?

[ubiuser]

Hi,

I came across an interesting side-effect of the changes introduced in v1.3.2 in the decodeBasic function (93663c4) in the 99designs/gqlgen codebase. The following reproducer runs fine prior the version mentioned before, but fails for the versions since.

Reproducer

package main

import (
	"fmt"
	"github.com/mitchellh/mapstructure"
)

func main() {
	var resp struct {
		Text *string `mapstructure:"text"`
	}

	query := map[string]interface{}{
		"text": "blah",
	}

	if err := wrapCaller(query, &resp); err != nil {
		fmt.Printf("decode failed: %v", err)
		return
	}
	fmt.Printf("3 %#v\n", resp)

	fmt.Printf("%#v", *resp.Text)
}

func wrapCaller(query map[string]interface{}, resp interface{}) error {
	fmt.Printf("1 %#v\n", resp)
	err := caller(query, &resp)
	fmt.Printf("2 %#v\n", resp)
	return err
}

func caller(query map[string]interface{}, resp interface{}) error {
	err := unpack(query, resp)
	return err
}

func unpack(data interface{}, into interface{}) error {
	d, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{
		Result:      into,
		TagName:     "json",
		ErrorUnused: true,
		ZeroFields:  true,
	})
	if err != nil {
		return fmt.Errorf("mapstructure: %w", err)
	}

	return d.Decode(data)
}

Output with v1.3.1

1 &struct { Text *string "mapstructure:\"text\"" }{Text:(*string)(nil)}
2 &struct { Text *string "mapstructure:\"text\"" }{Text:(*string)(0xc0000484c0)}
3 struct { Text *string "mapstructure:\"text\"" }{Text:(*string)(0xc0000484c0)}

Output with v1.3.2

1 &struct { Text *string "mapstructure:\"text\"" }{Text:(*string)(nil)}
2 &struct { Text *string "mapstructure:\"text\"" }{Text:(*string)(0xc0000484e0)}
3 struct { Text *string "mapstructure:\"text\"" }{Text:(*string)(nil)}
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x0 pc=0xeaf741]

The debugger says that resp is shadowed in wrapCaller and the nil value is returned, although one level down in caller everything looks fine. I believe this case is not covered by the unit tests in mapstructure_test.go.

I'm really interested to understand why this is happening, the mechanics in the background and how to solve it.

[jakezhu9]

Hi. I've been trying to fix it lately and this is what I got:

What happened in v1.3.2?

Looking at the decodeBasic function in 93663c4, we can see that when we can't address an element, it makes a copy and replaces the entire value. However, the parameter we pass in is a pointer to a pointer to resp (we do two pointer fetches to resp, one is wrapCaller(query, &resp) in main, and the other is caller( query , &resp) in wrapCaller). We cannot address pointers, so in v1.3.2, we will make a copy, changing the address pointed to by the pointer eventually. But the address of resp in main does not change. That's why we get the correct value in wrapCaller but nil in main. We can verify it by printing the address.
(to make it clear I changed the parameter name resp to r)

func wrapper(query map[string]interface{}, r interface{}) error {
   fmt.Printf("before caller:\n%#v\n", r)
   fmt.Printf("  address of r: %p\n", &r)
   fmt.Printf("  address of resp: %p\n", r)
   err := caller(query, &r)
   fmt.Printf("after caller:\n%#v\n", r)
   fmt.Printf("  address of r: %p\n", &r)
   fmt.Printf("  address of resp: %p\n", r)
   return err
}

output

before caller:
&struct { Text *string "mapstructure:\"text\"" }{Text:(*string)(nil)}
  address of r: 0xc000104d50
  address of resp: 0xc000140028
after caller:
&struct { Text *string "mapstructure:\"text\"" }{Text:(*string)(0xc000104dc0)}
  address of r: 0xc000104d50
  address of resp: 0xc000140050

Why resp is shadowed in wrapCaller?

The issue says that the shadowing is introduced by decodeBasic in v1.3.2. But actually I found that resp is shadowed in all versions. The reason for shadowing is that we try to get the address of resp (err := caller(query, &resp) in wrapCaller). If I change it to err := caller(query, resp), it will not be shadowed. I guess Go has a similar mechanism to decodeBasic in v1.3.2, when we try to get the address of something can't be address, Go will make a copy of that and shadow the old one.

How to fix?

• For your code, you can fix it by simply changing err := caller(query, &resp) to err := caller(query, resp) in wrapCaller. It doesn't cause other errors as you know the resp here is already a pointer. It also fix the shadowing issue.
• For mapstructure, I don't see any problem with it. After all, the parameter you pass to Decode is a pointer (PtrA) to a pointer (PtrB) to resp in main, we can only guarantee that the value pointed to by PtrA is correct, not the resp pointed to by PtrB.

Hope this is helpful to you :)

[StevenACoffman]

So the actual cause of the issue is that a mapstructure no longer works on a pointer to a pointer to a struct, where it used to do so. It's possible that mapstructure might itself detect that when it encounters that case and dereference the outer pointer?