[Pitch] Opt-in Strict Memory Safety Checking

Evolution Pitches
1

Hello!

Following the discussion of the prospective vision for opt-in strict memory safety checking, here's a proposal that introduces the @unsafe and @safe(unchecked) attributes and strict memory safety checking mode.

Here's a quick summary from the proposal of how we expect it to work for users:

The UnsafeBufferPointer type will be marked with @unsafe in the Standard library, as will the other unsafe types (e.g., UnsafePointer, UnsafeRawPointer):

@unsafe 
public struct UnsafeBufferPointer<Element> { ... }

This indicates that use of this type is not memory-safe. Any declaration that has UnsafeBufferPointer as part of its type is also unsafe, and would produce a warning under this strict safety mode, e.g.,

extension Array {
  // warning on next line: reference to unsafe generic struct 'UnsafeBufferPointer'
  func withUnsafeBufferPointerSimplified<T>(_ body: (UnsafeBufferPointer<Element>) -> T) -> T {
    // ...
  }
}

This warning can be suppressed by marking the function as @unsafe:

extension Array {
  @unsafe
  func withUnsafeBufferPointerSimplified<T>(_ body: (UnsafeBufferPointer<Element>) -> T) -> T {
    // ...
  }
}

Users of this function that also enable strict safety checking will see warnings when using it. For example:

extension Array<Int> {
  func sum() -> Int {
    // warning: use of unsafe function 'withUnsafeBufferPointerSimplified'
    withUnsafeBufferPointerSimplified { buffer in
      c_library_sum_function(buffer.baseAddress, buffer.count, 0)
    }
  }
}

Both the call to withUnsafeBufferPointerSimplified (which is @unsafe) and the call to c_library_sum_function (which has a parameter of @unsafe type UnsafePointer) would trigger warnings about uses of unsafe constructs. The author of sum has a choice to suppress the warnings:

  1. Mark the sum function as @unsafe, propagating the "unsafe" checking out to callers of sum; or
  2. Mark the sum function as @safe(unchecked), taking responsibility for the safety of the code within the body. Here, one needs to verify that the UnsafeBufferPointer itself is being used safely (i.e., accesses are in-bounds and the buffer doesn't escape the closure) and that c_library_sum_function does the same with the pointer and bounds it is given.

Lots more details are in the proposal.

Doug

39 Likes
2

In general I'm fine with the proposal as long as it stays opt-in as it's mentioned in the text.
What I'm a bit concerned with, looking at the proposed use-case for critical corporate workloads, is the @safe(unchecked) attribute. I know that there are use cases where a library author uses Unsafe types in a safe way, but many issues only occur in edge-cases (speaking from my own experience with UnsafeRawBufferPointer which may not be considered by the library author.

They think their method is safe, mark them es such and it can be used in systems which enable the -strict-memory-safety. But if there are unsafe edge-cases hidden, it completely defeats the purpose of such attribute and users of the library would still need to audit the code manually to potentially find issues with the implementation.

I sadly don't have a good solution for this problem at the moment but it should be mentioned anyways.

But as said, in general I would approve of such proposal also with the concerns laid out if the remaining risk is clearly communicated in a way.

1 Like
3

+1 overall.

These warnings be in the group Unsafe (possibly organized into subgroups) so that one can choose to escalate them to errors

It's a shame though that this is opt-in, IMO it should be opt-out. Use of unsafe constructs should cause errors (not warnings) by default for the ecosystem to see real benefits from it. Similar to how Swift 6 exposed data safety bugs as compiler errors, as opposed to warnings in later versions of Swift 5.

3 Likes
4

I am wondering what would be the proposed behavior here:

// Module A, strict safety on
@unsafe
public struct DataWrapper {
  var buffer: UnsafeBufferPointer<UInt8>
  
  public func checksum() -> Int32 { ... }
}

// Module B, strict safety off
import A

public struct MyType {
  public var wrapper: DataWrapper
  public func checksum() -> Int32 {}
    return wrapper.checksum()
  }
}

// Module C, strict safety on
import A
import B

// Do we get any warnings here?
func foo(x : MyType) -> Int32 {
  return x.checksum()
}
5

