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-2790] Reject UnsafePointer initialization via implicit pointer conversion. #4326

Closed
swift-ci opened this issue Sep 29, 2016 · 15 comments
Closed

Comments

@swift-ci
Copy link
Contributor

swift-ci commented Sep 29, 2016

Previous ID SR-2790
Radar rdar://problem/31232450
Original Reporter sclukey (JIRA User)
Type Bug
Status Resolved
Resolution Done
Environment

macOS: 10.11.6 (15G1004)
Swift: 3.0 (swiftlang-800.0.46.2 clang-800.0.38)

Additional Detail from JIRA
Votes 1
Component/s Foundation, Standard Library
Labels Bug
Assignee @hamishknight
Priority Medium

md5: 12bfaf656d95062691f07a1deaa3aa0d

is duplicated by:

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

relates to:

  • SR-9565 UnsafeMutablePointer on observed property of struct

Issue Description:

It seems perhaps that putting an UnsafePointer into an array somehow mangles the data. It also happens with UnsafeMutablePointer(mutating: ). I'm not sure whats going on so I'll just show an example.

I would expect the following code to print "swift", but instead it prints nonsense, generally two or three characters though: examples outputs include "P�", " -", "`[".

import Foundation

var strings:[UnsafePointer<Int8>] = []
strings.append(UnsafePointer<Int8>("swift".cString(using:String.Encoding.ascii))!)
print(String(cString:strings[0]))
@belkadan
Copy link

belkadan commented Sep 29, 2016

Reproduced. It sounds like the C string is getting deallocated, which I wouldn't expect because I thought cString(using:) put the string in an autorelease pool. @phausler?

@phausler
Copy link
Member

phausler commented Sep 29, 2016

   var strings:[UnsafePointer<Int8>] = []
let string = NSString(string: "swift")
strings.append(UnsafePointer<Int8>(string.cString(using:String.Encoding.ascii.rawValue))!)
print(String(cString:strings[0]))

This works as expected; so I suspect that the bridge is autoreleasing the NSString conversion when calling cString

@phausler
Copy link
Member

phausler commented Sep 29, 2016

   var strings:[UnsafePointer<Int8>] = []
let bytes = "swift".cString(using:String.Encoding.ascii)
strings.append(UnsafePointer<Int8>(bytes)!)
print(String(cString:strings[0]))

works as well...

@belkadan
Copy link

belkadan commented Sep 29, 2016

Ooh, interesting. The NSString owns the cString result? It's not an independent entry?

@phausler
Copy link
Member

phausler commented Sep 29, 2016

ASAN shows some interesting results here as well:

AddressSanitizer debugger support is active. Memory error breakpoint has been installed and you can now use the 'memory history' command.
=================================================================
==49378==ERROR: AddressSanitizer: heap-use-after-free on address 0x60400000cb30 at pc 0x0001008a1ac2 bp 0x7fff5fbff1d0 sp 0x7fff5fbfe990
READ of size 6 at 0x60400000cb30 thread T0
    #&#8203;0 0x1008a1ac1 in wrap_strlen (libclang_rt.asan_osx_dynamic.dylib+0x45ac1)
    #&#8203;1 0x1000e3990 in String.init(cString : UnsafePointer<Int8>) -> String (GarbageStrings+0x1000e3990)
    #&#8203;2 0x100001d7a in main main.swift:14
    #&#8203;3 0x7fff97179254 in start (libdyld.dylib+0x5254)

0x60400000cb30 is located 32 bytes inside of 38-byte region [0x60400000cb10,0x60400000cb36)
freed by thread T0 here:
    #&#8203;0 0x1008a6e29 in wrap_free (libclang_rt.asan_osx_dynamic.dylib+0x4ae29)
    #&#8203;1 0x100001bce in main main.swift:13
    #&#8203;2 0x7fff97179254 in start (libdyld.dylib+0x5254)

previously allocated by thread T0 here:
    #&#8203;0 0x1008a6c60 in wrap_malloc (libclang_rt.asan_osx_dynamic.dylib+0x4ac60)
    #&#8203;1 0x10030fc28 in swift_slowAlloc (GarbageStrings+0x10030fc28)
    #&#8203;2 0x10030fc73 in _swift_allocObject_ (GarbageStrings+0x10030fc73)
    #&#8203;3 0x100159517 in specialized specialized _ContiguousArrayBuffer.init(uninitializedCount : Int, minimumCapacity : Int) -> _ContiguousArrayBuffer<A> (GarbageStrings+0x100159517)
    #&#8203;4 0x10010864e in specialized specialized static Array._allocateBufferUninitialized(minimumCapacity : Int) -> _ArrayBuffer<A> (GarbageStrings+0x10010864e)
    #&#8203;5 0x10027fce2 in specialized specialized Array.init(repeating : A, count : Int) -> [A] (GarbageStrings+0x10027fce2)
    #&#8203;6 0x1000e49fe in _persistCString(UnsafePointer<Int8>?) -> [Int8]? (GarbageStrings+0x1000e49fe)
    #&#8203;7 0x100082abd in String.cString(using : String.Encoding) -> [Int8]? (GarbageStrings+0x100082abd)
    #&#8203;8 0x100001728 in main main.swift:13
    #&#8203;9 0x7fff97179254 in start (libdyld.dylib+0x5254)

