1642257 - GetProcInfo leaks the thread list

Status: RESOLVED FIXED

(Core :: Widget, defect, P3)

Description

[Markus Stange [:mstange]]

I missed this in my original review:

The threadList in GetProcInfo is allocated by task_threads but never freed.

There's not much useful documentation on task_threads online, but I did find this, which suggests using vm_deallocate: "The caller may wish to vm_deallocate this array when the data is no longer needed."
https://www.gnu.org/software/hurd/gnumach-doc/Task-Information.html

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Neha Kochar [:neha]]

David said the fix for bug 1652000 will fix this too. Since bug 1652000 is in M6b, moving this too.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

I attempted to land this as part of bug 1652000.

Turns out that the following line causes crashes.

auto guardThreadCount = MakeScopeExit([&] {
    vm_deallocate(selectedTask, (vm_address_t)threadList, sizeof(thread_t) * threadCount);
});

So either I'm deallocating improperly (likely) or we shouldn't deallocate this at all (would be surprising).

I wonder if we have a way to detect whether we actually leak memory in this case.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

It's interesting. We have several instances of function task_threads several times in mozilla-central. Only one of them deallocates: https://searchfox.org/mozilla-central/rev/b2395478c6adf6e5b241be1610fb1d920ed995ed/third_party/python/psutil/psutil/_psutil_osx.c#907-940 .

Other code found on the web first closes the ports before deallocating: https://gist.github.com/knightsc/bd6dfeccb02b77eb6409db5601dcef36 .

Of course, all of this is undocumented on Apple's site.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1642257 - Auto-clearing thread list in GetProcInfo;r?tarek!

Comment 6

[David Teller [:Yoric] - still alive but not very active]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc680297d5dfc09211d087201f7c806bc3a6ff27

Comment 7

[Markus Stange [:mstange]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #6)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc680297d5dfc09211d087201f7c806bc3a6ff27

There are two crashes in this push, too, so it seems deallocating the ports didn't work. (I would have surprised if it did. But I was also surprised that this causes crashes in the first place...)

Comment 8

[Markus Stange [:mstange]]

Oh, hah, I found the bug! You're deallocating memory in the wrong process! The list is allocated in the current task, but you're deallocating the memory in the remote task.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

(In reply to Markus Stange [:mstange] from comment #8)

Oh, hah, I found the bug! You're deallocating memory in the wrong process! The list is allocated in the current task, but you're deallocating the memory in the remote task.

That is odd... my latest patch seems to work and I haven't changed deallocation. I've just deallocating list items before deallocating the list itself. But then, I'll be first to confirm that it's guesswork at this stage.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=097de333ceca933d3daa6a171761ad0000ccdf05&selectedTaskRun=EOdcsMqtRaeczxlnXzQmHQ.0

Comment 10

[Markus Stange [:mstange]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #9)

(In reply to Markus Stange [:mstange] from comment #8)

Oh, hah, I found the bug! You're deallocating memory in the wrong process! The list is allocated in the current task, but you're deallocating the memory in the remote task.

That is odd... my latest patch seems to work and I haven't changed deallocation.

Well, be that as it may; you cannot just deallocate random memory in another process with a memory address from the current process.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=097de333ceca933d3daa6a171761ad0000ccdf05&selectedTaskRun=EOdcsMqtRaeczxlnXzQmHQ.0

This still calls vm_deallocate(selectedTask, ...). It should be calling vm_deallocate(mach_task_self(), ...).

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f6cd02e4a9a
Auto-clearing thread list in GetProcInfo;r=mstange

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/0f6cd02e4a9a