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-6756] Another Data slice memory issue! #3747

Closed
CharlesJS opened this issue Jan 14, 2018 · 8 comments
Closed

[SR-6756] Another Data slice memory issue! #3747

CharlesJS opened this issue Jan 14, 2018 · 8 comments
Assignees

Comments

@CharlesJS
Copy link

CharlesJS commented Jan 14, 2018

Previous ID SR-6756
Radar rdar://problem/36917973
Original Reporter @CharlesJS
Type Bug
Status Resolved
Resolution Duplicate

Attachment: Download

Environment

Apple Swift version 4.0.3 (swiftlang-900.0.74.1 clang-900.0.39.2)

Apple Swift version 4.1-dev (LLVM bdcc78a6ed, Clang 27a368e018, Swift 2e48f09e2e)

Apple Swift version 4.1 (swiftlang-902.0.34 clang-902.0.30)

Additional Detail from JIRA
Votes 0
Component/s Foundation
Labels Bug
Assignee @phausler
Priority Medium

md5: e506d8e4c13358b9fdc11c9aa8387219

duplicates:

  • SR-6515 Subdata of Data returned from URLSession is equal, but has different hash value

Issue Description:

PROLOGUE.

PICARD: Mr. Data. What is the current status of your positronic network?

DATA: Captain, I am a dispatch_data_t from Objective-C, which was toll-free bridged to an NSData object, and then bridged to Swift as a Data struct. Being a dispatch_data_t, I am composed of several non-contiguous blocks of data.

PICARD: Interesting. Can you enumerate the contents of your memory banks?

DATA: Certainly. "Felis." "Catus." "Is." "Your." "Taxonomic." "Nomenclature."

PICARD: Can you give your contents all at once?

DATA: "Felis catus is your taxonomic nomencl–" <crackle> Intriguing. That was a most unusual sensation.

PICARD: Mr. Data. Can you show us a slice of your contents?

DATA: I can attempt it, Captain. One moment while I—MASAKA IS WAKING. I AM IHAT.


Hey all, I've found another bug involving Data slices, probably the most involved one yet. As you can see from the (hopefully 😉) humorous prologue above, corruption occurs if you have a non-contiguous dispatch_data_t object in Objective-C, cast it to an NSData, return that to Swift as a Data struct, and then try to manipulate that Data as a collection, slice it, use withUnsafeBytes on it, or use anything to access it other than enumerateBytes.

I've attached a sample project, but here's how to do it at home:

DataMaker.m:

#import "DataMaker.h"
#include <dispatch/dispatch.h>

@implementation DataMaker

+ (NSData *)makeMeAData {
    dispatch_block_t dtor = DISPATCH_DATA_DESTRUCTOR_DEFAULT;
    
    dispatch_data_t data1 = dispatch_data_create("feliscatus", 10, nil, dtor);
    dispatch_data_t data2 = dispatch_data_create("isyour", 6, nil, dtor);
    dispatch_data_t data3 = dispatch_data_create("taxonomic", 9, nil, dtor);
    dispatch_data_t data4 = dispatch_data_create("nomenclature", 12, nil, dtor);
    dispatch_data_t data5 = dispatch_data_create_concat(dispatch_data_create_concat(
        dispatch_data_create_concat(data1, data2), data3), data4);
    
    return (NSData *)data5;
}

@end

main.swift:

import Foundation

let data = DataMaker.makeMeAData()!

data.enumerateBytes { ptr, idx, _ in
    print(String(decoding: ptr, as: UTF8.self))
}

print(String(decoding: data, as: UTF8.self))

let slice = data[(data.startIndex + 5)..<(data.endIndex - 5)]

print(String(decoding: slice, as: UTF8.self))

slice.withUnsafeBytes { (src: UnsafePointer<UInt8>) in
    let dst = UnsafeMutablePointer<UInt8>.allocate(capacity: slice.count)
    _ = memcpy(dst, src, slice.count)
    print(NSData(bytes: dst, length: slice.count))
    dst.deallocate(capacity: slice.count)
}

The output is as follows:

The printout of the slices provided by enumerateBytes looks as expected:

feliscatus
isyour
taxonomic
nomenclature

The version that we tried to convert to a String almost looks right, but is glitched near the end:

feliscatusisyourtaxonomicnomenclnomen

And once we slice it, things get really wild:

felis�����isyourtaxonomicno

<66656c69 73aaaaaa aaaa6973 796f7572 7461786f 6e6f6d69 636e6f

As you can see, not only does the slice not begin at "catus" as we would expect, but if Malloc Scribble is turned on, the part containing "catus" is replaced by AA bytes, as if the runtime is screaming. AAAAAAAAAA!!!

I hope you've found this report entertaining and/or helpful.

@belkadan
Copy link

belkadan commented Jan 16, 2018

I think we fixed this one in Swift 4.1, but I haven't checked yet. @phausler?

@CharlesJS
Copy link
Author

CharlesJS commented Jan 16, 2018

Just tried it with the Swift 4.1 Xcode toolchain snapshot from yesterday (link). Still seems to be happening over here.

@CharlesJS
Copy link
Author

CharlesJS commented Jan 26, 2018

Still occurs with the Swift 4.1 distribution included with the new Xcode 9.3 beta (9Q98q).

@belkadan
Copy link

belkadan commented Jan 26, 2018

@swift-ci create

@phausler
Copy link
Member

phausler commented Jan 26, 2018

This never got merged into 4.1; it was already fixed on top of tree.

apple/swift#13280 never completed even though the other PRs succeeded.

@CharlesJS
Copy link
Author

CharlesJS commented Jan 26, 2018

Disagree on the risk assessment:

"Low risk since this is not a common case of bridging from dispatch_data_t to NSData to Data and then slicing it."

This is a more common case than you would expect, because URLSession passes Data objects which have been bridged from dispatch_data_t to NSData and then to Data. So any slicing of data from the network is prone to corruption...

@phausler
Copy link
Member

phausler commented Jan 26, 2018

That is the only API that vends it from the SDK; the risk has to do with the review process not the risk of not taking it. Anyhow it has been approved already this just was missed to do CI shenanigans and branching strategies.
The fix is already in master - sadly it didnt make it to Xcode because of the failed CI.

@CharlesJS
Copy link
Author

CharlesJS commented Jan 27, 2018

Thanks.

@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

3 participants