Skip to content

x/net: possible DEREF_AFTER_NULL error in http2/server.go #68679

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
ognevlive opened this issue Jul 31, 2024 · 5 comments
Closed

x/net: possible DEREF_AFTER_NULL error in http2/server.go #68679

ognevlive opened this issue Jul 31, 2024 · 5 comments
Labels
WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
Unreleased

Comments

@ognevlive
Copy link

Go version

go version go1.22.1 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/ognev/.cache/go-build'
GOENV='/home/ognev/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/ognev/go/pkg/mod'
GOOS='linux'
GOPATH='/home/ognev/go'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.1'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build75870582=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Hello! I have run SAST tool (https://svace.pages.ispras.ru/svace-website/en/) against x/net package codebase and found one minor null dereferce error (https://cwe.mitre.org/data/definitions/476.html).
Description: After having been compared to a nil value at server.go:2557, pointer 'b.conn' is passed as implicit 'this' parameter in call to function 'http2.serverConn.noteBodyReadFromHandler' at server.go:2560, where it is dereferenced at server.go:2434.

What did you see happen?

https://github.com/golang/net/blob/master/http2/server.go#L2560

func (b *requestBody) Read(p []byte) (n int, err error) {
    if b.needsContinue {
        b.needsContinue = false
        b.conn.write100ContinueHeaders(b.stream) // (3) also could be null dereference
    }
    if b.pipe == nil || b.sawEOF {
        return 0, io.EOF
    }
    n, err = b.pipe.Read(p)
    if err == io.EOF {
        b.sawEOF = true
    }
    if b.conn == nil && inTests {  (1) // inTests always False 
        return
    }
    b.conn.noteBodyReadFromHandler(b.stream, n, err) (2) // if b is null then this expression lead to panic
    return
}

What did you expect to see?

Possible version of fixed code:

func (b *requestBody) Read(p []byte) (n int, err error) {
    if b.needsContinue {
        b.needsContinue = false
        if b.conn != nil {
            b.conn.write100ContinueHeaders(b.stream)
        }
    }
    if b.pipe == nil || b.sawEOF {
        return 0, io.EOF
    }
    n, err = b.pipe.Read(p)
    if err == io.EOF {
        b.sawEOF = true
    }
    if b.conn == nil {
        if inTests {
            return
        }
        return 0, fmt.Errorf("connection is nil")
    }
    b.conn.noteBodyReadFromHandler(b.stream, n, err)
    return
}
@gopherbot gopherbot added this to the Unreleased milestone Jul 31, 2024
@gabyhelp
Copy link

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@ianlancetaylor
Copy link
Member

How could b.conn ever be nil at that point? The only case I see would seem to be in the test that was added in https://go.dev/cl/31636, which constructs a requestBody in such a way that b.conn can be nil. If b.conn is nil in some other way, then something has gone wrong. I don't think there is anything to do here.

@ianlancetaylor ianlancetaylor added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jul 31, 2024
@ognevlive
Copy link
Author

Thank you for your response.
It's hard for me to validate this error, and because of that, I came for your help.
I stay on the position that it's better to inform developers.
If you think that this situation is impossible, then we can close this issue.

@ianlancetaylor
Copy link
Member

Thanks, but we should not contort our code to satisfy analysis tools that make incorrect reports.

@ianlancetaylor ianlancetaylor closed this as not planned Won't fix, can't repro, duplicate, stale Aug 1, 2024
@alexeyerm

This comment was marked as off-topic.

Sign in to view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@ianlancetaylor @gopherbot @ognevlive @alexeyerm @gabyhelp