I think making it opt-out could be a separate pitch in the future once we have more experience using this on real world projects.

3 Likes
6

Sure, but I'd probably make my support for the pitch conditional on a clear path towards making it opt-out. If it never becomes opt-out and stays in the opt-in stage indefinitely, there's little incentive for ecosystem-wide adoption.

7

As I understand it, the pitched vision is indeed that this is an opt-in mode forever, for the reasons outlined already. In particular, since seamless C interop is a tentpole feature of Swift, ecosystem-wide adoption of a mode that precludes C interop is a non-goal.

If, in practice, every use of an unsafe API must be annotated as @safe(unchecked) in order to exist in the Swift ecosystem, then the whole thing is just busywork and the annotation tells us nothing.

5 Likes
8

Practically speaking, I don't think we can solve this problem by omitting @safe(unchecked). It means pushing the unsafe code out to modules that don't enable -strict-memory-safety, and I expect the overall effect is that less code will be safe.

It's not in this proposal, but the vision document notes that we want auditability for the uses of @safe(unchecked) and non-strict modules. I think that's the mechanism for organizations to enforce stronger constraints than exist in the language itself.

The Alternatives Considered section talks about why I think this should be opt-in for the foreseeable future. I maintain that, for most Swift programmers, the default we have now is the right one.

I think we'll see real benefits in the ecosystem from social pressures so long as we make it available in a manner that is easy to adopt and doesn't get in people's way. The use of warnings + dedicated warning group approach, the design of @safe(unchecked), and the no-source-or-ABI-impact approach of this design is intended to allow this to spread where it needs to go quickly and easily without the big "this is part of a language mode" hammer.

No warnings anywhere. For a module that doesn't enable strict concurrency, it is as-if we inferred @unsafe on any declaration whose signature includes an @unsafe type and @safe(unchecked) on any declaration whose definition uses an unsafe construct (but its signature does not).

Doug

1 Like
9

I see. So the implication is, users might actually want/need all modules to be compiled with strict safety if they want guarantees that they do not touch unaudited unsafe constructs.

1 Like
10

Yes. This is another aspect of the "auditability" that I keep referring to, knowing whether the modules you depend on have enabled strict memory safety.

Doug

11

Hi all,

I realized that I failed to write down the rules for overriding and conformances. The proposal is updated with these two sections:

Unsafe overrides

Overriding a safe method within an @unsafe one could introduce unsafety, so it will produce a diagnostic in the strict safety mode:

class Super {
  func f() { }
}

class Sub: Super {
  @unsafe func f() { ... } // warning: override of safe instance method with unsafe instance method
}

to suppress this warning, the Sub class itself can be marked as @unsafe, e.g.,

@unsafe
class Sub: Super {
  func f() { ... } // warning: override of safe instance method with unsafe instance method
}

The @unsafe annotation is at the class level because any use of the Sub type can now introduce unsafe behavior, and any indication of that unsafe behavior will be lost once that Sub is converted to a Super instance.

Unsafe conformances

Implementing a protocol requirement that is not @unsafe (and not part of an @unsafe protocol) within an @unsafe declaration introduces unsafety, so it will produce a diagnostic in the strict safety mode:

protocol P {
  func f()
}

struct ConformsToP { }

extension ConformsToP: P {
  @unsafe func f() { } // warning: unsafe instance method 'f()' cannot satisfy safe requirement
}

To suppress this warning, one can place @unsafe on the extension (or type) where the conformance to P is supplied. This notes that the conformance itself is unsafe:

@unsafe
extension ConformsToP: P {
  @unsafe func f() { }
}

Doug

1 Like
12

Suggestion—

Existing precedent suggests that a modifier on an extension is a shorthand for modifying all contained members, and I definitely understand why that's undesirable for @unsafe.

But here we don't need that, because we already have a precedent for decorating the conformance itself:

extension ConformsToP: @unsafe P {
  @unsafe func f() { }
}

Perhaps it's desirable to extend the same approach to subclassing such that having unsafe overrides doesn't also mean that every member defined on Sub is also unsafe?

5 Likes
13

