Bug 211600

Summary:

Crash under WebKit::XPCServiceMain

Product:

WebKit

Reporter:

Chris Dumez <cdumez>

Component:

WebKit2

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, darin, ddkilzer, ggaren, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none

Description Chris Dumez 2020-05-07 15:30:18 PDT

Crash under WebKit::XPCServiceMain:
Thread 0 Crashed ↩:: Dispatch queue: com.apple.main-thread
0   libsystem_platform.dylib      	0x00007fff70c528b5 _platform_strcmp + 181
1   com.apple.WebKit              	0x7fff47d28ca7 WebKit::XPCServiceMain(int, char const**) + 114 (/AppleInternal/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebKit2/WebKit2-7609.1.20.111.8/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:125)
2   libdyld.dylib                 	0x00007fff70a5ccc9 start + 1

Comment 1 Chris Dumez 2020-05-07 15:30:29 PDT

<rdar://problem/62875458>

Comment 2 Chris Dumez 2020-05-07 15:37:11 PDT

Created attachment 398806 [details]
Patch

Comment 3 Darin Adler 2020-05-07 15:44:49 PDT

Comment on attachment 398806 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        - Crash under strcmp() could in theory happen if expectedBundleVersion.UTF8String was null, which could
> +          happen if expectedBundleVersion was null. I now use higher level String types for the versions, make
> +          sure they are not null and use String comparison to compare them.

I understand how checks for null are a good idea. Not sure why using WTF::String is helpful.

Comment 4 Chris Dumez 2020-05-07 15:49:47 PDT

(In reply to Darin Adler from comment #3)
> Comment on attachment 398806 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398806&action=review
> 
> > Source/WebKit/ChangeLog:12
> > +        - Crash under strcmp() could in theory happen if expectedBundleVersion.UTF8String was null, which could
> > +          happen if expectedBundleVersion was null. I now use higher level String types for the versions, make
> > +          sure they are not null and use String comparison to compare them.
> 
> I understand how checks for null are a good idea. Not sure why using
> WTF::String is helpful.

This was my own personal preference, I much prefer dealing C++ types, than C or ObjC types. We are getting one of the version as a const char* and there other as a NSString*, both can silently be converted to String type, so this was convenient too.

Comment 5 Chris Dumez 2020-05-07 15:50:51 PDT

Created attachment 398808 [details]
Patch

Comment 6 Chris Dumez 2020-05-07 15:51:13 PDT

(In reply to Chris Dumez from comment #5)
> Created attachment 398808 [details]
> Patch

Fixes iOS build.

Comment 7 EWS 2020-05-07 16:54:59 PDT

Committed r261360: <https://trac.webkit.org/changeset/261360>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398808 [details].

Comment 8 Alexey Proskuryakov 2020-05-07 17:31:02 PDT

I hope that this fixes all crashes _under_ WebKit::XPCServiceMain ;)

Comment 9 Darin Adler 2020-05-08 08:59:08 PDT

Comment on attachment 398808 [details]
Patch

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

> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:142
> +        String webKitBundleVersion = xpc_dictionary_get_string(bootstrap.get(), "WebKitBundleVersion");

Just noticed a change in behavior here. The old code treated this as UTF-8. The new code treats it as Latin-1. But I suspect really this is all guaranteed to be ASCII so this change in behavior is not important.