Fix a bug in CFReadStreamGetBuffer for data streams

[baarde]

The dataGetBuffer function computes the number of bytes that have been read from the stream so far when it should compute the number of bytes remaining, with the following issues:

• A stream that hasn't been read yet reads 0 bytes and sets its status to AtEnd.
• A stream that is near its end reads more bytes than it should.

That bug has been affecting CoreFoundation and Foundation on OS X and iOS for a while.
That bug will eventually affect NSInputStream's getBuffer(:length:) when it's implemented.

Reference: http://openradar.me/5177598
Unit Test: baarde/swift-corelibs-foundation@81125ffc83051e2ebdca79cb504b51fd389dba5f

[salgernon]

Hi there, CFReadStreamGetBuffer hasn't worked properly forever (not quite relevant link: http://lists.apple.com/archives/macnetworkprog/2002/Jul/msg00046.html) and I fear that fixing it would encourage use of the CF and NS variants. "GetBuffer" is fundamentally incompatible with efforts to make CFStream (and NSStream) in any way thread safe. My preference would be to remove the API from the swift open source drop, and deprecate it from Foundation and CoreFoundation.

Since CFReadStreamGetBuffer is akin to CFStringGetCStringPtr in that it can (and will) fail if it cannot satisfy its O(1) performance constraint , anyone trying to use it should already have fallback code in place.

I was going to make comments on alternatives, but they should be pretty obvious. There's no public API I can think of that would consume an NS/CFInputStream and expect GetBuffer to work.

[parkera]

@salgernon it sounds like maybe we should still take this though, because reading more bytes than you should could be a pretty big problem. Even if this fix is just for the Darwin CF.

[salgernon]

@parkera, In the public API, the only way to implement an NSInputStream is by subclasses; in that case, a subclassser CAN provide an implementation of -getBuffer:length that works. However, since the stream consumer must have code to deal with the case where the get buffer fails, I would suggest that we ALWAYS make it fail while in the CFStream implementation, and deprecating the method / function.

I can't figure out when -getBuffer:length: is useful in any case where you didn't actually know that you had a memory stream in the first place, in which case, you may as well have been passed an [NSData subdataWithRange:]. (Oh, and while i"m on the subject, +[NSData dataWithBytesNoCopy:length:] should be deprecated in favor of a variant that takes an explicit destructor (or a block; there's a version of -initWithBytesNoCopy:length:deallocator:))

I guess my rant comes down to: if you've got a buffer of bytes, you don't need a stream unless you need to hand the stream to a subsystem. That subsystem cannot assume that any stream passed to it will be able to respond successfully to -getBuffer, so will always need to resort to -read:.

Maybe we should take this to the API group?

[baarde]

-getBuffer:length: prevents the need to copy the bytes when the subsystem that reads the stream can handle them on the fly. I don't know whether the performance gained by avoiding a call to memmove is worth the hassle to add an extra path to the code. The people who designed the API must have thought so but I don't think anybody actually uses that method (otherwise it may have been fixed).

But if somebody wants to modify the NSStream/CFStream API and deprecate methods, that's fine by me. I would even suggest to deprecate two more methods, namely scheduleInRunLoop:forMode: and removeFromRunLoop:forMode:, which could be replaced by a much nicer setDelegateQueue: (not only for the sake of simplicity but mainly because that would solve bridging issues that occur when one tries to subclass NSStream).

[parkera]

I'm going to take this one for the buffer fix, and we will discuss the deprecation in another thread.