SUMMARY: AddressSanitizer: heap-use-after-free (libclang_rt.asan_osx_dynamic.dylib+0x45ac1) in wrap_strlen
Shadow bytes around the buggy address:
  0x1c0800001910: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
  0x1c0800001920: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
  0x1c0800001930: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
  0x1c0800001940: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
  0x1c0800001950: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
=>0x1c0800001960: fa fa fd fd fd fd[fd]fa fa fa 00 00 00 00 00 fa
  0x1c0800001970: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
  0x1c0800001980: fa fa fd fd fd fd fd fd fa fa 00 00 00 00 00 00
  0x1c0800001990: fa fa 00 00 00 00 00 fa fa fa 00 00 00 00 00 00
  0x1c08000019a0: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
  0x1c08000019b0: fa fa fd fd fd fd fd fd fa fa 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
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  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
2016-09-29 10:32:20.248234 GarbageStrings[49378:4409233] =================================================================
2016-09-29 10:32:20.248471 GarbageStrings[49378:4409233] ==49378==ERROR: AddressSanitizer: heap-use-after-free on address 0x60400000cb30 at pc 0x0001008a1ac2 bp 0x7fff5fbff1d0 sp 0x7fff5fbfe990
2016-09-29 10:32:20.248489 GarbageStrings[49378:4409233] READ of size 6 at 0x60400000cb30 thread T0
2016-09-29 10:32:20.248504 GarbageStrings[49378:4409233]     #&#8203;0 0x1008a1ac1 in wrap_strlen (libclang_rt.asan_osx_dynamic.dylib+0x45ac1)
2016-09-29 10:32:20.248518 GarbageStrings[49378:4409233]     #&#8203;1 0x1000e3990 in String.init(cString : UnsafePointer<Int8>) -> String (GarbageStrings+0x1000e3990)
2016-09-29 10:32:20.248532 GarbageStrings[49378:4409233]     #&#8203;2 0x100001d7a in main main.swift:14
2016-09-29 10:32:20.248546 GarbageStrings[49378:4409233]     #&#8203;3 0x7fff97179254 in start (libdyld.dylib+0x5254)
2016-09-29 10:32:20.248559 GarbageStrings[49378:4409233] 
2016-09-29 10:32:20.248580 GarbageStrings[49378:4409233] 0x60400000cb30 is located 32 bytes inside of 38-byte region [0x60400000cb10,0x60400000cb36)
2016-09-29 10:32:20.248595 GarbageStrings[49378:4409233] freed by thread T0 here:
2016-09-29 10:32:20.253460 GarbageStrings[49378:4409233]     #&#8203;0 0x1008a6e29 in wrap_free (libclang_rt.asan_osx_dynamic.dylib+0x4ae29)
2016-09-29 10:32:20.253491 GarbageStrings[49378:4409233]     #&#8203;1 0x100001bce in main main.swift:13
2016-09-29 10:32:20.253507 GarbageStrings[49378:4409233]     #&#8203;2 0x7fff97179254 in start (libdyld.dylib+0x5254)
2016-09-29 10:32:20.253520 GarbageStrings[49378:4409233] 
2016-09-29 10:32:20.253533 GarbageStrings[49378:4409233] previously allocated by thread T0 here:
2016-09-29 10:32:20.253547 GarbageStrings[49378:4409233]     #&#8203;0 0x1008a6c60 in wrap_malloc (libclang_rt.asan_osx_dynamic.dylib+0x4ac60)
2016-09-29 10:32:20.253561 GarbageStrings[49378:4409233]     #&#8203;1 0x10030fc28 in swift_slowAlloc (GarbageStrings+0x10030fc28)
2016-09-29 10:32:20.253575 GarbageStrings[49378:4409233]     #&#8203;2 0x10030fc73 in _swift_allocObject_ (GarbageStrings+0x10030fc73)
2016-09-29 10:32:20.253602 GarbageStrings[49378:4409233]     #&#8203;3 0x100159517 in specialized specialized _ContiguousArrayBuffer.init(uninitializedCount : Int, minimumCapacity : Int) -> _ContiguousArrayBuffer<A> (GarbageStrings+0x100159517)
2016-09-29 10:32:20.261445 GarbageStrings[49378:4409233]     #&#8203;4 0x10010864e in specialized specialized static Array._allocateBufferUninitialized(minimumCapacity : Int) -> _ArrayBuffer<A> (GarbageStrings+0x10010864e)
2016-09-29 10:32:20.261472 GarbageStrings[49378:4409233]     #&#8203;5 0x10027fce2 in specialized specialized Array.init(repeating : A, count : Int) -> [A] (GarbageStrings+0x10027fce2)
2016-09-29 10:32:20.261487 GarbageStrings[49378:4409233]     #&#8203;6 0x1000e49fe in _persistCString(UnsafePointer<Int8>?) -> [Int8]? (GarbageStrings+0x1000e49fe)
2016-09-29 10:32:20.261501 GarbageStrings[49378:4409233]     #&#8203;7 0x100082abd in String.cString(using : String.Encoding) -> [Int8]? (GarbageStrings+0x100082abd)
2016-09-29 10:32:20.261515 GarbageStrings[49378:4409233]     #&#8203;8 0x100001728 in main main.swift:13
2016-09-29 10:32:20.261539 GarbageStrings[49378:4409233]     #&#8203;9 0x7fff97179254 in start (libdyld.dylib+0x5254)
2016-09-29 10:32:20.301910 GarbageStrings[49378:4409233] 
2016-09-29 10:32:20.301937 GarbageStrings[49378:4409233] SUMMARY: AddressSanitizer: heap-use-after-free (libclang_rt.asan_osx_dynamic.dylib+0x45ac1) in wrap_strlen
2016-09-29 10:32:20.301953 GarbageStrings[49378:4409233] Shadow bytes around the buggy address:
2016-09-29 10:32:20.301967 GarbageStrings[49378:4409233]   0x1c0800001910: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
2016-09-29 10:32:20.301981 GarbageStrings[49378:4409233]   0x1c0800001920: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
2016-09-29 10:32:20.301995 GarbageStrings[49378:4409233]   0x1c0800001930: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
2016-09-29 10:32:20.302008 GarbageStrings[49378:4409233]   0x1c0800001940: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
2016-09-29 10:32:20.302023 GarbageStrings[49378:4409233]   0x1c0800001950: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
2016-09-29 10:32:20.302036 GarbageStrings[49378:4409233] =>0x1c0800001960: fa fa fd fd fd fd[fd]fa fa fa 00 00 00 00 00 fa
2016-09-29 10:32:20.302666 GarbageStrings[49378:4409233]   0x1c0800001970: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
2016-09-29 10:32:20.302697 GarbageStrings[49378:4409233]   0x1c0800001980: fa fa fd fd fd fd fd fd fa fa 00 00 00 00 00 00
2016-09-29 10:32:20.302722 GarbageStrings[49378:4409233]   0x1c0800001990: fa fa 00 00 00 00 00 fa fa fa 00 00 00 00 00 00
2016-09-29 10:32:20.302738 GarbageStrings[49378:4409233]   0x1c08000019a0: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
2016-09-29 10:32:20.302752 GarbageStrings[49378:4409233]   0x1c08000019b0: fa fa fd fd fd fd fd fd fa fa fd fd fd fd fd fd
2016-09-29 10:32:20.302766 GarbageStrings[49378:4409233] Shadow byte legend (one shadow byte represents 8 application bytes):
2016-09-29 10:32:20.302779 GarbageStrings[49378:4409233]   Addressable:           00
2016-09-29 10:32:20.302793 GarbageStrings[49378:4409233]   Partially addressable: 01 02 03 04 05 06 07
2016-09-29 10:32:20.302806 GarbageStrings[49378:4409233]   Heap left redzone:       fa
2016-09-29 10:32:20.302862 GarbageStrings[49378:4409233]   Heap right redzone:      fb
2016-09-29 10:32:20.302880 GarbageStrings[49378:4409233]   Freed heap region:       fd
2016-09-29 10:32:20.302894 GarbageStrings[49378:4409233]   Stack left redzone:      f1
2016-09-29 10:32:20.302907 GarbageStrings[49378:4409233]   Stack mid redzone:       f2
2016-09-29 10:32:20.302920 GarbageStrings[49378:4409233]   Stack right redzone:     f3
2016-09-29 10:32:20.302933 GarbageStrings[49378:4409233]   Stack partial redzone:   f4
2016-09-29 10:32:20.302946 GarbageStrings[49378:4409233]   Stack after return:      f5
2016-09-29 10:32:20.302959 GarbageStrings[49378:4409233]   Stack use after scope:   f8
2016-09-29 10:32:20.302973 GarbageStrings[49378:4409233]   Global redzone:          f9
2016-09-29 10:32:20.302986 GarbageStrings[49378:4409233]   Global init order:       f6
2016-09-29 10:32:20.302999 GarbageStrings[49378:4409233]   Poisoned by user:        f7
2016-09-29 10:32:20.303136 GarbageStrings[49378:4409233]   Container overflow:      fc
2016-09-29 10:32:20.303154 GarbageStrings[49378:4409233]   Array cookie:            ac
2016-09-29 10:32:20.303168 GarbageStrings[49378:4409233]   Intra object redzone:    bb
2016-09-29 10:32:20.303181 GarbageStrings[49378:4409233]   ASan internal:           fe
2016-09-29 10:32:20.303194 GarbageStrings[49378:4409233]   Left alloca redzone:     ca
2016-09-29 10:32:20.303207 GarbageStrings[49378:4409233]   Right alloca redzone:    cb
2016-09-29 10:32:20.303228 GarbageStrings[49378:4409233] 
==49378==ABORTING
AddressSanitizer report breakpoint hit. Use 'thread info -s' to get extended information about the report.

