Skip to content

Proxy: Fix nil pointer panic in BuildStreamURL and resource leak in ParseBody#4659

Open
sevico wants to merge 2 commits into
ossrs:developfrom
sevico:fix/utils-nil-pointer-and-resource-leak
Open

Proxy: Fix nil pointer panic in BuildStreamURL and resource leak in ParseBody#4659
sevico wants to merge 2 commits into
ossrs:developfrom
sevico:fix/utils-nil-pointer-and-resource-leak

Conversation

@sevico

@sevico sevico commented Apr 11, 2026

Copy link
Copy Markdown

Problem

Two bugs in internal/utils/utils.go:

1. Nil pointer panic in BuildStreamURL

net.ParseIP() returns nil for non-IP hostnames (e.g., "example.com"). The code calls ip.To4() without checking if ip is nil first, which causes a panic crash.

// Before: panics when hostname is not an IP address
if ip := net.ParseIP(u.Hostname()); ip.To4() != nil {

// After: safe nil check
if ip := net.ParseIP(u.Hostname()); ip != nil && ip.To4() != nil {

2. Resource leak in ParseBody

defer r.Close() is placed after the ReadAll error check. If ReadAll fails, the function returns early without the defer being registered, so r is never closed.

// Before: r.Close() not reached on ReadAll error
func ParseBody(r io.ReadCloser, v interface{}) error {
    b, err := ioutil.ReadAll(r)
    if err != nil {
        return errors.Wrapf(err, "read body") // r is never closed
    }
    defer r.Close()

// After: r always closed
func ParseBody(r io.ReadCloser, v interface{}) error {
    defer r.Close()
    b, err := ioutil.ReadAll(r)

Impact

  • BuildStreamURL panics when the stream URL contains a domain name instead of an IP address.
  • ParseBody leaks HTTP request body on read errors.

@winlinvip winlinvip added the EnglishNative This issue is conveyed exclusively in English. label Apr 11, 2026
@winlinvip winlinvip force-pushed the develop branch 3 times, most recently from 6ce415e to 6ee6f1c Compare May 17, 2026 16:09
…arseBody

1. BuildStreamURL: net.ParseIP() returns nil for non-IP hostnames
   (e.g., "example.com"), then calling nil.To4() panics. Add nil
   check before calling To4().

2. ParseBody: defer r.Close() is placed after the ReadAll error
   check. If ReadAll fails, the function returns early without
   closing r, causing a resource leak. Move defer to the top of
   the function.
@suzp1984 suzp1984 force-pushed the fix/utils-nil-pointer-and-resource-leak branch from b9d182c to e124f9f Compare May 19, 2026 03:19
@suzp1984

Copy link
Copy Markdown
Contributor

I rebased your branch to develop branch.
And this commit make sense.

Add TestParseBody_CloseCalledOnReadError to verify r.Close() is called
even when ReadAll fails (resource leak fix).

Enhance TestBuildStreamURL with:
- Comment explaining the example.com case would panic before the nil
  pointer fix (net.ParseIP returns nil for non-IP hostnames)
- IPv6 test cases to verify ip.To4() check works correctly
- Clarifying comments for each test case category
@suzp1984

Copy link
Copy Markdown
Contributor

One more thing, I added UT to cover this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

EnglishNative This issue is conveyed exclusively in English.

Development

Successfully merging this pull request may close these issues.

3 participants