1168010 - leak in updater.cpp GetManifestContents

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P2)

Description

[Robert Strong (they/them - no direct email)]

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#3607

free is never called on mbuf

Comment 1

[Robert Strong (they/them - no direct email)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ded40585e08f&exclusion_profile=false

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ded40585e08f/try-linux64-asan/try_ubuntu64-asan_vm_test-mochitest-other-bm117-tests1-linux64-build2.txt.gz

19:56:20     INFO -  ==5817==ERROR: LeakSanitizer: detected memory leaks
19:56:20     INFO -  Direct leak of 145 byte(s) in 1 object(s) allocated from:
19:56:20     INFO -      #0 0x471501 in __interceptor_malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
19:56:20     INFO -      #1 0x49595f in GetManifestContents(char const*) /builds/slave/try-l64-asan-00000000000000000/build/src/toolkit/mozapps/update/updater/updater.cpp:3633
19:56:20     INFO -      #2 0x4920c1 in DoUpdate /builds/slave/try-l64-asan-00000000000000000/build/src/toolkit/mozapps/update/updater/updater.cpp:3750
19:56:20     INFO -      #3 0x4920c1 in UpdateThreadFunc(void*) /builds/slave/try-l64-asan-00000000000000000/build/src/toolkit/mozapps/update/updater/updater.cpp:2278
19:56:20     INFO -      #4 0x7f4c7e8a7e99 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x7e99)

Comment 2

[Molly Howell (she/her) (no longer active)]

I just independently rediscovered this leak, so I'll add some detail for anybody that might end up working on it.

The leaked object is the buffer returned from GetManifestContents in updater.cpp, whichever buffer that is for the specific platform (wrb on Windows, mbuf/rb elsewhere). That function is called in two places:
For the call in DoUpdate, just freeing the buffer normally at the end of the function is all that's needed. I would use a mozilla::UniquePtr to make that easy.
For the call in AddPreCompleteActions, freeing the buffer at the end of that function would break things right now. The problem is that pointers into the middle of the buffer are saved by the various Action types in their Parse methods, and those pointers are used after AddPreCompleteActions returns, when DoUpdate prepares and executes the whole ActionList. Either the buffer needs to be somehow preserved until the end of DoUpdate and then freed (by having a reference to it somewhere in DoUpdate's scope, or using reference counting where each Action that's created increments the count, or something), or the Action types need to make their own copies of the parts of the buffer that they care about, so that they no longer depend on the whole thing sticking around.

This is not a high-priority leak because a) it amounts to a handful of kilobytes, b) it's not in the main application, it's in the updater binary, which is only invoked during application updates, and c) the updater exits soon after these buffers would be freed.

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: Bug 1168010 - Fix leaks in updater.cpp and nsUpdateDriver.cpp. r=spohl

Fixes leak where the return value of GetManifestContents in updater.cpp is not freed
Fixes leak where the return value of get_quoted_path in updater.cpp is not freed
Fixes leak in nsUpdateDriver.cpp ApplyUpdate
With these leaks fixed the UI tests that stage updates can run on Linux asan

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4fe736c38366
Fix leaks in updater.cpp and nsUpdateDriver.cpp. r=spohl

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/4fe736c38366