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

Panic when serializing null pointers #31

Closed
lombare opened this issue Aug 24, 2021 · 2 comments · Fixed by #34
Closed

Panic when serializing null pointers #31

lombare opened this issue Aug 24, 2021 · 2 comments · Fixed by #34

Comments

@lombare
Copy link

lombare commented Aug 24, 2021

Hello I think there is an issue in the serialization process because when we serialize this kind of struct :

type Struct struct {
    Test map[string]*bool  `json:"test" groups:"test"`
}

Via :

data := Struct{
    Test: map[string]*bool{
        "a": &true // replace this by a function that makes a ptr out of a boolean
        "b": nil
     }
}

opts := sheriff.Options{Groups: string[]{"test"}}
jsonStr, err := sheriff.Marshal(&o, data); // <-- panics here
// error management here

Panic occurs here at sheriff.go line 222 :

220:	if k == reflect.Ptr {
221: 		v = v.Elem()
222:		val = v.Interface()
223:		k = v.Kind()
224:	}

It looks like when the value is a Ptr, sheriff tries to dereference it and then get it's value, but if the Ptr is nil then it dereferences a nil.

Tell me if I'm wrong but a safe fix for this could be :

if k == reflect.Ptr {
	if v.IsNil() {
		return v.Interface()
	}
    	v = v.Elem()
	val = v.Interface()
	k = v.Kind()
}
@mweibel
Copy link
Collaborator

mweibel commented Aug 24, 2021

without testing your change, this looks more or less right (use val instead of using v.Interface()). Feel free to open a PR with the change, ensure to add a test validating the fix works :)

@mweibel
Copy link
Collaborator

mweibel commented Aug 30, 2021

fixed via #32

@mweibel mweibel closed this as completed Aug 30, 2021
simaotwx added a commit to simaotwx/sheriff that referenced this issue Sep 1, 2021
Maps and slices that are nil are empty and valid.
The change introduced by liip#32 which fixes liip#31 has changed how
nil slices and maps are marshaled (`null` instead of `[]`).

Fix this regression by removing maps and slices from the check.
simaotwx added a commit to simaotwx/sheriff that referenced this issue Sep 1, 2021
Maps and slices that are nil are empty and valid.
The change introduced by liip#32 which fixes liip#31 has changed how
nil slices and maps are marshaled (`null` instead of `[]`).

Fix this regression by removing maps and slices from the check.
simaotwx added a commit to simaotwx/sheriff that referenced this issue Sep 1, 2021
Slices that are nil are empty and valid.
The change introduced by liip#32 which fixes liip#31 has changed how
nil slices are marshaled (`null` instead of `[]`).

Fix this regression by removing slices from the check.
mweibel pushed a commit that referenced this issue Sep 1, 2021
Slices that are nil are empty and valid.
The change introduced by #32 which fixes #31 has changed how
nil slices are marshaled (`null` instead of `[]`).

Fix this regression by removing slices from the check.
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 a pull request may close this issue.

2 participants