dualfroz
8a7336f727
A DecodeCustomTypeFunc returning nil to mean "leave this pointer field unset" panicked in reflect.Value.Set, since reflect.ValueOf(nil) is the invalid zero Value. Guard against a nil return before Set. This is the panic from #68/#69: the earlier fix routed pointer fields through the generic Ptr recursion instead of guarding nil, which discarded a custom function's own populated pointer result and had to be reverted. Guarding val == nil directly fixes the panic without touching the case that broke.
109 lines
3.2 KiB
Markdown
109 lines
3.2 KiB
Markdown
# 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.
|