Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 13, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions stdlib/public/Darwin/Foundation/NSValue.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,31 @@ ${ ObjectiveCBridgeableImplementationForNSValue("CGVector") }
${ ObjectiveCBridgeableImplementationForNSValue("CGSize") }
${ ObjectiveCBridgeableImplementationForNSValue("CGAffineTransform") }

extension NSValue {
public func value<StoredType>(of type: StoredType.Type) -> StoredType? {
if StoredType.self is AnyObject.Type {
let encoding = String(cString: objCType)
// some subclasses of NSValue can return @ and the default initialized variant returns ^v for encoding
guard encoding == "^v" || encoding == "@" else {
return nil
}
return nonretainedObjectValue as? StoredType
} else if _isPOD(StoredType.self) {
var storedSize = 0
var storedAlignment = 0
NSGetSizeAndAlignment(objCType, &storedSize, &storedAlignment)
guard MemoryLayout<StoredType>.size == storedSize && MemoryLayout<StoredType>.alignment == storedAlignment else {
return nil
}
let allocated = UnsafeMutablePointer<StoredType>.allocate(capacity: 1)
defer { allocated.deallocate() }
Copy link
Contributor

@itaiferber itaiferber Feb 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@itaiferber itaiferber Feb 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if #available(OSX 10.13, iOS 11.0, tvOS 11.0, watchOS 4.0, *) {
getValue(allocated, size: storedSize)
} else {
getValue(allocated)
}
return allocated.pointee
}
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

}
}
27 changes: 27 additions & 0 deletions test/stdlib/NSValueBridging.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,4 +91,31 @@ nsValueBridging.test("NSValue can only be cast back to its original type") {
_ = rangeObject as! CGRect
}

nsValueBridging.test("NSValue fetching method should be able to convert constructed values safely") {
let range = NSRange(location: 17, length: 38)
let value = NSValue(range: range)
expectEqual(value.value(of: NSRange.self)?.location, range.location)
expectEqual(value.value(of: NSRange.self)?.length, range.length)
expectEqual(value.value(of: CGRect.self), .none)
expectEqual(value.value(of: String.self), .none)
expectTrue(value.value(of: AnyObject.self) == nil)


class GenericThingNotRootedInObjC<T> {
init() { }
}

let obj = GenericThingNotRootedInObjC<String>()
// extend the lifetime to ensure that the non retained pointer is valid for the test duration
withExtendedLifetime(obj) { () -> Void in
let value = NSValue(nonretainedObject: obj)
let resString = value.value(of: GenericThingNotRootedInObjC<String>.self)
expectEqual(resString === obj, true) // ensure the value is exactly the same

let resInt = value.value(of: GenericThingNotRootedInObjC<Int>.self)
expectTrue(resInt == nil)
}

}

runAllTests()
18 changes: 15 additions & 3 deletions utils/gyb_foundation_support.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ def ObjectiveCBridgeableImplementationForNSValue(Type):
_getObjCTypeEncoding({Type}.self)) == 0,
"NSValue does not contain the right type to bridge to {Type}")
result = {Type}()
source.getValue(&result!)
if #available(OSX 10.13, iOS 11.0, tvOS 11.0, watchOS 4.0, *) {{
source.getValue(&result!, size: MemoryLayout<{Type}>.size)
}} else {{
source.getValue(&result!)
}}
}}

public static func _conditionallyBridgeFromObjectiveC(_ source: NSValue,
Expand All @@ -23,7 +27,11 @@ def ObjectiveCBridgeableImplementationForNSValue(Type):
return false
}}
result = {Type}()
source.getValue(&result!)
if #available(OSX 10.13, iOS 11.0, tvOS 11.0, watchOS 4.0, *) {{
source.getValue(&result!, size: MemoryLayout<{Type}>.size)
}} else {{
source.getValue(&result!)
}}
return true
}}

Expand All @@ -34,7 +42,11 @@ def ObjectiveCBridgeableImplementationForNSValue(Type):
_getObjCTypeEncoding({Type}.self)) == 0,
"NSValue does not contain the right type to bridge to {Type}")
var result = {Type}()
unwrappedSource.getValue(&result)
if #available(OSX 10.13, iOS 11.0, tvOS 11.0, watchOS 4.0, *) {{
unwrappedSource.getValue(&result, size: MemoryLayout<{Type}>.size)
}} else {{
unwrappedSource.getValue(&result)
}}
return result
}}
}}
Expand Down