[Sema] Diagnose unsound pointer conversions #27695

hamishknight · 2019-10-15T17:26:55Z

(This is an updated version of #20467)

In this PR is the logic to emit a diagnostic for unsound array-to-pointer, string-to-pointer, and inout-to-pointer conversions. This is done through the introduction of an underscored attribute @_nonEphemeral that denotes a pointer parameter which cannot accept pointer conversions that are only valid for the duration of the call they're passed to.

This includes all array-to-pointer and string-to-pointer conversions, and most inout-to-pointer conversions with the exception of:

Global and static variables
Stored properties on such variables that are accessed directly (e.g excluding ones with property observers, ones on classes, protocols, and resilient types in general)
Force unwraps of any of the above

Various pointer parameters in the stdlib have been marked as @_nonEphemeral, primarily pointer parameters for initialisers on pointer types. In addition, this PR infers @_nonEphemeral on pointer parameters for memberwise initialisers and enum case constructors.

By default, the diagnostic emitted will be warning. I have added a frontend flag to allow upgrading to an error, and hopefully this will allow us to transition to an error by default in a future language version.

Resolves SR-2790.

hamishknight · 2019-10-15T17:27:37Z

Given how large this PR is, I've split off the commits that can be landed separately with no intended functionality change: #27696. Once that has been landed, I can further peel off the main @_nonEphemeral implementation, leaving this PR with just the stdlib + @_nonEphemeral inference changes (last 2 commits).

hamishknight · 2019-10-15T17:28:00Z

@swift-ci please test

hamishknight · 2019-10-15T17:28:13Z

@swift-ci please test source compatibility

Results: Debug (UPASS) – Release (PASS).

swift-ci · 2019-10-15T18:00:10Z

Build failed
Swift Test Linux Platform
Git Sha - 6e6b044f4b31bfa8ea74528bc00412945f2c2929

swift-ci · 2019-10-15T19:56:08Z

Build failed
Swift Test OS X Platform
Git Sha - 6e6b044f4b31bfa8ea74528bc00412945f2c2929

A handful of independent changes from #27695

hamishknight · 2019-10-15T21:50:56Z

@swift-ci please test

hamishknight · 2019-10-15T22:06:41Z

I've peeled off the @_nonEphemeral implementation into #27705.

swift-ci · 2019-10-15T22:50:42Z

Build failed
Swift Test Linux Platform
Git Sha - 75184a074c8cd97bed48942a0044b4f8cf1587fc

hamishknight · 2019-10-16T02:11:52Z

@swift-ci please smoke test

rjmccall · 2019-10-17T00:50:05Z

FWIW, this is essentially an escape analysis and @_nonEphemeral is essentially @escaping applied to pointers. I'm not saying we should necessarily adopt that spelling here, since presumably we don't intend to make a complete and safe pointer model around UnsafePointer, but that's what it is.

hamishknight · 2019-10-17T17:19:08Z

@rjmccall Yup, I wanted to keep this distinct from @escaping for a couple of reasons:

Given this isn't an official feature, I wanted to make sure the attribute was non-user-facing, so we don't expose it in things like code completion or allow the user to start marking their pointer parameters as @escaping. I also wanted to make sure this attribute doesn't get considered in things like name mangling so it doesn't affect ABI.
@_nonEphemeral can currently be used to mark other kinds of pointer parameters that don't escape the pointer passed, but still shouldn't accept temporary pointer conversions. For example, one such parameter in this PR is:
```
moveInitialize(@_nonEphemeral from source: UnsafeMutablePointer, count: Int)
```
as passing a temporary pointer conversion here could lead to over releasing the pointee.
As you say, I don't think we intend to build a fully safe pointer model. In particular, applying the same restrictions as non-escaping function parameters to pointer parameter would almost certainly be too source breaking.

atrick

This is immensely exciting to me! These checks will definitely catch a massive amount of nasty C interop bugs. By combining these sort of diagnostics with some Swift-for-C-Programmer documentation we can resolve some major pain points. I hope we can turn the diagnostics into errors in the release after next. Should we have a bug for that?

@rjmccall mentioned an escape analysis. I don't see any such thing in this PR, except that the manual additional of @nonEphemeral could be subsumed by a more general analysis. Eventually we should handle general escaping pointers, such as rdar://problem/25001764 Detect when a temporary pointer to a writeback buffer is persisted after the function returns:

func takesPtr(_ ptr: UnsafePointer<UInt64>) {}

private func _ptr(_ ptr: UnsafePointer<UInt64>) -> UnsafePointer<UInt64> {
  return UnsafePointer<UInt64>(ptr)
}

var x = UInt64(0)
takesPtr(_ptr(&x))

At that level, the diagnostic should be a SIL pass. We've been moving diagnostics that depend on dataflow analysis out of the type checker and into SIL passes where they can be expressed more robustly and without adding type checker constraints. I suspect the thing to do at that point would be to layer the diagnostics, so most of the logic in this PR stays in the type checker with more difficult cases handled later. At the SIL level, I don't think we need the @nonEphemeral attribute, since the job of the analysis is to infer that.

I am curious though... is it necessary for the precision of these diagnostics to be part of the constraint system? I don't see why it would be. In fact, it I seems to me that we do not want pointer lifetime mistakes to affect overload resolution. If I'm missing something, please clarify.

I really don't understand all the implications of adding constraints to the type checker, but overall your implementation looks extremely thorough and high quality.

hamishknight · 2019-10-22T19:52:59Z

@atrick Thanks for the review! It would be great to see this combined with a SIL pass to diagnose more tricky cases, and I'd certainly be happy to look into that as a potential follow-up :)

I hope we can turn the diagnostics into errors in the release after next. Should we have a bug for that?

Sure thing, I'll file one after this gets merged.

I am curious though... is it necessary for the precision of these diagnostics to be part of the constraint system? I don't see why it would be. In fact, it I seems to me that we do not want pointer lifetime mistakes to affect overload resolution. If I'm missing something, please clarify.

It's not entirely necessary for this to be a part of the constraint system, though it does allow us to emit better diagnostics for things like ambiguities, where we can inform the user that none of the candidates are viable because they don't accept temporary pointer conversions.

It will also allow for composition with other constraint system diagnostics, so we can tell the user earlier that what they're trying to do is invalid. Unfortunately this doesn't work so well at the moment as we discard additional fixes with the same locator, but eventually it would be nice to emit a temporary pointer error on the following along with an optional unwrap error:

func foo() {
  var x: Int?
  let ptr = UnsafePointer<Int>(&x)
}

I do agree that ideally @_nonEphemeral probably shouldn't affect overload resolution, though I don't think this should be a major issue in practice. One slight benefit is that it can help uncover declarations that should have been marked @_nonEphemeral in the first place.

hamishknight · 2019-10-22T19:56:14Z

If @xedin and @jrose-apple are also happy to review this as a whole, then I'll close #27705.

jrose-apple

The non-ConstraintSystem parts seem good, but I'm also pretty unhappy with this being part of overload resolution. It feels like something that MiscDiagnostics or even SIL diagnostics should handle instead—after we've type-checked everything, find out if we relied on an ephemeral pointer.

If you want to keep going with this approach, @xedin (or @hborla) should review the ConstraintSystem parts.

include/swift/AST/Attr.def

include/swift/AST/DiagnosticsSema.def

jrose-apple · 2019-10-30T21:31:27Z

include/swift/AST/DiagnosticsSema.def

+// @_nonEphemeral conversion diagnostics
+ERROR(cannot_pass_type_to_non_ephemeral,none,
+      "cannot pass %0 to parameter; argument #%1 must be a pointer that "
+      "outlives the call%select{| to %3}2", (Type, unsigned, bool, DeclName))


This is okay but it'd be nice to get the names in here too (see argument_out_of_order_named_named). That can be follow-up work for sure, though.

@jrose-apple Good idea! I'll do this as a follow-up, as we'll probably want to add a new "get argument label" API to ArgumentMismatchFailure and FunctionArgApplyInfo.

include/swift/Option/FrontendOptions.td

lib/Sema/CSFix.cpp

stdlib/public/core/UnsafePointer.swift

hamishknight · 2019-10-30T23:11:04Z

@jrose-apple Thanks for the review! I'll go ahead and make those changes. Note that @xedin has reviewed the constraint system parts of this over on #27705.

Regarding the fact that @_nonEphemeral affects overload resolution, I guess one simple solution would to change the TreatEphemeralAsNonEphemeral fix such that it doesn't affect the SK_Fix score of the solution (like warning fixes currently do, but we'd then emit an error). This would allow us to benefit from composition with other constraint system diagnostics without affecting overload resolution. Does this sound reasonable @xedin?

hamishknight · 2019-10-31T19:32:34Z

Thanks again everyone for the review! I've gone ahead and changed things such the diagnostic no longer affects overload resolution. I've also rebased to resolve a bunch of merge conflicts.

If possible I'd like to keep this logic a part of the constraint system, as it's much easier to detect and attach a fix there when evaluating the various X-to-pointer conversions rather than attempting to traverse the AST in MiscDiagnostics.

hamishknight · 2019-10-31T19:33:14Z

@swift-ci please test

hamishknight · 2019-10-31T19:33:20Z

@swift-ci please test source compatibility

lib/Sema/CSSimplify.cpp

jrose-apple · 2019-11-01T16:38:00Z

The thing I like about MiscDiagnostics is that it's not mixed up with deciding how a program behaves. The ConstraintSystem decides what the user said, and MiscDiagnostics figures out whether it's actually what they meant or actually allowed. You could sink all of that down to SIL, I guess, but I'm not sure what benefit you'd get.

In my head, MiscDiagnostics would ideally be a pluggable visitor the way SIL optimizer passes are pluggable; if you have an expression-level diagnostic, you just add it to the visitor. Then the visitor does a single walk of the AST. (I might be biased by my static analyzer past; the analyzer's "checkers" work a lot like this.)

atrick · 2019-11-01T16:45:24Z

The purpose of sinking the diagnostic into SIL is to leverage being able to infer @nonEphemeral via an escape analysis of the pointer value.

jrose-apple · 2019-11-01T18:06:39Z

Hamish is right: non-ephemerality is a property of the function even as the body changes, so it can't be inferred from the body. (In particular, if a function happens to not escape a pointer today, it should still be able to reserve the privilege to do so.)

atrick · 2019-11-02T02:08:19Z

When @nonEphemeral is used as an API annotation I don't see an advantage of diagnosing it in SIL. I do like the idea that the diagnostic provably cannot affecting type resolution in any other way.

This non-user-facing attribute is used to denote pointer parameters which do not accept pointers produced from temporary pointer conversions such as array-to-pointer, string-to-pointer, and in some cases inout-to-pointer.

Diagnose ephemeral conversions that are passed to @_nonEphemeral parameters. Currently, this defaults to a warning with a frontend flag to upgrade to an error. Hopefully this will become an error by default in a future language version.

These include the pointer-to-pointer and pointer-to-buffer-pointer initialiser parameters amongst a couple of others, such as `Unmanaged.fromOpaque`, and the source for the `move[...]` family of methods.

These include memberwise initializers for pointer properties and enum case constructors for pointer payloads. In both cases, the pointer is being immediately escaped, so don't allow the user to pass a temporary pointer value.

This commit changes the behaviour of the error for passing a temporary pointer conversion to an @_nonEphemeral parameter such that it doesn't affect overload resolution. This is done by recording the fix with an impact of zero, meaning that we don't touch the solution's score. In addition, this change means we no longer need to perform the ranking hack where we favour array-to-pointer, as the disjunction short-circuiting will continue to happen even with the fix recorded.

Currently this does nothing, as we're warning by default, but once we error by default, this will downgrade the diagnostic to a warning.

hamishknight · 2019-11-03T19:15:12Z

Just rebased to resolve a merge conflict. @xedin @atrick if it's okay with you, I'd like to get this merged sometime Monday so we can get this into 5.2. I'm more than happy to make any further changes and cherry-pick to 5.2 if needed.

hamishknight · 2019-11-03T19:15:54Z

@swift-ci please test

hamishknight · 2019-11-03T21:38:58Z

@swift-ci please test Linux platform

xedin · 2019-11-04T02:48:43Z

@hamishknight I'm fine with this, go ahead and merge.

hamishknight · 2019-11-04T23:40:06Z

@swift-ci please smoke test

hamishknight · 2019-11-05T01:18:48Z

I checked, and @atrick is also happy for this to be merged.

hamishknight · 2019-11-05T01:20:39Z

Thanks everyone! I'll work on adding argument labels to the diagnostics, as well as a changelog entry as follow-ups.

hamishknight mentioned this pull request Oct 15, 2019

A handful of independent changes from #27695 #27696

Merged

hamishknight mentioned this pull request Oct 15, 2019

[Sema] Warn on unsound pointer conversions #20467

Closed

hamishknight added a commit that referenced this pull request Oct 15, 2019

A handful of independent changes from #27695 (#27696)

3818a16

A handful of independent changes from #27695

hamishknight force-pushed the health-and-safety-pointers-repilot branch from 6e6b044 to 75184a0 Compare October 15, 2019 21:50

hamishknight mentioned this pull request Oct 15, 2019

[Sema] Implement @_nonEphemeral #27705

Closed

hamishknight force-pushed the health-and-safety-pointers-repilot branch from 75184a0 to cc6fddd Compare October 16, 2019 02:10

natecook1000 requested a review from atrick October 16, 2019 23:02

atrick approved these changes Oct 22, 2019

View reviewed changes

hamishknight requested review from xedin and jrose-apple October 22, 2019 19:55

jrose-apple reviewed Oct 30, 2019

View reviewed changes

hamishknight force-pushed the health-and-safety-pointers-repilot branch from 9a22926 to 01c4c09 Compare October 31, 2019 19:13

xedin reviewed Oct 31, 2019

View reviewed changes

lib/Sema/CSSimplify.cpp Outdated Show resolved Hide resolved

swiftlang deleted a comment from swift-ci Oct 31, 2019

hamishknight and others added 8 commits November 3, 2019 08:40

[AST] Add @_nonEphemeral parameter attribute

92c42ec

This non-user-facing attribute is used to denote pointer parameters which do not accept pointers produced from temporary pointer conversions such as array-to-pointer, string-to-pointer, and in some cases inout-to-pointer.

[Sema] Diagnose unsound pointer conversions

7077a68

Diagnose ephemeral conversions that are passed to @_nonEphemeral parameters. Currently, this defaults to a warning with a frontend flag to upgrade to an error. Hopefully this will become an error by default in a future language version.

[CSDiagnostics] Add ephemeralness ambiguity notes

d0dbbf9

[stdlib] Mark some parameters @_nonEphemeral

1bc56dc

These include the pointer-to-pointer and pointer-to-buffer-pointer initialiser parameters amongst a couple of others, such as `Unmanaged.fromOpaque`, and the source for the `move[...]` family of methods.

[Sema] Infer @_nonEphemeral for various parameters

8cccbe0

These include memberwise initializers for pointer properties and enum case constructors for pointer payloads. In both cases, the pointer is being immediately escaped, so don't allow the user to pass a temporary pointer value.

Add a -disable-invalid-ephemeralness-as-error flag

998434f

Currently this does nothing, as we're warning by default, but once we error by default, this will downgrade the diagnostic to a warning.

Address other review feedback

65dda6d

hamishknight force-pushed the health-and-safety-pointers-repilot branch from 9b09611 to 65dda6d Compare November 3, 2019 19:00

swiftlang deleted a comment from swift-ci Nov 3, 2019

hamishknight merged commit 4023199 into swiftlang:master Nov 5, 2019

hamishknight deleted the health-and-safety-pointers-repilot branch November 5, 2019 01:19

hamishknight mentioned this pull request Nov 6, 2019

[Changelog] Add entry for unsound pointer diagnostics #28109

Merged

hamishknight mentioned this pull request Apr 16, 2020

[stdlib] Add a couple of missing @_nonEphemeral attributes #31075

Merged

hamishknight mentioned this pull request Nov 5, 2019

[SR-2790] Reject UnsafePointer initialization via implicit pointer conversion. swiftlang/swift-corelibs-foundation#4326

Closed

[Sema] Diagnose unsound pointer conversions #27695

[Sema] Diagnose unsound pointer conversions #27695

Uh oh!

Conversation

hamishknight commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hamishknight commented Oct 15, 2019

Uh oh!

hamishknight commented Oct 15, 2019

Uh oh!

hamishknight commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-ci commented Oct 15, 2019

Uh oh!

swift-ci commented Oct 15, 2019

Uh oh!

hamishknight commented Oct 15, 2019

Uh oh!

hamishknight commented Oct 15, 2019

Uh oh!

swift-ci commented Oct 15, 2019

Uh oh!

hamishknight commented Oct 16, 2019

Uh oh!

rjmccall commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hamishknight commented Oct 17, 2019

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

hamishknight commented Oct 22, 2019

Uh oh!

hamishknight commented Oct 22, 2019

Uh oh!

jrose-apple left a comment • 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!

Uh oh!

Uh oh!

jrose-apple Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

hamishknight Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hamishknight commented Oct 30, 2019

Uh oh!

hamishknight commented Oct 31, 2019

Uh oh!

hamishknight commented Oct 31, 2019

Uh oh!

hamishknight commented Oct 31, 2019

Uh oh!

Uh oh!

jrose-apple commented Nov 1, 2019

Uh oh!

atrick commented Nov 1, 2019

Uh oh!

jrose-apple commented Nov 1, 2019

Uh oh!

atrick commented Nov 2, 2019

Uh oh!

hamishknight commented Nov 3, 2019

Uh oh!

hamishknight commented Nov 3, 2019

Uh oh!

hamishknight commented Nov 3, 2019

Uh oh!

xedin commented Nov 4, 2019

Uh oh!

hamishknight commented Nov 4, 2019

Uh oh!

hamishknight commented Oct 15, 2019 •

edited

Loading

hamishknight commented Oct 15, 2019 •

edited

Loading

rjmccall commented Oct 17, 2019 •

edited

Loading

jrose-apple left a comment •

edited

Loading