Skip to content
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

crypto/x509: potential memory leak on macOS #51752

Closed
tie opened this issue Mar 17, 2022 · 4 comments
Closed

crypto/x509: potential memory leak on macOS #51752

tie opened this issue Mar 17, 2022 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Darwin

Comments

@tie
Copy link
Contributor

tie commented Mar 17, 2022 •
edited

What version of Go are you using (go version)?

$ go version
go version go1.18rc1 darwin/amd64

Does this issue reproduce with the latest release?

Not sure.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

GO111MODULE=""

GOARCH="amd64"

GOBIN=""

GOCACHE="/Users/tie/Library/Caches/go-build"

GOENV="/Users/tie/Library/Application Support/go/env"

GOEXE=""

GOEXPERIMENT=""

GOFLAGS="-trimpath"

GOHOSTARCH="amd64"

GOHOSTOS="darwin"

GOINSECURE=""

GOMODCACHE="/Users/tie/go/pkg/mod"

GONOPROXY=""

GONOSUMDB=""

GOOS="darwin"

GOPATH="/Users/tie/go"

GOPRIVATE=""

GOPROXY="https://proxy.golang.org,direct"

GOROOT="/Users/tie/sdk/go1.18rc1"

GOSUMDB="sum.golang.org"

GOTMPDIR=""

GOTOOLDIR="/Users/tie/sdk/go1.18rc1/pkg/tool/darwin_amd64"

GOVCS=""

GOVERSION="go1.18rc1"

GCCGO="gccgo"

GOAMD64="v1"

AR="ar"

CC="clang"

CXX="clang++"

CGO_ENABLED="0"

GOMOD="/Users/tie/darwin/darwin/go.mod"

GOWORK="/Users/tie/darwin/go.work"

CGO_CFLAGS="-g -O2"

CGO_CPPFLAGS=""

CGO_CXXFLAGS="-g -O2"

CGO_FFLAGS="-g -O2"

CGO_LDFLAGS="-g -O2"

PKG_CONFIG="pkg-config"

GOGCCFLAGS="-fPIC -arch x86_64 -m64 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/7l/18dhcjwx2hd24rh8_ccyh7s40000gn/T/go-build687169505=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I’m writing bindings for a subset of macOS framework APIs, and while looking up Core Foundation examples, I’ve noticed suspicious CFMutableArray usage in Go.

On macOS, crypto/x509 uses Core Foundation types to interact with Security framework. In particular, it uses CFArrayCreateMutable with CFArrayCallBacks set to zero (i.e. NULL). Apple docs state that a NULL value is identical to release/retain callbacks set to NULL.

go/src/crypto/x509/internal/macos/corefoundation.go

Lines 158 to 161 in d17b65f

func CFArrayCreateMutable() CFRef {
ret := syscall(abi.FuncPCABI0(x509_CFArrayCreateMutable_trampoline), kCFAllocatorDefault, 0, 0 /* kCFTypeArrayCallBacks */, 0, 0, 0)
return CFRef(ret)
}

CFArrayCreateMutable is used in (*x509.Certificate).systemVerify, where a leaf certificate allocated with SecCertificateCreateWithData is appended and never released explicitly.

go/src/crypto/x509/root_darwin.go

Lines 13 to 16 in d17b65f

certs := macOS.CFArrayCreateMutable()
defer macOS.ReleaseCFArray(certs)
leaf := macOS.SecCertificateCreateWithData(c.Raw)
macOS.CFArrayAppendValue(certs, leaf)

What did you expect to see?

kCFTypeArrayCallBacks argument and CFRelease call.

What did you see instead?

I’d assume that the memory is never released, but I haven’t tested this yet.
@seankhliao seankhliao added OS-Darwin NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 17, 2022
@seankhliao
Copy link
Member

cc @golang/security

@tie
Copy link
Contributor Author

tie commented Mar 17, 2022

Sorry for bothering, I’ve run the code with debugger and now I see that ReleaseCFArray releases all array elements.

@tie tie closed this as completed Mar 17, 2022
@tie
Copy link
Contributor Author

tie commented Mar 17, 2022 •
edited

On a second thought, it’s strange that I can change the code as follows—as far as I’m aware, CFRelease panics if refcount is zero.

leaf := macOS.SecCertificateCreateWithData(c.Raw)
defer macOS.CFRelease(leaf) // NB does not panic

certs := macOS.CFArrayCreateMutable()
defer macOS.ReleaseCFArray(certs)
macOS.CFArrayAppendValue(certs, leaf)

@tie tie reopened this Mar 17, 2022
@tie
Copy link
Contributor Author

tie commented Mar 17, 2022

Hm, that’s not detected as a memory leak by leaks, so probably just an API quirk. Closing the issue.

@tie tie closed this as completed Mar 17, 2022
@golang golang locked and limited conversation to collaborators Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Darwin
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gopherbot @seankhliao @tie