@belkadan
Copy link

belkadan commented Sep 29, 2016

Ahh, the problem is that String.cString(using:) returns an array, not a pointer. That array can be implicitly converted to a pointer, but (a) the pointer you get from that operation is temporary, and not considered valid beyond the particular call it is used in, and (b) nothing is keeping the Array alive.

@phausler
Copy link
Member

phausler commented Sep 29, 2016

it is returning either an inner pointer or an inner pointer of an NSData that is created from the bytes.

So I think this is not directly associated with NSString and perhaps a code-gen problem. I was able to break on the correct free call and backtrace to a swift_unknownRelease where it is calling free inappropriately before usage.

main.asm

@phausler
Copy link
Member

phausler commented Sep 29, 2016

@belkadan correct! the API differential is a problem since Foundation vends as a UnsafePointer and swift re-interprets it as an array of CChars.

@belkadan
Copy link

belkadan commented Sep 29, 2016

Ah, Swift isn't reinterpreting it; it's correctly calling its _persistCString helper to copy the contents into the array. But it's certainly a pitfall for users.

@natecook1000
Copy link
Member

natecook1000 commented Oct 29, 2016

@belkadan I think you're right that the root problem is the implicit conversion—the call to UnsafePointer<Int8>.init(_:) with the array is an error that we don't catch. Given that that kind of implicit conversion will always be an error, along with passing a variable via inout, is there any way we can warn or error on this kind of implicit casting? The pointer initializers should allow implicit casting between pointer types, but not the broader implicit casting that's there to support C bridging. They're currently deceptively tempting for developers, especially because they can "work" in some contexts, despite being undefined behavior.