The proposal has some arguments for using @safe(unchecked) over unsafe blocks. I wanted to add some arguments for the opposite here for completeness.

  1. @safe(unchecked, message: "...") is an indication that we want to document why is it fine to use a certain unsafe construct. This is hard to do in a single message at the top of the function when we have multiple unsafe constructs in the same function. Moreover, these messages belong to the implementation, not to the interface. The caller only needs to know that the function is safe to use, and does not need to know why. I think it is more natural to have separate unsafe blocks for separate unsafe uses with separate messages.
  2. Would it be possible for users of unsafe blocks to define a macro that replaces unsafe with do when older compiler versions are used?
  3. Unsafe blocks are better for auditing changes to unsafe functions. Whenever a new unsafe construct is added, a new unsafe block appears. On the other hand if the whole body of the unsafe function is a free for all it is less obvious when changes add new unsafe constructs that need to be audited.
  4. Unsafe blocks are better for refactoring. If we only mark functions unsafe, factoring out a block of code or moving it to another function can start triggering new warnings. On the other hand, when the unsafe constructs are encapsulated in unsafe blocks, the moved code would not trigger new warnings.
  5. I wonder if the escape described in the pitch could actually be detected and warned about:
    let (ptr, count) = unsafe {
      withUnsafeBufferPointerSimplified { buffer in
        (buffer.baseAddress, buffer.count)
      }
    }

Here, I would expect ptr to have an @unsafe type, and it is outside of an unsafe block. So I'd expect this incorrectly encapsulated code to trigger a diagnostic.

Overall, I have a slight preference towards unsafe blocks but I am also OK with the current pitch as is.

4 Likes
14

For me it’s the exact opposite. As we’ve seen in the other prospective vision about a default non-concurrency mode, many Swift developers (especially new ones) are already overwhelmed by warnings and errors they don’t understand.

Although these 2 topics are different in a way that the datarace warnings are indeed completely unnecessary in code not utilizing concurrency and these warnings for unsafe types would be correct for every sort of code, I don’t see the necessity to warn a developer about a thing that might cause problems in some specific condition you don’t know.

Using the type directly already warns the user by the long type name and the Unsafe-prefix.

Using the type indirectly via an imported type using unsafe constructs as an implementation detail has two ways it can go:

  1. You crash every time you run the program/your program leaks significantly:
    You discover that in tests and will find out how to handle the API correctly to not make it happen.
  2. In some edge cases the use of unsafe types via an import causes issues:
    If you cannot live with it, opt into strict checking and build your code around it, if you are like the vast majority of Swift developers developing some small App, don’t care and just hope that the user doesn’t cause a stack overflow by putting to much data into an buffer on the background thread. It won’t be the only crash you encounter without having a big team behind a app hunting down every small bug.

Realistically, popular libraries handle unsafe types with the care they need, even without adding any attributes and you should not scare people away from libraries because a warning that this is unsafe might pop onto the screen just because the library dev missed to add an attribute to a var.

This should always stay an opt-in as the consequences only affect a really small part of programs significantly.

15

How does strict memory safety interact with macros that expand to use unsafe code?

16

Thank you, I'm going to incorporate some of these into the Alternatives Considered section to provide some more balance.

No, macros can't change structure in this way. I suppose you could create some expression macro that expands to an #if hasFeature(Unsafe) check and uses unsafe { ... }, or maybe do some trick where we have a global func named unsafe that takes a closure and executes it (but we'll run into problems with async and throws effects).

I guess? You can't limit your reasoning to just the new unsafe block, and need to consider it in the context of the whole function anyway.

I would have called that a good thing: if you're taking some unsafe code and putting it elsewhere, I'd like to see the warnings.

You know, my example is bogus. We would complain about the inferred type of ptr being @unsafe.

At the end of the day, it's "just" syntactic sugar---the core behavior of the proposal remains the same regardless of the mechanism we adopt for this suppression.

The same as if you copied in the expanded code, so a macro can introduce unsafe code but that would either need to be within a context that already deals with the unsafety (@unsafe or @safe(unchecked)). For macros that introduce declarations, they can make them @unsafe or @safe(unchecked) if needed. This does mean that just searching for @safe(unchecked) isn't a full audit mechanism.

