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.
<rdar://problem/37865316>
Created attachment 336693 [details] Patch
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?
Created attachment 336699 [details] Patch
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?
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.
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.
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.
Comment on attachment 336699 [details] Patch Ok then,
Committed r230045: <https://trac.webkit.org/changeset/230045>
Follow-up build fix: Committed r230047: <https://trac.webkit.org/changeset/230047>