openai / openai/openai-cli

Multipart encoder can panic on typed nil io.Reader pointers

Open Beginner friendly
#89 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
697
Forks
60
Avg merge
2d 21h
Merged PRs (30d)
71

Description

Summary

internal/apiform detects io.Reader implementations before it applies the encoder's existing nil-pointer handling. A typed nil pointer whose type implements io.Reader can therefore be routed into io.Copy, which invokes Read on a nil receiver and can panic.

The encoder already defines nil pointers as empty form fields, so reader detection currently bypasses an intended invariant.

Reproduction

On current main (d082a010f7c6cacf407d8a1581446a7857f9f1bb), a minimal reader type demonstrates the problem:

type panicReader struct{}

func (*panicReader) Read([]byte) (int, error) {
    panic("Read called on nil receiver")
}

var reader *panicReader

var buf bytes.Buffer
writer := multipart.NewWriter(&buf)
_ = Marshal(map[string]any{"file": reader}, writer)

Because *panicReader implements io.Reader, encodeValue checks:

if t.Implements(reflect.TypeOf((*io.Reader)(nil)).Elem()) {
    return e.encodeReader(key, val, writer)
}

before reaching the pointer branch:

case reflect.Pointer:
    if val.IsNil() || !val.IsValid() {
        return writer.WriteField(key, "")
    }

The nil pointer is therefore converted to an io.Reader interface and passed to io.Copy, which calls its Read method.

Expected behavior

A typed nil pointer should follow the same nil-pointer semantics regardless of whether its type happens to implement io.Reader: encode an empty field and do not invoke methods on the nil receiver.

Suggested fix

Check nil pointer/interface values before reader-interface detection. Keep reader handling unchanged for non-nil values.

Add a regression using a typed nil reader whose Read method panics if invoked, and verify multipart encoding completes without a panic and emits an empty field.

Impact

This is robustness and request-serialization correctness. A legitimate typed nil value can currently turn request construction into a process panic instead of being handled by the encoder's existing null-value path.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in internal/apiform at encodeValue and compare its nil-pointer handling with reader detection and encodeReader. Add a regression using the typed nil panicReader from the reproduction, then verify multipart encoding completes without invoking Read and emits an empty field.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
api
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
84/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.