Skip to content
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.

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

[SR-1957] Remove unsafeAddressOf #44566

Closed
gribozavr opened this issue Jul 1, 2016 · 10 comments
Closed

[SR-1957] Remove unsafeAddressOf #44566

gribozavr opened this issue Jul 1, 2016 · 10 comments
Labels
affects ABI Flag: Affects ABI feature A feature request or implementation standard library Area: Standard library umbrella swift evolution implemented Flag → feature: A feature that was approved through the Swift evolution process and implemented

Comments

@gribozavr
Copy link
Collaborator

Previous ID SR-1957
Radar rdar://problem/18589289
Original Reporter @gribozavr
Type Bug
Status Closed
Resolution Done
Additional Detail from JIRA
Votes 0
Component/s Standard Library
Labels Bug, AffectsABI, swift-evolution-proposal-needed
Assignee None
Priority Medium

md5: 0a2d4f61740d8666fd515693afde6a6b

Issue Description:

We are not aware of any real usecases for `unsafeAddressOf`. If there are any, it should be renamed to `unsafeAddress(of:)` to follow the guidelines.

NOTE: Requires a swift-evolution proposal.

@swift-ci
Copy link
Collaborator

swift-ci commented Jul 6, 2016

Comment by Charlie Monroe (JIRA)

Maybe I'm missing something, but I use it on several occasions:

  • for logging pointer address of the object (to distinguish between objects that may contain the same values, e.g. custom mutable C-String implementation)

  • pseudo-random generation - get an unsafe address of something on the stack, get distance from UnsafePointer<Void>(bitPattern: 0) and start with that as a partially random number - something that in C would be `int i; void *randPtr = &i;`

  • casting objects to be used with legacy APIs that take `void *` (UnsafePointer<Void>) as context argument (or even objc_getAssociatedObject).

  • adding Swift objects to CFArray (which again takes UnsafePointer<Void> as an argument).

Am I missing some initializer on UnsafePointer that takes AnyObject as parameter and returns UnsafePointer<Void>?

@gribozavr
Copy link
Collaborator Author

Thanks for the list!

> for logging pointer address of the object
> casting objects to be used with legacy APIs that take `void *`
> adding Swift objects to CFArray

Unmanaged covers these.

> pseudo-random generation - get an unsafe address of something on the stack

I'm afraid this is not a valid usecase. Leaking the address of the stack (or the heap) to a 3rd party (a potential attacker) is a security risk because it makes exploiting other vulnerabilities easier by defeating ASLR.

@swift-ci
Copy link
Collaborator

swift-ci commented Jul 6, 2016

Comment by Charlie Monroe (JIRA)

Ok, I can see how Unmanaged covers this, though it's a bit more verbose:

Unmanaged.passUnretained(x).toOpaque()

vs

unsafeAddress(of: x)

And I personally dislike that you need to deal with retained/unretained when all you are interested in is the address.

Ad the pseudo-random generator - it's not entirely leaking the address of the stack - as I've mentioned, this is used to get some base value, from which we can use e.g. the last few bits (or every second, etc.) which isn't indicative of anything on its own, yet is still enough random for most purposes since the value differs from launch to launch. But without the entire 64 bits, this is IMHO fairly useless to the attacker since the possible stack address range is still incredibly large (when talking about 64-bit architectures), even though it usually is in the higher end of the memory range.

@gribozavr
Copy link
Collaborator Author

> And I personally dislike that you need to deal with retained/unretained when all you are interested in is the address.

I understand the annotation burden, but it is critical to be explicit in the source what kind of reference kind semantics you want for this cast. For example, when attaching the object to another object through objc_setAssociatedObject, you need to use ".passRetained", not ".passUnretained".

> But without the entire 64 bits, this is IMHO fairly useless to the attacker since the possible stack address range is still incredibly large

ASLR only randomizes a few bits at the top of the address (not all 64 bits), our CPUs don't even support full 64-bit addresses now, e.g., AMD64 implementations support only 48 bits, and most simple PRNGs are trivially invertible. The risk is real.

> yet is still enough random for most purposes since the value differs from launch to launch

Why not use a seeding mechanism that was designed for this purpose? For example, reading from /dev/urandom, or using arc4random().

@swift-ci
Copy link
Collaborator

swift-ci commented Jul 6, 2016

Comment by Charlie Monroe (JIRA)

