Bug 219993

Summary: $(findstring iphone,$(SDKROOT)) fails when SDKROOT is not lowercase
Product: WebKit Reporter: Ryan Hostetler <rhost>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric.carlson, ews-watchlist, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Ryan Hostetler 2020-12-17 12:38:25 PST 
rdar://72436093

In our makefiles checks SDKROOT are case sensitive and fail if the SDKROOT has uppercase.
EG: $(findstring iphone,$(SDKROOT)) fails with SDKs cased as iPhoneOS.

This patch converts SDKROOT path to lowercase so any casing is matched in the same way.
Comment 1 Ryan Hostetler 2020-12-17 12:45:03 PST 
Created attachment 416453 [details]
Patch
Comment 2 Darin Adler 2020-12-17 12:54:42 PST 
Is there any user controlled directory name that can be in the SDKROOT path? What if the volume name contains the letter "iPhone", "TV", or "macOS" in them?
Comment 3 Ryan Hostetler 2020-12-17 13:11:45 PST 
(In reply to Darin Adler from comment #2)
> Is there any user controlled directory name that can be in the SDKROOT path?
> What if the volume name contains the letter "iPhone", "TV", or "macOS" in
> them?

The default SDKROOT path lives within the Xcode directory eg:
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.0.sdk

The most likely problem would arise from a renamed Xcode eg: `Xcode-iphone.app`, which is still a risk without this patch.

If we wanted to protect further we could use match against `sdks/iphone`, though I'm not sure if there are use cases where the SDK would be moved out of the `SDKs` path.
Comment 4 Darin Adler 2020-12-17 13:47:11 PST 
(In reply to Ryan Hostetler from comment #3)
> The default SDKROOT path lives within the Xcode directory eg:
> /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/
> Developer/SDKs/iPhoneOS13.0.sdk
> 
> The most likely problem would arise from a renamed Xcode eg:
> `Xcode-iphone.app`, which is still a risk without this patch.

Or an Xcode someone put into a directory like "/Users/darin/iPhoneTools".

And, yes, I was not suggesting it’s new in this patch, although doing case folding makes it even more likely to get it wrong.

> If we wanted to protect further we could use match against `sdks/iphone`,
> though I'm not sure if there are use cases where the SDK would be moved out
> of the `SDKs` path.

Sure, or even "platform/developer/sdks/iphone". Not sure what’s best.
Comment 5 Darin Adler 2020-12-17 13:48:35 PST 
Or use something that extracts only the last piece of the SDKROOT path. There must be a function for that, maybe "basename".
Comment 6 Ryan Hostetler 2020-12-17 14:13:23 PST 
(In reply to Darin Adler from comment #5)
> Or use something that extracts only the last piece of the SDKROOT path.
> There must be a function for that, maybe "basename".

Looks like notdir will grab just the SDK name:
$(notdir $(call to_lower,$(SDKROOT)))
> iphoneos13.0.sdk
Comment 7 Ryan Hostetler 2020-12-17 14:22:46 PST 
Created attachment 416470 [details]
Patch
Comment 8 Darin Adler 2020-12-17 15:17:35 PST 
Comment on attachment 416470 [details]
Patch

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

> Source/Makefile:6
> +	ifneq (,$(findstring macosx,$(notdir $(call to_lower,$(SDKROOT)))))

I think it would be better to call notdir first before calling shell, so we pass a shorter string. Not sure why we used all lowercase for "to_lower" when we use all uppercase for other make variables.
Comment 9 Ryan Hostetler 2020-12-17 15:35:36 PST 
Created attachment 416476 [details]
Patch
Comment 10 Ryan Haddad 2020-12-21 12:41:16 PST 
rdar://72436093
Comment 11 EWS 2020-12-21 12:46:35 PST 
Committed r271037: <https://trac.webkit.org/changeset/271037>

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