Doug

17

Ah, of course! Thank you.

If we do this, we need to find the right place at which we can introduce safety checking. For example...

class Super {
  init() {
    f()
  }

  func f() { }
}

class Sub: @unsafe Super {
  @unsafe override func f() { }
}

Creating an instance of Sub() involves unsafe code, but I'm not sure where to diagnose it: the initializer we get from Super is safe, and calls f which was safe at the time, but the override is now unsafe. I suppose we could try to diagnose this at the point where we do the Sub -> Super or Sub.Type -> Super.Type implicit conversion, or really anywhere that we depend on subtyping. That's a bit harder to specify and implement.

Doug

1 Like
18

As I have mentioned previously, I believe custom executors are unsafe. A bug in a SerialExecutor implementation could lead to data races and there is no way for the compiler to statically verify otherwise.

I believe the following demonstrates UB that would not be caught by the proposed unsafe attribute, is that correct?

import Synchronization
import Foundation

final class BadSerialExec: SerialExecutor {

    let queue = Mutex<[UnownedJob]>([])

    nonisolated(unsafe) var threadPool: [Thread]!

    init() {
        threadPool = (0..<2).map { _ in
            Thread { [self] in
                while true {
                    if let nextJob = queue.withLock({ $0.popLast() }) {
                        nextJob.runSynchronously(on: self.asUnownedSerialExecutor())
                    }
                }
            }
        }

        threadPool!.forEach { $0.start() }
    }

    func enqueue(_ job: consuming ExecutorJob) {
        let job = UnownedJob(job)
        queue.withLock { $0.append(job) }
    }
}


actor MyActor {
    let executor = BadSerialExec()
    var value = 50

    nonisolated var unownedExecutor: UnownedSerialExecutor {
        executor.asUnownedSerialExecutor()
    }

    func reset() { value = 50 }
    func increment() { value += 1 }
    func decrement() { value -= 1 }
}

let instance = MyActor()

for _ in 0..<100 {
    await instance.reset()
    await withDiscardingTaskGroup { group in
        for _ in 0..<50 {
            group.addTask {
                try? await Task.sleep(nanoseconds: 200)
                await instance.increment()
            }
        }
        for _ in 0..<50 {
            group.addTask {
                try? await Task.sleep(nanoseconds: 200)
                await instance.decrement()
            }
        }
    }
    let result = await instance.value
    print("\(result) \(result != 50 ? "[!]" : "")")
}

If I comment out the custom actor executor and use the standard library's default one, it prints a string of 50s, every single time. If I leave it in, the obviously-wrong executor causes the actor to race on accesses to its internal state, resulting in values form 48 to 52.

(Okay, there is the nonisolated(unsafe) var threadPool, but that's just to make the init easier to write because the Thread body needs to capture self - it isn't where the UB comes from)

2 Likes
19

You did mention this previously, and I failed to incorporate it into my proposal. My apologies! Let's figure out how to handle this. One partial answer is here:

The UnownedSerialExecutor type should be marked @unsafe. From that, we get that unownedExecutor is also unsafe. Practically speaking, that means any actor with a custom serial executor is unsafe. We could say that you need to mark such actors as @unsafe, and maybe allow @safe(unchecked) as an alternative there that asserts that the use of custom executor is safe. Do we think that's enough, or do we need to move unsafety into the realm of SerialExecutor as well because a bad one could violate data-race safety?

Doug

1 Like
20

Well, I could imagine a library wanting to vend a @safe(unchecked) serial executor - for instance the DispatchSerialQueue.

We could potentially make the Unowned part of USE less dangerous by requiring it to return a lifetime-bound value, but we can never really validate the Serial part statically.

So an idea that seems reasonable to me is:

  1. Mark Actor.unownedExecutor as unsafe until it can return a lifetime-constrained value.

  2. Mark the SerialExecutor protocol as unsafe. Library authors will get the warning they need to pay extra attention, and mark their code as @safe(unchecked) if they are vending it as API.

The final result would be that user code returning a lifetime-bound serial executor which has been audited and marked @safe(unchecked) would just work without additional annotations.

2 Likes