Bug 184106 – Protect against invalid mach ports returned by mach_port_request_notification

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

If mach_port_request_notification returns an error, the returned previous notification port may be uninitialized. We need to make sure we check error codes returned by this call, and make sure the returned port is always used safely.

[Brent Fulgham]

<rdar://problem/37865316>

[Brent Fulgham]

Created attachment 336693 [details]
Patch

[Chris Dumez]

Comment on attachment 336693 [details]
Patch

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

> Source/WebKit/Platform/IPC/mac/ConnectionMac.mm:539
> +                // If mach_port_request_notification fails, 'previousNotificationPort' will be uninitialized.

I do not get it. previousNotificationPort is initialized to MACH_PORT_NULL before the mach_port_request_notification(). Are we really worried about mach_port_request_notification() failing and STILL setting our previousNotificationPort to something else than MACH_PORT_NULL?

[Brent Fulgham]

Created attachment 336699 [details]
Patch

[Chris Dumez]

Comment on attachment 336699 [details]
Patch

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

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:163
> +    mach_port_t previousNotificationPort = MACH_PORT_NULL;

I understand the need to initialize the variable before the call to mach_port_request_notification().

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:170
> +        previousNotificationPort = MACH_PORT_NULL;

I do not understand the need to reset it to NULL if the calls to mach_port_request_notification fails.

if mach_port_request_notification() fails and somehow still sets our previousNotificationPort to something else than NULL. Then why wouldn't we want to deallocate it?

[Chris Dumez]

Comment on attachment 336699 [details]
Patch

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

>> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:163
>> +    mach_port_t previousNotificationPort = MACH_PORT_NULL;
> 
> I understand the need to initialize the variable before the call to mach_port_request_notification().

Looking at rdar://problem/37865316, I think this line change should be the only change we need to make. Please let me know why you think the rest is needed.

[Chris Dumez]

Comment on attachment 336699 [details]
Patch

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

>>> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:163
>>> +    mach_port_t previousNotificationPort = MACH_PORT_NULL;
>> 
>> I understand the need to initialize the variable before the call to mach_port_request_notification().
> 
> Looking at rdar://problem/37865316, I think this line change should be the only change we need to make. Please let me know why you think the rest is needed.

To be clear, I am also fine with the extra assertions and the extra LOG_ERROR(). I do not understand resetting previousNotificationPort to MACH_PORT_NULL in case of error though.

[Brent Fulgham]

Comment on attachment 336699 [details]
Patch

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

>>>> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:163
>>>> +    mach_port_t previousNotificationPort = MACH_PORT_NULL;
>>> 
>>> I understand the need to initialize the variable before the call to mach_port_request_notification().
>> 
>> Looking at rdar://problem/37865316, I think this line change should be the only change we need to make. Please let me know why you think the rest is needed.
> 
> To be clear, I am also fine with the extra assertions and the extra LOG_ERROR(). I do not understand resetting previousNotificationPort to MACH_PORT_NULL in case of error though.

I spoke with an engineer responsible for MiG stuff. He said that it would be unwise, since the contract of 'mach_port_request_notification' is meant to imply that you don't do ANYTHING with the return values if it gives you an error.

So, we definitely don't want to call 'mach_port_deallocate' or something with its output value if it generated an error.

However, previousNotificationPort is never used again in this method, so I'm find removing the assignment.

>> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:170
>> +        previousNotificationPort = MACH_PORT_NULL;
> 
> I do not understand the need to reset it to NULL if the calls to mach_port_request_notification fails.
> 
> if mach_port_request_notification() fails and somehow still sets our previousNotificationPort to something else than NULL. Then why wouldn't we want to deallocate it?

We don't deallocate this port in this method, so I'm fine just removing the assignment here.

[Chris Dumez]

Comment on attachment 336699 [details]
Patch

Ok then,

[Brent Fulgham]

Committed r230045: <https://trac.webkit.org/changeset/230045>

[Brent Fulgham]

Follow-up build fix:
Committed r230047: <https://trac.webkit.org/changeset/230047>