Bug 184202 – Failures from mach port reference handling should be fatal

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

We may corrupt the Mach port space by improperly matching the equivalent of reference counting retains (mach_port_mod_refs) with releases (mach_port_deallocate).

Our current implementation of MachSendRights::create does not grab a reference if the passed port is MACH_PORT_DEAD, but we unconditionally call mach_port_deallocate on the port, which could lead to a reference count mismatch.

Likewise, our MachSendRight destructor does not release the port if it has changed to MACH_PORT_DEAD (e.g., if a child process dies), again leading to a mismatch in retain/releases.

Finally, failures in mach_port_deallocate should be fatal because they indicate that the application was attempting to remove an unowned right. This is a fatal condition for Mach, and should lead to an abort. 

This patch does the following:

1. It creates a helper function that does the right thing for safely deallocating a mach port.
2. It uses it in multiple places.
3. It revises 'MachSendRight::create" so that it properly handles the condition of a dead port.
4. It revises the MachSendRight destructor to properly handle the condition of a dead port.

[Brent Fulgham]

<rdar://problem/37771114>

[Brent Fulgham]

Created attachment 336898 [details]
Patch

[Brent Fulgham]

Created attachment 336900 [details]
Patch

[EWS Watchlist]

Comment on attachment 336900 [details]
Patch

Attachment 336900 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7153261

New failing tests:
http/tests/xmlhttprequest/access-control-repeated-failed-preflight-crash.html

[EWS Watchlist]

Created attachment 336903 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Anders Carlsson]

Comment on attachment 336900 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336900&action=review

> Source/WebCore/platform/cocoa/MachSendRight.h:56
> +WEBCORE_EXPORT void deallocateMachPortSafely(mach_port_t);

I don't think you should name this deallocateMachPortSafely. You should use the term "send right" and "receive right" to better indicate what type of mach port is being deallocated.

[Brent Fulgham]

David Remahl (Apple mach guy) reviewed the mach code changes and approved them.

[Brent Fulgham]

(In reply to Anders Carlsson from comment #6)
> Comment on attachment 336900 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336900&action=review
> 
> > Source/WebCore/platform/cocoa/MachSendRight.h:56
> > +WEBCORE_EXPORT void deallocateMachPortSafely(mach_port_t);
> 
> I don't think you should name this deallocateMachPortSafely. You should use
> the term "send right" and "receive right" to better indicate what type of
> mach port is being deallocated.

Do you mean something like "deallocateMachSendOrReceiveRightSafely"?

Or do you mean have a send right version and a receive right version?

[Brent Fulgham]

Created attachment 337016 [details]
Patch

[Brent Fulgham]

Created attachment 337018 [details]
Patch

[EWS Watchlist]

Comment on attachment 337018 [details]
Patch

Attachment 337018 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7182362

New failing tests:
http/tests/xmlhttprequest/access-control-repeated-failed-preflight-crash.html

[EWS Watchlist]

Created attachment 337027 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Brent Fulgham]

Created attachment 337131 [details]
Patch

[Anders Carlsson]

Comment on attachment 337131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337131&action=review

> Source/WTF/wtf/cocoa/CPUTimeCocoa.mm:88
>      RELEASE_ASSERT(ret == KERN_SUCCESS);
> -    mach_port_deallocate(mach_task_self(), threadPort);
> +    auto kr = mach_port_deallocate(mach_task_self(), threadPort);
> +    if (kr != KERN_SUCCESS) {
> +        LOG_ERROR("mach_port_deallocate error for port %d: %s (%x)", threadPort, mach_error_string(kr), kr);
> +        if (kr == KERN_INVALID_RIGHT)
> +            CRASH();
> +    }
>  
>      return Seconds(info.user_time.seconds + info.system_time.seconds) + Seconds::fromMicroseconds(info.user_time.microseconds + info.system_time.microseconds);

I think you can just use Mach::SendRight here directly, something like

auto threadPort = Mach::SendRight::adopt(mach_thread_self());
auto ret = thread_info(threadPort.sendRight(), ...);

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:171
>          if (!(threadBasicInfo->flags & TH_FLAGS_IDLE))
>              usage += threadBasicInfo->cpu_usage / static_cast<float>(TH_USAGE_SCALE) * 100.0;
>  
> -        mach_port_deallocate(mach_task_self(), threadList[i]);
> +        auto kr = mach_port_deallocate(mach_task_self(), threadList[i]);
> +        if (kr != KERN_SUCCESS) {
> +            LOG_ERROR("mach_port_deallocate error for port %d: %s (%#x)", threadList[i], mach_error_string(kr), kr);
> +            if (kr == KERN_INVALID_RIGHT)
> +                CRASH();
> +        }