Ad objc_setAssociatedObject - for the value of the key, it shouldn't be retained - the run-time (last time I checked) doesn't care about whether the key points to an object or not, all that matters is its actual value which is usually achieved by a reference to something on the heap (usually a string constant):

/// ObjC:
static NSString const * NSManagedObjectContextXUSyncManagerKey  
                   = "NSManagedObjectContextXUSyncManagerKey"

objc_setAssociatedObject(obj, &NSManagedObjectContextXUSyncManagerKey, 
                   newValue, OBJC_ASSOCIATION_RETAIN)
/// Swift:
private let NSManagedObjectContextXUSyncManagerKey: NSString 
                 = "NSManagedObjectContextXUSyncManagerKey"

objc_setAssociatedObject(self, unsafeAddressOf(NSManagedObjectContextXUSyncManagerKey),
                 newValue, .OBJC_ASSOCIATION_RETAIN)

Reading from /dev/urandom suffers from:

  • performance issues. It's true that last time I did anything with random numbers, it was for iOS 4, so things might be different with current hardware, but the fact that each random number requires a syscall is a bit pricy.

  • it is documented that it will only contain something random if the OS has any data to supply - if you read a lot of random data from /dev/urandom, it either blocks your read, or returns the same number as before (I currently can't remember which it was).

But anyway, that is slightly off topic. I'd personally keep unsafeAddres(of🙂, but move it from a floating global function under Unamanged?

extension Unmanaged {
    func unsafeAddress(of: T) -> UnsafePointer<Void>
}

@gribozavr
Copy link
Collaborator Author

Sure, if you use an object with static lifetime, you don't need to retain it. It is not always the case that people use such objects. In that case, it is critical for code clarity to make the ownership semantics explicit.

> Reading from /dev/urandom suffers from:

Sorry, I don't see any of these reasons as compelling. If you need random data that's any good, you need to use appropriate APIs. If you don't care, you can use the current time. Not to even mention that the address you get from the stack is not even guaranteed to differ from run to run, ASLR can be off. You can just hardcode a constant of your choice as the seed, it would be as effective. Using current time would be a better choice. (Not a good one, on the global measure of things though; on iOS, arc4random() would be a good choice.)

> but the fact that each random number requires a syscall is a bit pricy

I was not suggesting to perform I/O for every random number, just for the seed. Also, on iOS, you have arc4random(), which you don't even need to seed yourself or worry about the I/O for the seed.

> it is documented that it will only contain something random if the OS has any data to supply

If you don't want to wait for the high-quality random data, you can use async I/O and just substitute a 0 as the seed when you hit your timeout. I don't understand why not just always use 0 as the seed though, if low-quality randomness is acceptable.

> But anyway, that is slightly off topic. I'd personally keep unsafeAddres(of, but move it from a floating global function under Unamanged?

I don't see what issue would this solve.

@swift-ci
Copy link
Collaborator

swift-ci commented Jul 6, 2016

Comment by Charlie Monroe (JIRA)

Fair points.

About the rename/move - it doesn't solve anything, but rather keeps the same functionality categorized rather than "floating around".

@swift-ci
Copy link
Collaborator

Comment by Charlie Monroe (JIRA)

This is now part of apple/swift-evolution#437

@lilyball
Copy link
Mannequin

lilyball mannequin commented Jul 27, 2016

charlieMonroe (JIRA User) was right about objc_setAssociatedObject - if you're passing a retained object as the key, you're leaking your object. objc_setAssociatedObject doesn't take an object for the key. It takes a pointer, and it uses strict pointer equality without ever dereferencing the pointer. This is why unsafeAddressOf was a good fit for it, because you want to discard the notion of passing an object at all, since you're only using the object as a source for a stable unique address.

@rintaro
Copy link
Mannequin

rintaro mannequin commented Nov 11, 2016

SE-0127 and PR: #3644

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@AnthonyLatsis AnthonyLatsis added feature A feature request or implementation swift evolution implemented Flag → feature: A feature that was approved through the Swift evolution process and implemented and removed swift evolution proposal needed Flag → feature: A feature that warrants a Swift evolution proposal bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. labels Mar 22, 2023
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects ABI Flag: Affects ABI feature A feature request or implementation standard library Area: Standard library umbrella swift evolution implemented Flag → feature: A feature that was approved through the Swift evolution process and implemented
Projects
None yet
Development

No branches or pull requests

3 participants