Skip to content

Panic binding numeric values into non-byte slice destinations (reachable via nullable slice query params) #155

Description

@mromaszewicz

BindStringToObject panics with reflect: call of reflect.Value.OverflowInt on slice Value whenever the destination is a non-[]byte slice and the value happens to parse as an integer. Found during an adversarial review of the #153 implementation; it is unrelated to that change and reproduces on main and every release since v1.2.0.

Minimal reproduction

var s []string
runtime.BindStringToObject("123", &s)
// panic: reflect: call of reflect.Value.OverflowInt on slice Value

Note the asymmetry that kept this hidden: BindStringToObject("abc", &s) returns a normal error (strconv.ParseInt: parsing "abc": invalid syntax). Only values that parse as integers reach the panic.

Reachable from generated code on the request path

A nullable slice query parameter (output-options: nullable-type: true) using the default query serialization (form + explode) panics on any request whose value is numeric:

var dest nullable.Nullable[[]string]
err := runtime.BindQueryParameter("form", true, true, "p",
    url.Values{"p": {"123"}}, &dest)
// panic: reflect: call of reflect.Value.OverflowInt on slice Value

i.e. ?p=123 panics while ?p=abc returns a binding error. Under net/http the server recovers the handler goroutine and drops the connection, and most frameworks' recovery middleware turns it into a 500 — so this is a per-request availability/noise problem rather than a process crash, but a request-crafted panic on the binding path is still worth fixing promptly.

Mechanism

bindstring.go:

case reflect.Slice:
    if opts.Format == "byte" && isByteSlice(t) {
        // base64 path, returns
    }
    // Non-binary slices fall through to the default error case.  <-- comment is wrong
    fallthrough
case reflect.Int, reflect.Int8, ...:
    val, err = strconv.ParseInt(src, 10, 64)
    if err == nil {
        if v.OverflowInt(val) {   // <-- panics: v is a slice

The fallthrough (introduced in 224825a, PR #98, first released in v1.2.0) lands in the integer case, not the default error case the comment claims. When ParseInt fails, the error return masks the problem; when it succeeds, v.OverflowInt panics because v is a slice reflect.Value.

The exploded-query route reaches it because BindQueryParameterWithOptions's primitive default: branch calls BindStringToObject for any kind that isn't Slice/Struct — including the bool-keyed map that nullable.Nullable[T] is — and the nullable-wrapper recursion in bindstring.go then binds "123" into a fresh []string.

Suggested fix

Replace the fallthrough with an explicit jump to the unhandled-type error, e.g.:

case reflect.Slice:
    if opts.Format == "byte" && isByteSlice(t) {
        ...
        return nil
    }
    err = fmt.Errorf("can not bind to destination of type: %s", t.Kind())

That restores the pre-v1.2.0 behavior (a clean error) for both the numeric and non-numeric cases, and makes the existing comment true. A regression test should pin both "123" and "abc" against []string, plus the nullable.Nullable[[]string] exploded-query route.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions