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

Jump to bottom

[SR-11315] Unsafe[Mutable][Raw]Pointer constructors are never safe when combined with implicit pointer casting. #53716

Closed
Lukasa opened this issue Aug 16, 2019 · 3 comments
Closed

[SR-11315] Unsafe[Mutable][Raw]Pointer constructors are never safe when combined with implicit pointer casting. #53716

Lukasa opened this issue Aug 16, 2019 · 3 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior.

Comments

@Lukasa
Copy link
Contributor

Lukasa commented Aug 16, 2019

Previous ID SR-11315
Radar rdar://problem/54386877
Original Reporter @Lukasa
Type Bug
Status Resolved
Resolution Duplicate
Additional Detail from JIRA
Votes 0
Component/s
Labels Bug
Assignee None
Priority Medium

md5: d970a0bab5ab8b40872874cd7a734ff7

duplicates:

  • SR-2790 Reject UnsafePointer initialization via implicit pointer conversion.

Issue Description:

Some types in the Swift standard library, such as Array or String, are capable of being implicitly promoted to pointer types. This is intended as a compatibility feature for working with C programs.

However, this implicit promotion behaviour also works for programs written in Swift: any function in Swift that takes an UnsafePointer of appropriate type can work with these stdlib types. This turns out to play poorly with the constructors of the Unsafe[Mutable][Raw]Pointer types.

There is no published rule on how long the pointer derived from the implicit promotion is valid. John's writeup on the implicit pointer conversions suggests that the answer is that the scope of the conversion is for "immediate access", but I don't think this is a well-defined duration either.

Regardless, the observed behaviour of this rule is that explicitly passing an Array or a String to the constructor of an Unsafe[Mutable][Raw]Pointer is never correct in a Swift program. The effect of doing this is to immediately produce a dangling pointer. This can be observed in the following program:

func foo(_ t: UnsafeRawPointer) -> Int {
    let ptr = UnsafeMutableRawBufferPointer.allocate(byteCount: 1024, alignment: 1)
    ptr.copyMemory(from: UnsafeRawBufferPointer(start: t, count: 1024))
    return 1024
}

@inline(never)
func test() {
    foo(UnsafeRawPointer(Array(repeating: 1, count: 1_000_000)))
}

test()

This program takes an array, creates a raw pointer from it, and passes that raw pointer to a function. The function copies bytes out of that pointer.

When compiled with {{ -sanitize=address }}, this program immediately fails:

=================================================================
==29890==ERROR: AddressSanitizer: heap-use-after-free on address 0x000111c64820 at pc 0x00010d92ce52 bp 0x7ffee23369b0 sp 0x7ffee2336160
READ of size 1024 at 0x000111c64820 thread T0
    #​0 0x10d92ce51 in __asan_memcpy (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x5de51)
    #​1 0x10d8c9e87 in test() (test:x86_64+0x100000e87)
    #​2 0x10d8c9c78 in main (test:x86_64+0x100000c78)
    #​3 0x7fff711b32a4 in start (libdyld.dylib:x86_64+0x112a4)

0x000111c64820 is located 32 bytes inside of 8000032-byte region [0x000111c64800,0x000112405a20)
freed by thread T0 here:
    #​0 0x10d92f98d in wrap_free (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x6098d)
    #​1 0x7fff708eefaf in _swift_release_dealloc (libswiftCore.dylib:x86_64+0x2cefaf)
    #​2 0x10d8c9e66 in test() (test:x86_64+0x100000e66)
    #​3 0x10d8c9c78 in main (test:x86_64+0x100000c78)
    #​4 0x7fff711b32a4 in start (libdyld.dylib:x86_64+0x112a4)

previously allocated by thread T0 here:
    #​0 0x10d92f7d3 in wrap_malloc (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x607d3)
    #​1 0x7fff708edb98 in swift_slowAlloc (libswiftCore.dylib:x86_64+0x2cdb98)
    #&#8203;2 0x7fff708edc13 in _swift_allocObject_(swift::TargetHeapMetadata<swift::InProcess> const*, unsigned long, unsigned long) (libswiftCore.dylib:x86_64+0x2cdc13)
    #&#8203;3 0x7fff7062f2ae in _ContiguousArrayBuffer.init(_uninitializedCount:minimumCapacity:) (libswiftCore.dylib:x86_64+0xf2ae)
    #&#8203;4 0x10d8c9ce1 in test() (test:x86_64+0x100000ce1)
    #&#8203;5 0x10d8c9c78 in main (test:x86_64+0x100000c78)
    #&#8203;6 0x7fff711b32a4 in start (libdyld.dylib:x86_64+0x112a4)

SUMMARY: AddressSanitizer: heap-use-after-free (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x5de51) in __asan_memcpy
Shadow bytes around the buggy address:
  0x10002238c8b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x10002238c8c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x10002238c8d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x10002238c8e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x10002238c8f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x10002238c900: fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd
  0x10002238c910: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x10002238c920: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x10002238c930: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x10002238c940: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x10002238c950: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==29890==ABORTING
[1]    29890 abort      ./test

The only way to understand the effect of this code is to observe that the pointer created from the Array is valid only for the duration of the function call for which the conversion occurred. The bug goes away if the program is changed to implicitly promote Array to UnsafeRawPointer:

func foo(_ t: UnsafeRawPointer) -> Int {
    let ptr = UnsafeMutableRawBufferPointer.allocate(byteCount: 1024, alignment: 1)
    ptr.copyMemory(from: UnsafeRawBufferPointer(start: t, count: 1024))
    return 1024
}

@inline(never)
func test() {
    foo(Array(repeating: 1, count: 1_000_000))
}

test()

This program becomes memory correct.

While the wider question of implicit pointer promotion is a difficult one to address, I think we should take action to address the fact that explicitly constructing a pointer from an Array or a String is never correct: the constructed pointer is invalid the moment it is able to be used.

I think we can address this specific problem by adding explicit constructors to the UnsafePointer types that take String or Array, and that trigger compile warnings or errors. This would eliminate an entire class of bugs to do with this use-case.
@Lukasa
Copy link
Contributor Author

Lukasa commented Aug 16, 2019

@swift-ci create

@hamishknight
Copy link
Collaborator

FYI I've been working on a fix for this by introducing an @_nonEphemeral attribute for pointer parameters that cannot accept such temporary conversions. I opened a PR a while ago (#20467 and am hoping to open an updated version within the next few weeks or so.

Should we mark this bug as a dupe of https://bugs.swift.org/browse/SR-2790?

@Lukasa
Copy link
Contributor Author

Lukasa commented Aug 16, 2019

Fine by me!

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Lukasa @hamishknight