1454448 - Free of static memory in ExecuteServiceCommand()

Status: RESOLVED FIXED

(Toolkit :: Application Update, enhancement)

Description

[mozillabugs]

ExecuteServiceCommand() (toolkit\components\maintenanceservice\workmonitor.cpp) can attempt to free static memory if it experiences an OOM.

The bug is that line 584 doesn't check the status from UuidToString(), which can fail with OOM (see https://msdn.microsoft.com/en-us/library/windows/desktop/aa379352(v=vs.85).aspx ). If that occurs, line 588 unconditionally frees |guidString|, which contains a pointer to static memory:

570: BOOL
571: ExecuteServiceCommand(int argc, LPWSTR *argv)
572: {
573:   if (argc < 3) {
574:     LOG_WARN(("Not enough command line arguments to execute a service command"));
575:     return FALSE;
576:   }
577: 
578:   // The tests work by making sure the log has changed, so we put a
579:   // unique ID in the log.
580:   RPC_WSTR guidString = RPC_WSTR(L"");
581:   GUID guid;
582:   HRESULT hr = CoCreateGuid(&guid);
583:   if (SUCCEEDED(hr)) {
584:     UuidToString(&guid, &guidString);
585:   }
586:   LOG(("Executing service command %ls, ID: %ls",
587:        argv[2], reinterpret_cast<LPCWSTR>(guidString)));
588:   RpcStringFree(&guidString);
...
774: }

Comment 1

[Daniel Veditz [:dveditz]]

The maintenance service is a separate process that only processes signed update files so not too worried about this one.

Comment 2

[Molly Howell (she/her) (mostly inactive)]

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

Comment 3

[Molly Howell (she/her) (mostly inactive)]

Attachment: bug1454448.diff

Comment 4

[Molly Howell (she/her) (mostly inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/983b15929e0424b2346df3d39af2d352ed4db0b9
Bug 1454448 - Properly handle an allocation failure. r=rstrong

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/983b15929e04

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Sounds like this can ride the trains.