diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..e85aa0f --- /dev/null +++ b/PR.md @@ -0,0 +1,108 @@ +# Fix panic when a custom type function returns nil for a pointer field + +## Problem + +A `DecodeCustomTypeFunc` registered against a pointer type is a natural way +to say "there is no value, leave this pointer nil" by returning `nil, nil` +for an empty input (the `time.Time`-style workaround for "unset" is not +available for a custom pointer type). Doing that panics: + +```go +type CustomTime *time.Time + +type TestError struct { + CT CustomTime +} + +decoder := form.NewDecoder() +decoder.RegisterCustomTypeFunc(func(s []string) (interface{}, error) { + if s[0] == "" { + return nil, nil + } + parsed, err := time.Parse(time.RFC3339, s[0]) + if err != nil { + return nil, err + } + return CustomTime(&parsed), nil +}, CustomTime(nil)) + +var test TestError +decoder.Decode(&test, url.Values{"CT": {""}}) +``` + +``` +panic: reflect: call of reflect.Value.Set on zero Value +``` + +## Root cause + +`decoder.go:196-204`, inside the custom-type-function branch of +`setFieldByType`: + +```go +val, err := cf(arr[idx:]) +if err != nil { + d.setError(namespace, err) + return +} + +v.Set(reflect.ValueOf(val)) +``` + +When `cf` returns a nil `interface{}`, `reflect.ValueOf(val)` is the invalid +zero `reflect.Value`, and `Set` panics on it. + +This exact bug was fixed once in #68, but that fix went through the generic +`case reflect.Ptr:` recursion path instead of guarding the nil case, which +discarded the custom function's own return value for a pointer type whenever +it returned something other than nil. That regressed the case where the +custom function itself returns a fully populated pointer (`&User{...}`), so +#68 was reverted in #69, and the original nil-pointer panic has been back on +master since. + +## Fix + +Guard against a nil `val` before calling `Set`, right where the value is +produced, instead of special-casing `reflect.Ptr` after the fact. A nil +return leaves the field at its existing zero value, everything else is set +exactly as before: + +```go +if val == nil { + return +} + +v.Set(reflect.ValueOf(val)) +``` + +## Verification + +`TestDecoder_CustomTypeFuncPointerNilValue` in `decoder_test.go` covers both +sides: a custom function for `*User` that returns `nil, nil` for an empty +value (must not panic, field stays nil), and the same function returning a +populated `*User` (must still decode correctly - this is the case #68 broke). + +Reverting only the `setFieldByType` guard (test left in place) panics the +test binary itself, not just a failed assertion: + +``` +--- FAIL: TestDecoder_CustomTypeFuncPointerNilValue +panic: reflect: call of reflect.Value.Set on zero Value [recovered] + panic: reflect: call of reflect.Value.Set on zero Value +... + .../decoder.go:202 +0x35f + .../decoder.go:177 +0x1f0 + .../form_decoder.go:174 +0x225 +``` + +`go build ./...`, `go vet ./...` and `gofmt -l decoder.go decoder_test.go` +are clean. `go test ./... -count=1` and `go test ./... -count=1 -race`: 48 +tests, all passing, including the new one. + +## Flags for the account owner + +- `.github/CONTRIBUTING.md` only asks for tests covering the change; no + CLA/DCO step. +- No behaviour change for any case that already worked: a nil-returning + custom function on a pointer field previously always panicked, so there is + no existing caller relying on that panic. diff --git a/decoder.go b/decoder.go index d746ea0..a912e0c 100644 --- a/decoder.go +++ b/decoder.go @@ -199,6 +199,14 @@ func (d *decoder) setFieldByType(current reflect.Value, namespace []byte, idx in return } + // A custom function returning a nil interface has no value to + // set, eg. to signal "leave this field at its zero value" for + // an empty input; reflect.ValueOf(nil) is the invalid zero + // Value and Set would panic on it. + if val == nil { + return + } + v.Set(reflect.ValueOf(val)) set = true return diff --git a/decoder_test.go b/decoder_test.go index 2b631a0..3424c70 100644 --- a/decoder_test.go +++ b/decoder_test.go @@ -1881,6 +1881,37 @@ func TestDecoder_RegisterCustomTypeFunc(t *testing.T) { Equal(t, v.Slice, []customString{"customv1", "customv2"}) } +func TestDecoder_CustomTypeFuncPointerNilValue(t *testing.T) { + type User struct { + Name string + } + + type TestStruct struct { + U *User `form:"u"` + } + + d := NewDecoder() + d.RegisterCustomTypeFunc(func(vals []string) (interface{}, error) { + if vals[0] == "" { + // Signals "leave the field at its zero value" for an absent input, + // this must not panic even though the field itself is a pointer. + return nil, nil + } + return &User{Name: vals[0]}, nil + }, &User{}) + + var empty TestStruct + err := d.Decode(&empty, url.Values{"u": {""}}) + Equal(t, err, nil) + Equal(t, empty.U, nil) + + var populated TestStruct + err = d.Decode(&populated, url.Values{"u": {"bob"}}) + Equal(t, err, nil) + NotEqual(t, populated.U, nil) + Equal(t, populated.U.Name, "bob") +} + func TestDecoder_EmptyArrayString(t *testing.T) { type T1 struct { F1 string `form:"F1"`