Repository navigation
FIX: Free exception messages and the reload file name - #5
Merged
Merged
Conversation
fiftyoneDegreesExceptionGetMessage allocates its message with the library's Malloc and the caller must free it. The twelve places that turn an exception into a Go error copied the message with C.GoString and never freed it, so every failed call leaked one message. 1,000 calls of MatchEvidence with freed evidence left 195,000 bytes unfreed. messageError copies a message into an error with errors.New and then frees it with fiftyoneDegreesFree, which partners the Malloc that allocated it. fiftyoneDegreesFree is a function pointer, which Go cannot call, so a static C function in the cgo preamble calls it. The status message in InitManagerFromFile uses the same helper. It was freed with MemoryStandardFree, which only matches Malloc while the library's memory tracking is off. errors.New replaces fmt.Errorf at all thirteen places because a message is not a format string. A data file called missing-%d.hash was reported as missing-%!d(MISSING).hash, and go vet reports the pattern once the go version in go.mod is 1.24 or later. TestMessageError covers this.
ReloadFromFile copied the file name into C memory with C.CString and never freed it. 1,000 reloads with a 65,617 character path grew the process's private bytes by 66,797,568 bytes, and by 675,840 bytes with the copy freed.
allocatedDuring runs a call with the library's memory tracking switched on and returns the bytes still allocated afterwards. TestMessageFreed uses it on InitManagerFromFile with a missing file, once through the exception path and once through the status path. Before the fix the exception path left 218 bytes. The status path left 193 bytes because its message was freed with MemoryStandardFree, which the tracker does not see, rather than the library's Free.
This was referenced Nov 2, 2026
ReloadFromFile and ReloadFromOriginalFile take main's version, from the reload fix, which already frees the exception message and the file name and also reports a failed status.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Carried over from device-detection-go#102 by James Rosewell, a draft with no review yet, while device-detection-go is being merged into this repository. The branch is brought in as is, with its original commits, so the changes are under
device-detection/.Closes #3
Problem
fiftyoneDegreesExceptionGetMessageallocates its message with the library'sMallocand the caller must free it. Twelve places inddcopied the message into a Go error withC.GoStringand never freed it, so every failed call leaked one message. The status message inInitManagerFromFilewas freed withMemoryStandardFreerather than theFreethat partnersMalloc. All thirteen passed the message tofmt.Errorfas its format string, so a%in a data file path was garbled.ReloadFromFilealso copied its file name withC.CStringand never freed the copy. #3 has the details and a link to each place.Change
dd/exception.goaddsmessageError, which copies a message the C library allocated into an error witherrors.Newand then frees it. Go cannot call a C function pointer, so the free goes throughfreeMessage, a static C function in the cgo preamble that callsfiftyoneDegreesFree.dd/config_hash.go,dd/resource_hash.goanddd/results_hash.gocallmessageErrorat the twelve exception sites and the status site.config_hash.gono longer importsfmt.ReloadFromFilefrees its copy of the file name.dd/memory.goaddsallocatedDuring, which runs a call with the library's memory tracking switched on and returns the bytes still allocated afterwards.dd/exception_test.goaddsTestMessageError, which checks that the message for a missing file calledmissing-%d.hashholds the path unchanged, andTestMessageFreed, which checks that a failedInitManagerFromFileleaves nothing allocated through the exception path (LowMemory) and through the status path (InMemory).Both tests use only a missing file. Like every test in
dd, they need the Lite data file thatTestMainlooks for.Verification
Windows 11, Go 1.25.1, MSYS2 UCRT64 gcc 16.2.0,
CGO_ENABLED=1, with51Degrees-LiteV4.1.hashat the repository root and inassets/, whereTestMainand theonpremisetests look for it. "Before" is 100f55d and "after" is 4b5857d.go build -p 2 ./...go vet -p 2 ./dd ./onpremisego vet -p 2 ./dd ./onpremisewith go.mod temporarily set togo 1.24go test -p 2 -count=1 ./ddgo test -p 2 -count=1 ./onpremisego test -p 2 -count=1 -v -run 'TestMessageError|TestMessageFreed' ./ddTestMessageErrorfails withmissing-%!d(MISSING).hash,TestMessageFreed/exceptionleaves 218 bytes,TestMessageFreed/statusleaves 193 bytesInitManagerFromFilecalls, missing file,LowMemoryMatchEvidencecalls with freed evidenceReloadFromFilecalls with a 65,617 character pathFor the "before" run of the two tests, the four production files were reset to 100f55d and the tests were kept. In the status case the old code did free the message, but with
MemoryStandardFree, which the library's tracking does not see, so the tracker still counted its 193 bytes.The last three rows come from local checks that are not committed. The first two wrap
fiftyoneDegreesSetUpMemoryTrackingandfiftyoneDegreesUnsetMemoryTrackingaround the calls. The third reads the process's private bytes throughGetProcessMemoryInfo, so it only runs on Windows.go vet ./...fails at 100f55d on four unkeyed struct literals inexamples/uach/uach_test.go, which this change does not touch, so vet was run on the two library packages.CI
The shared build and test workflow only picks up pull requests that are not drafts, so the build and test matrix has not run on this pull request. The UTM link lint is the only check that runs on a draft.
What remains
C.CStringallocates with C'smalloc, which the library's tracking does not see, and Go has no portable way to read the C heap.ReloadFromFileandReloadFromOriginalFilediscard the status code that the C reload returns, so a failed reload reports success. That is not changed here. It is ReloadFromFile and ReloadFromOriginalFile report success when a reload fails #4, fixed in FIX: Return the status of a failed reload as an error #6, which is stacked on this one.