`invalid memory address or nil pointer dereference` when accessing UserInfoAddress

[morigs]

Describe the bug When user info is missing address field it's decoded into userinfo struct where Address field has UserInfoAddress type but nil value.

UserInfoAddress type doesn't expect nil as receiver, so code like info.GetAddress().GetCountry() panics.

To Reproduce Steps to reproduce the behavior:

1. Unmarshal userinfo without address
2. Try info.GetAddress().GetCountry()
3. See panic

Expected behavior Probably, returning default values for all fields (e.g. "").

Another (IMO better but breaking) way is to return a pointer. So it will be possible to check value GetAddress() == nil

Additional context Temp workaround:

func isNil(val any) bool {
	return val == nil || (reflect.ValueOf(val).Kind() == reflect.Ptr && reflect.ValueOf(val).IsNil())
}

addr := info.GetAddress()
if !isNil(addr) {
  // ...
}

[fforootd]

This sound really like a bug 😁

Thank you for reporting this.

CC @hifabienne

[livio-a]

my proposal would be to do de nil check in GetAddress:

func (u *userinfo) GetAddress() UserInfoAddress {
	if isNil(u.Address) {
		return nil
	}
	return u.Address
}

so it would return nil, which i believe was the intention at first anyway

@morigs or @adlerhurst is this even breaking? the UserInfoAddress return value is an interface and and thus could already have been nil

[morigs]

Well, it should be breaking. However, using reflection to detect nil here looks a bit strange... It's like embedding workaround right into codebase instead of fixing)

[muhlemmer]

Well, it should be breaking. However, using reflection to detect nil here looks a bit strange... It's like embedding workaround right into codebase instead of fixing)

Any interface type can be compared directly to nil:

func (u *userinfo) GetAddress() UserInfoAddress {
	if u.Address == nil {
		return nil
	}
	return u.Address
}

However, that's kind of pointless, because if u.Address == nil, than a nil was going to be returned anyway. The OP describes the panic in case of chaining.

info.GetAddress().GetCountry()

Here GetCountry() is called on a nil UserInfoAddress

There are multiple solutions to this problem:

1. Make it the responsibility of the caller

One might argue that chaining is not supported by the project and is unsafe. The caller must check if UserInfoAddress is not nil.

For example this would not panic:

var country string

if info := u.GetAddress(); info != nil {
    country = info.GetCountry()
}

The fix would be just a documentation update:

type UserInfo interface {
    ...
    // GetAddress returns nil if UserInfoAddress is not present.
    GetAddress() UserInfoAddress
     ...
}

2. Does it need to be an interface type?

Why not get rid of the UserInfo interface type and rename the private userInfoAddress to UserInfoAddress and return a value of it?

I'm not familiar enough with the project to know if the UserInfoAddress ever changes underlying implementation. That would break.

3. Provide a nilUserInfoAddress implementation

Implement UserInfoAddress with methods that return nothing.

type nilUserInfoAddress struct{}

func (nilUserInfoAddress) GetFormatted() string { return "" }
func (nilUserInfoAddress) GetStreetAddress() string { return "" }
func (nilUserInfoAddress) GetLocality() string { return "" }
func (nilUserInfoAddress) GetRegion() string { return "" }
func (nilUserInfoAddress) GetPostalCode() string { return "" }
func (nilUserInfoAddress) GetCountry() string { return "" }

func (u *userinfo) GetAddress() UserInfoAddress {
	if u.Address == nil {
		return nilUserInfoAddress{}
	}
	return u.Address
}

However, this is a slippery slope. Do you intend to support safe chaining of Get methods that return interface types throughout the project? Similar bugs might be hiding elsewhere, where chaining is allowed. (if not, go for option 1 or 2)

For example, after a quick walk through the go docs I found op.Authorizer whose IDTokenHintVerifier() IDTokenHintVerifier returns an interface type. It would require quite some additions for all those similar cases if you make the choice to support chaining in your project.

4. Make the current userInfoAddress implementation safe

Check if *userInfoAddress is nil in every Get method. I would say this is a quite typical approach, also used in protobuf generated Getters. But again, do you intend to support Get chaining?

func (u *userInfoAddress) GetFormatted() string {
    if u == nil {
        return ""
    }
    return u.Formatted
}

func (u *userInfoAddress) GetStreetAddress() string {
    if u == nil {
        return ""
    }
    return u.StreetAddress
}

// repeat for all methods

[muhlemmer]

Please discard the rant above. I missed the fact that we can just return an empty instance of userInfoAddress in case of nil, with no further complications.

func (u *userinfo) GetAddress() UserInfoAddress {
	if u.Address == nil {
		return new(userInfoAddress)
	}
	return u.Address
}

I will submit a PR in the next minutes.

[fforootd]

Please discard the rant above. I missed the fact that we can just return an empty instance of userInfoAddress in case of nil, with no further complications.

func (u *userinfo) GetAddress() UserInfoAddress {
	if u.Address == nil {
		return new(userInfoAddress)
	}
	return u.Address
}

I will submit a PR in the next minutes.

Haha, no worries. Haven't had time to look into it.

I am certain that @livio-a is Happy to receive a PR

[livio-a]

Hey @muhlemmer I finally found some time to go through all the comments and your PR.

The problem is caused by the fact that checking an interface on nil will only check the interface itself and not the underlying value. Therefore @morigs used a temporary workaround in his code using reflection. The PR therefore does not solve the problem for all cases, e.g. this would still panic:

info := &userinfo{}
var address *userInfoAddress
info.SetAddress(address)
info.GetAddress().GetCountry() //<- panics

While digging into it, I've found the initial reason for the error to occur. The custom Unmarshal function of the userinfo type always sets the userInfoAddress:

func (u *userinfo) UnmarshalJSON(data []byte) error {
	type Alias userinfo
	a := &struct {
		Address *userInfoAddress `json:"address,omitempty"`
		*Alias
		UpdatedAt int64 `json:"update_at,omitempty"`
	}{
		Alias: (*Alias)(u),
	}
	if err := json.Unmarshal(data, &a); err != nil {
		return err
	}
	u.Address = a.Address
	...

A simple nil check on a.Address there would solve at least the problem there and we can then check for nil in GetAddress(). The issue mentioned above will still exist though.

[morigs]

Hi, any plans for fixing this?

[fforootd]

Hi, any plans for fixing this?

Sure, this week @livio-a is in vacation but I am sure he will look into this next week

[muhlemmer]

@livio-a , yes you are right. I've adjusted the tests to use actual json and I was able to reproduce the same panic.

There are two options to fix this:

1. Catch the nil value for a.Address in userinfo.Unmarshal and set it to new instance of userInfoAddress -> see updated PR #207;
2. Don't use pointers for userInfoAddress implementation -> see new PR #211;

Both options work, and might be a matter of taste.