var number: Int8 = 0
var array: [Int8] = [1, 2, 3, 4, 5]
var ptr: UnsafeMutablePointer<Int8> = ...

// Can we block these first two?
let bad1 = UnsafeMutablePointer(&number)
let bad2 = UnsafePointer(array)
let okay = UnsafePointer(ptr)

// Tempting b/c it works:
bad1.pointee = 23
// number == 23

cc @atrick

@belkadan
Copy link

belkadan commented Oct 31, 2016

Yeah, we've definitely talked about doing this. It's hard to come up with general rules (other than "only allow these conversions when calling imported things", which I think is too restrictive), but specifically rejecting the pointer construction case is reasonable.

@hamishknight
Copy link

hamishknight commented Oct 26, 2018

I've been working on a patch to reject such unsound pointer conversions through the introduction of an underscored @_nonEphemeral attribute that marks parameters that cannot accept temporary pointer values created from inout-to-pointer, string-to-pointer and array-to-pointer conversions (I'll stage it in as a warning, having it become an error in a future version).

Does this sound like a good direction in which to solve this issue? I'm hoping to open a WIP PR later today to see what kind of source breakage we get when running it through the compat suite.

@hamishknight
Copy link

hamishknight commented Oct 26, 2018

Opened a PR: apple/swift#20070

@belkadan Could you please run the source compat suite on it?

@hamishknight
Copy link

hamishknight commented Nov 5, 2019

Resolved by apple/swift#27695 we will now emit a warning for this (which will hopefully become an error in a future release). sclukey (JIRA User) please verify using the next available master snapshot from https://swift.org/download/#snapshots.

@glessard
Copy link
Contributor

glessard commented Jun 8, 2021

The original bug as expressed here now results in a warning, but implicit conversions from array to pointer don't get the warning. I'm not sure I know why the following example should behave differently than the original (the following is equivalent to "bad2" from Nate's example above):

var strings: [UnsafePointer<CChar>] = []
strings.append("swift".cString(using: .ascii)!)
print("\(strings[0])")

Related: https://bugs.swift.org/browse/SR-11373

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 5, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants