Add a safe API for NSValue and migrate NSValue value fetching to the size variants for validation #22265

phausler · 2019-01-31T17:49:46Z

No description provided.

phausler · 2019-01-31T17:49:55Z

@swift-ci please smoke test

phausler · 2019-01-31T19:01:07Z

@swift-ci please smoke test

jrose-apple · 2019-01-31T21:01:56Z

stdlib/public/Darwin/Foundation/NSValue.swift.gyb

+extension NSValue {
+    @_transparent // this must be transparent to pass the type encoding over to the _getObjCTypeEncoding function
+    public func value<StoredType>(of type: StoredType.Type) -> StoredType? {
+        guard strcmp(objCType, _getObjCTypeEncoding(StoredType.self)) == 0 else {


@transparent doesn't guarantee this, but I'm also concerned about adding this check to NSValue when it wasn't there before. You can see #19691 for an example where this would cause a problem.

Well there are some other monsters under the water here. Passing in String.self or Any.self crashes the compiler.

phausler · 2019-02-05T22:18:48Z

@swift-ci please smoke test

itaiferber · 2019-02-05T22:23:36Z

stdlib/public/Darwin/Foundation/NSValue.swift.gyb

+                return nil
+            }
+            let allocated = UnsafeMutablePointer<StoredType>.allocate(capacity: 1)
+            defer { allocated.deallocate() }


This feels potentially dangerous to me. Are we absolutely guaranteed that the read from this address to copy the value out will happen prior to the deallocation?

I don't have a better answer, this is just triggering alarms in my head. Who can we ask about this?

Returning by value needs to make that copy so that it be returned; I'm not sure what sequence of events would cause the return out not to be filled before this defer is invoked.

yup, the deallocation happens at the end of the scope, after the assignment of the return value to send back from my understanding

Confirming that this is guaranteed to do the right thing; Philippe is correct.

Okay, I figured as much. I just wanted to be absolutely certain.

itaiferber · 2019-02-05T22:26:38Z

stdlib/public/Darwin/Foundation/NSValue.swift.gyb

@@ -23,3 +23,30 @@ ${ ObjectiveCBridgeableImplementationForNSValue("CGVector") }
 ${ ObjectiveCBridgeableImplementationForNSValue("CGSize") }
 ${ ObjectiveCBridgeableImplementationForNSValue("CGAffineTransform") }

+extension NSValue {
+    public func value<StoredType>(of type: StoredType.Type) -> StoredType? {
+        if StoredType.self == AnyObject.self {


This case and the NSObject case can just be coalesced into

if type is AnyObject.Type { guard String(cString: objcType) == "@" else { return nil } return self.nonretainedObjectValue as? StoredType }

The metatype check should catch all reference types

but this would allow swift classes not rooted on NSObject right?

Yeah, but couldn't you end up doing the same thing by storing something as an AnyObject in the first place?

At least I guess we can coalesce the cases with if StoredType.self == AnyObject.self || StoredType.self is NSObject.Type — it just looks like both implementations are exactly equal.

Is being rooted in NSObject a requirement of something? The code already allows Swift classes not rooted in NSObject, and secretly all Swift objects on Darwin are rooted in NSObject anyway, IIRC.

(Feel free to correct me if not so.)

I guess it is probably isomorphic anyhow on Darwin platforms, I can collapse it

Swift objects on Darwin are not rooted in NSObject; they just conform to NSObjectProtocol.

I can't think of any problem passing a Swift class instance through an NSValue (as long as you keep it alive while doing so), but I might not have all the information.

itaiferber · 2019-02-05T22:28:04Z

stdlib/public/Darwin/Foundation/NSValue.swift.gyb

+            getValue(allocated, size: storedSize)
+            return allocated.pointee
+        }
+        return nil


I'm a little concerned about returning nil here — wouldn't it surprise someone to box up a

struct Foo { let name: String let value: Any }

but not be able to unbox it safely this way?

It is, but it is also fundamentally unsafe to do it, since String is not a POD.

you cant really safely box that in the first place tbh, that is something that should be addressed later

Sorry, the crux of that example was actually meant to be the Any and not the String, but the same applies — it's not safe to box up a Foo, but for someone who has already boxed up the Foo, they can only get it out using the existing, less safe methods.

It would be nicer to be less restrictive on fetching values out and more restrictive on stuffing values in, but I guess we can do that in the future.

I don't think it's practically possible for someone to have ended up with an already-NSValue-boxed Foo. The Swift runtime won't ever box it in a Foo, and client code would have to go pretty far out of their way to manually copy the struct into an NSValue themself. (Nevermind that once they did, they'd have to deal with memory leaks and/or use-after-frees because NSValue can't copy or destroy a non-POD value when the object is copied or deallocated.)

phausler · 2019-02-05T23:43:06Z

@swift-ci please smoke test

phausler · 2019-02-06T17:57:16Z

@swift-ci please smoke test

phausler · 2019-02-06T18:54:20Z

@swift-ci please smoke test

phausler · 2019-02-06T20:35:44Z

@swift-ci please smoke test

phausler · 2019-02-07T22:17:05Z

@swift-ci please smoke test

phausler · 2019-02-12T18:56:51Z

@swift-ci please smoke test

phausler · 2019-02-12T23:36:09Z

@swift-ci please smoke test

phausler · 2019-02-13T18:00:58Z

@swift-ci please smoke test

phausler · 2019-02-13T20:21:13Z

@swift-ci please smoke test

…size variants for validation

phausler · 2019-02-13T20:58:45Z

@swift-ci please smoke test

phausler force-pushed the safe_nsvalue_accessor branch from 50e8501 to cc88199 Compare January 31, 2019 17:53

millenomi approved these changes Jan 31, 2019

View reviewed changes

jrose-apple reviewed Jan 31, 2019

View reviewed changes

phausler force-pushed the safe_nsvalue_accessor branch from cc88199 to bbcc7bd Compare February 5, 2019 22:18

itaiferber reviewed Feb 5, 2019

View reviewed changes

phausler force-pushed the safe_nsvalue_accessor branch from bbcc7bd to 25ccee6 Compare February 5, 2019 22:48

phausler force-pushed the safe_nsvalue_accessor branch from 25ccee6 to 504c1b8 Compare February 6, 2019 18:53

phausler force-pushed the safe_nsvalue_accessor branch from 504c1b8 to 23feeff Compare February 6, 2019 20:35

phausler force-pushed the safe_nsvalue_accessor branch from 23feeff to 660203c Compare February 7, 2019 22:16

phausler force-pushed the safe_nsvalue_accessor branch from 660203c to f0d2f1f Compare February 8, 2019 22:34

phausler force-pushed the safe_nsvalue_accessor branch from f0d2f1f to 5586886 Compare February 12, 2019 20:50

phausler force-pushed the safe_nsvalue_accessor branch from 5586886 to 48b71a6 Compare February 13, 2019 18:00

phausler force-pushed the safe_nsvalue_accessor branch from 48b71a6 to f10ce87 Compare February 13, 2019 20:20

Add a safe API for NSValue and migrate NSValue value fetching to the …

6b438b5

…size variants for validation

phausler force-pushed the safe_nsvalue_accessor branch from f10ce87 to 6b438b5 Compare February 13, 2019 20:58

phausler merged commit 1fe1b86 into swiftlang:master Feb 13, 2019

Add a safe API for NSValue and migrate NSValue value fetching to the size variants for validation #22265

Add a safe API for NSValue and migrate NSValue value fetching to the size variants for validation #22265

Uh oh!

Conversation

phausler commented Jan 31, 2019

Uh oh!

phausler commented Jan 31, 2019

Uh oh!

phausler commented Jan 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phausler commented Feb 5, 2019

Uh oh!

itaiferber Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itaiferber Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phausler commented Feb 5, 2019

Uh oh!

phausler commented Feb 6, 2019

Uh oh!

phausler commented Feb 6, 2019

Uh oh!

phausler commented Feb 6, 2019

Uh oh!

phausler commented Feb 7, 2019

Uh oh!

phausler commented Feb 12, 2019

Uh oh!

phausler commented Feb 12, 2019

Uh oh!

phausler commented Feb 13, 2019

Uh oh!

phausler commented Feb 13, 2019

Uh oh!

phausler commented Feb 13, 2019

Uh oh!

Uh oh!

itaiferber Feb 5, 2019 •

edited

Loading

itaiferber Feb 5, 2019 •

edited

Loading