Fix memory corruption with NSMutableData(length:)

[mbvreddy]

NSMutableData(length:) allocates a memory of 'length' bytes and initialises NSMutableData with the same malloced bytes as buffer. And, capacity is 16 (default). And, any further appends to NSMutableData actually happens outside malloced region corrupting the memory next to it.

Fix is to avoid reusing malloced region as buffer. Instead, create new buffer with capacity and copy contents of malloced region into the buffer and free malloced region after copy.

[parkera]

I think the problem goes deeper than this. NSMutableData should create a CFMutableData and defer its operations to that. The init method we're calling here should always copy the data into the new mutable buffer.

[mbvreddy]

@parkera NSMutableData.init() calls boils down to _CFDataInit() which creates CFMutableData already. This PR corrects init method, NSMutableData(length:) to copy the contents into the new CFMutableData buffer.

[parkera]

So we call malloc and get an uninitialized region, then copy it into another buffer, then free it?

[mbvreddy]

@parkera Yes. That is exactly what happens after this fix.

[parkera]

Why copy an uninitialized malloc buffer? It seems like an unnecessary performance cost.

[mbvreddy]

@parkera
We have only 2 option, either create buffer in Foundation and ask CoreFoundation to reuse the same buffer or let CoreFoundation create buffer for us and copy the initial content. Logic of initial buffer capacity and the amount it needs to grow further is defined in CoreFoundation.

If we have to avoid copy of malloc'ed data, we need to create buffer at Foundation layer and reuse the same. But, we may not know the capacity of the buffer and we just need to create a buffer of capacity 'length' which will be expanded on first append.

So, its a trade-off between copying malloc'ed content or allow it to grow at first append call.

[phausler]

I wonder if it might be better to just call the plain init method and set the length that way we are less wasteful?

[mbvreddy]

@phausler that sounds good and neat. init() would initialize buffer with default capacity (say 16) and length as 0.

CFDataSetLength() would check if capacity is < newlength. If yes, and buffer is growable, it expands it.

let me build it.

[phausler]

Fyi this is the abstract objc version:

- (id)initWithLength:(NSUInteger)length {
    self = [self initWithCapacity:length];
    [self setLength:length];
    return self;
}

[mbvreddy]

@phausler modified fix and contributed simple testcase as well.

[parkera]

Thanks!