Looking at cpuUsage(), the memory (as well as the mach ports!) are leaked if the call to thread_info fails. (Since it returns without deallocating ports or memory).

I think you should add a function that returns the threads as a Vector<MachSendRight>, something like

Vector<MachSendRight> getMachThreads();

then you can have cpuUsage call the function and you'll get the lifetime management for free.

> Source/WebCore/platform/cocoa/MachSendRight.cpp:62
> +void deallocateSendOrReceiveRightSafely(mach_port_t port)

I don't think this function is ever used on receive rights (because they are released using mach_port_mod_refs).

[Brent Fulgham]

Comment on attachment 337131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337131&action=review

>> Source/WTF/wtf/cocoa/CPUTimeCocoa.mm:88
>>      return Seconds(info.user_time.seconds + info.system_time.seconds) + Seconds::fromMicroseconds(info.user_time.microseconds + info.system_time.microseconds);
> 
> I think you can just use Mach::SendRight here directly, something like
> 
> auto threadPort = Mach::SendRight::adopt(mach_thread_self());
> auto ret = thread_info(threadPort.sendRight(), ...);

Oh! Yes, I just need to move WebCore/platform/cocoa/MachSendRight -> WTF/etc.

>> Source/WebCore/platform/cocoa/MachSendRight.cpp:62
>> +void deallocateSendOrReceiveRightSafely(mach_port_t port)
> 
> I don't think this function is ever used on receive rights (because they are released using mach_port_mod_refs).

Yeah, I'll rename.

[Brent Fulgham]

Created attachment 337148 [details]
Patch

[Brent Fulgham]

Created attachment 337177 [details]
Patch

[Anders Carlsson]

Comment on attachment 337177 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337177&action=review

Not entirely sure why you moved MachSendRight to WTF - it doesn't seem to be used by anything below WebCore. Looks good otherwise.

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:142
> +static Vector<MachSendRight> getMachThreads()

I think you should call this threadSendrights() - a get prefix usually implies a boolean return value + out parameter.

> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:148
> +        return Vector<MachSendRight>();

Can just return { }; here I think.

> Source/WebCore/platform/graphics/avfoundation/objc/VideoFullscreenLayerManagerObjC.mm:130
>              mach_port_t fencePort = [oldContext createFencePort];
>              [newContext setFencePort:fencePort];
> -            mach_port_deallocate(mach_task_self(), fencePort);
> +            deallocateSendRightSafely(fencePort);

Can just do

auto fencePort = MachSendRight::adopt([oldContext createFencePort] here.

> Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp:71
> +    if (m_port) {
> +        auto kr = mach_port_deallocate(mach_task_self(), m_port);
> +        if (kr != KERN_SUCCESS) {
> +#if RELEASE_LOG_DISABLED
> +            ASSERT_UNUSED(kr, kr == KERN_SUCCESS);
> +#else
> +            RELEASE_LOG_ERROR(VirtualMemory, "%p - SharedMemory::Handle::clear: Failed to deallocate port. %{public}s (%x)", this, mach_error_string(kr), kr);
> +            ASSERT_NOT_REACHED();
> +#endif
> +        }
> +    }

Can't this use deallocateSendRightSafely?

[Brent Fulgham]

Comment on attachment 337177 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337177&action=review

>> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:142
>> +static Vector<MachSendRight> getMachThreads()
> 
> I think you should call this threadSendrights() - a get prefix usually implies a boolean return value + out parameter.

OK!

>> Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm:148
>> +        return Vector<MachSendRight>();
> 
> Can just return { }; here I think.

OK!

>> Source/WebCore/platform/graphics/avfoundation/objc/VideoFullscreenLayerManagerObjC.mm:130
>> +            deallocateSendRightSafely(fencePort);
> 
> Can just do
> 
> auto fencePort = MachSendRight::adopt([oldContext createFencePort] here.

OK!

>> Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp:71
>> +    }
> 
> Can't this use deallocateSendRightSafely?

Oh, yes!

[Brent Fulgham]

Committed r230269: <https://trac.webkit.org/changeset/230269>