MultipartDeserialize: guard against nil part on malformed body#69
Conversation
mime/multipart.Reader.NextPart can return a nil *Part alongside a non-EOF error when the body is not valid MIME multipart data (plain text, JSON, truncated multipart, mismatched boundary, empty body, ...). The previous loop treated any non-EOF error as "just break" and then dereferenced part.Header, so every NF that calls MultipartDeserialize (SMF on POST /nsmf-pdusession/v1/sm-contexts, UDM, AUSF) panicked on a crafted request and returned HTTP 500. Treat non-EOF errors from NextPart and non-EOF errors from part.Read as real failures and return them wrapped. Also fix the inverted check on part.Read: the code was returning from the loop when err == nil, which dropped every successfully read part; swap to the correct `err != nil` branch (ignoring io.EOF, which just marks end-of-part). Add a regression test that feeds three malformed bodies (plain text, empty, JSON) through MultipartDeserialize and asserts no panic plus an error. Pre-fix the plain-text case panicked on part.Header.Get. Fixes free5gc/free5gc#1026. Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Hardens multipart/related deserialization to avoid panics and correctly surface errors when the request body is malformed (e.g., declared multipart but actually plain text/JSON/empty), addressing free5gc/free5gc#1026.
Changes:
- Update
MultipartDeserializeto handleNextPart()EOF vs non-EOF errors correctly and explicitly reject unexpected nil parts. - Fix inverted
part.Readerror handling so successful reads don’t abort the loop. - Add a regression test ensuring malformed bodies return an error without panicking.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| client.go | Improves multipart parsing error handling to prevent nil deref panics and propagate malformed-body errors. |
| client_test.go | Adds coverage for malformed multipart bodies to ensure no panic and an error is returned. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| n, err = part.Read(multipartBody) | ||
| if err == nil { | ||
| return err | ||
| // Read returns err == io.EOF when the part is fully consumed; any |
There was a problem hiding this comment.
part.Read(multipartBody) reads only once into a fixed 1400-byte buffer. If a multipart part exceeds 1400 bytes, Read can return n==len(buf) with err==nil, and the code will proceed with truncated data (e.g., incomplete JSON or binary). Consider reading the full part (e.g., io.ReadAll / io.ReadAll(io.LimitReader(...)) or a loop until io.EOF) so valid requests aren’t corrupted or rejected.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@Alonza0314 LGTM |
Summary
mime/multipart.Reader.NextPartcan return a nil*Partalongside a non-EOF error when the body declaresmultipart/relatedbut is not valid MIME multipart data — plain text, JSON, empty body, truncated part, or a mismatched boundary. The previousMultipartDeserializeloop treated any non-EOF error fromNextPartas a silent "break and fall through", then dereferencedpart.Header.Get("Content-Type")on line 611 and panicked.Every NF using this helper (SMF on
POST /nsmf-pdusession/v1/sm-contexts, UDM, AUSF, ...) returned HTTP 500 on a single crafted request, and Gin's recovery middleware was the only thing preventing the whole process from dying.Fix
NextPartbranch so EOF breaks cleanly, any other error is wrapped and returned, and an unexpected nil part is rejected explicitly.part.Read: the old code returned whenerr == nil, which dropped every successfully read part; replace witherr != nil && err != io.EOFso only real failures abort the loop.TestMultipartDeserialize_MalformedBodycovering plain-text / empty / JSON bodies. Pre-fix the plain-text case panicked onpart.Header.Get; post-fix all three return an error with no panic.Test
Fixes free5gc/free5gc#1026.
Signed-off-by: SAY-5 SAY-5@users.noreply.github.com