Fix panic due to metadata being nil #4719
Conversation
|
LGTM Update: Missed your todo. |
| @@ -83,7 +83,7 @@ func (b *backend) pathLoginUpdate(ctx context.Context, req *logical.Request, dat | |||
| return logical.ErrorResponse("invalid role ID"), nil | |||
| } | |||
|
|
|||
| var metadata map[string]string | |||
| metadata := make(map[string]string) | |||
There was a problem hiding this comment.
In the secret id case, we unilaterally assign to metadata, which means we should still add a nil map check before writing in the role name in case that ever is (or might become with future revisions) nil.
There was a problem hiding this comment.
Secret IDs constructor is initializing the map. So, it is unlikely that metadata is nil. But still, I've added a nil check before assigning the role_name.
There was a problem hiding this comment.
Now that we have the nil check below, allocating the map isn't strictly necessary but I'm good either way :)
| @@ -262,7 +262,9 @@ func (b *backend) pathLoginUpdate(ctx context.Context, req *logical.Request, dat | |||
| } | |||
|
|
|||
| // Always include the role name, for later filtering | |||
| metadata["role_name"] = role.name | |||
| if metadata != nil { | |||
There was a problem hiding this comment.
This makes the comment not actually be correct. If metadata is nil, you don't actually end up with a map with role_name. So either this is best effort, or if it's nil you need to create a map with role_name in it.
| @@ -83,7 +83,7 @@ func (b *backend) pathLoginUpdate(ctx context.Context, req *logical.Request, dat | |||
| return logical.ErrorResponse("invalid role ID"), nil | |||
| } | |||
|
|
|||
| var metadata map[string]string | |||
| metadata := make(map[string]string) | |||
There was a problem hiding this comment.
Now that we have the nil check below, allocating the map isn't strictly necessary but I'm good either way :)
|
@calvn I think its a good observation. There really is no reason to have an unnecessary allocation in a hot path. Removed it. |
|
@vishalnayak It's not unnecessary. I'd leave it up above. If it's already allocated the below check will skip, but it provides a measure of safety in case the intervening code modifies it (say, from the bound cidr code) without checking to see if it's nil or not. |
Fixes #4716