Bug 15672 - NSData.FromStream(stream) may result in a segfault
Summary: NSData.FromStream(stream) may result in a segfault
Status: RESOLVED FIXED
Alias: None
Product: iOS
Classification: Xamarin
Component: Xamarin.iOS.dll ()
Version: 7.0.4.x
Hardware: Other Other
: --- normal
Target Milestone: Untriaged
Assignee: Sebastien Pouliot
URL:
Depends on:
Blocks:
 
Reported: 2013-10-24 16:51 UTC by Melvyn
Modified: 2013-10-25 14:58 UTC (History)
3 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Melvyn 2013-10-24 16:51:48 UTC
If the stream passed to the NSData.FromStream(stream) method have a Length property returning a negative value (e.g. -1) a call to this method ends up crashing the whole application with a segfault.

More precisely, the crash occurs on the line calling NSMutableData.FromCapacity(int) with the Length of the stream.

I don't think a stream is ever supposed to return -1 for its Length, but WebConnectionStream (mono internal class) is doing it, and is triggering the crash when reading a stream from the web... See https://bugzilla.xamarin.com/show_bug.cgi?id=15671
Comment 1 Sebastien Pouliot 2013-10-24 19:59:29 UTC
Right, NSData definitively should not crash.
Comment 2 Sebastien Pouliot 2013-10-24 20:11:49 UTC
There's actually an (uncatched) ObjC exception being thrown if calling `NSMutableData.FromCapacity(-1)`

Objective-C exception thrown.  Name: NSInvalidArgumentException Reason: *** -[NSConcreteMutableData initWithCapacity:]: absurd capacity: 4294967295, maximum size: 2147483648 bytes
Comment 3 Sebastien Pouliot 2013-10-24 22:33:25 UTC
Fixed in master / b58010aab6b0a81a5def898219784b96e615996c

QA: unit tests added in the same revision

The workaround (until a fix is released) is to check for the stream's Length before calling `NSData.FromStream`
Comment 4 Melvyn 2013-10-25 13:17:17 UTC
Or to check for CanSeek. If the implementation of the stream follows the documentation, when the Length is invalid, CanSeek should return false.

http://msdn.microsoft.com/en-us/library/system.io.stream.canseek.aspx
Comment 5 Sebastien Pouliot 2013-10-25 13:24:17 UTC
Yes, it could have been a fix but:

* a Stream implementation can be buggy (e.g. I added a buggy one in our test suite to validate the fix);

* NSMutableData.FromCapacity (and other methods/ctor) would still crash if misused;

so I added validations in NSMutableData so both issues are fixed.
Comment 6 Melvyn 2013-10-25 13:29:56 UTC
So you didn't actually change NSData.FromStream()?

If so, This method will still crash when called with a WebConnectionStream. With another error, but still.

This method should check for CanSeek before using Length, or directly checking if Length is valid. (not just checking for it throwing an exception)

Excuse me if I misunderstood you.
Comment 7 Sebastien Pouliot 2013-10-25 14:08:55 UTC
> If the implementation of the stream follows the
> documentation, when the Length is invalid, CanSeek should return false.

This is not what the CanSeek documentation says. A correct Stream.Length implementation should throw a NotSupportedException when it return false for CanSeek.

And I can confirm that `NSData.FromStream` works correctly if `Length` throws a `NotSupportedException`.

> If so, This method will still crash

No. Throwing an exception and crashing are two different things. Applications can recover (catch) from the former.

> This method should check for CanSeek before using Length,

The NSData.FromStream code cannot cope with all possible cases of a mis-implemented Stream. 

However I see your point and it's not clear we can easily fix WebConnectionStream (at least not without quite a bit of testing). Furthermore checking `CanSeek` first is better than catching an exception. I'll add this check into FromStream (and add/adapt the tests).
Comment 8 Melvyn 2013-10-25 14:54:08 UTC
> when the Length is invalid, CanSeek should return false.

Yeah, it's the opposite, sorry. What I was implying is that if CanSeek is false, the user should not even try to get the Length.

Regarding the crash vs exception thing, if the exception is thrown in an internal method because I passed it an internal implementation of stream, from my (user) perspective, it's still buggy.
And in this case, the loading of the stream would still fail, while it would have perfectly worked if the FromStream method was using the default value it uses when Length throws an exception.

>The NSData.FromStream code cannot cope with all possible cases of a mis-implemented Stream. 

Yep, I understand that, but as you said, it's better to check CanSeek than to catch an exception.

Thank you :)
Comment 9 Sebastien Pouliot 2013-10-25 14:58:33 UTC
CanSeek check added in master / e7a5a1911b465a3eca799895a19fee15218508b8

QA: additional unit tests added in same revision