Bug 15671 - System.Net.WebConnectionStream.Length returns -1 when it can't parse the Content-Length
Summary: System.Net.WebConnectionStream.Length returns -1 when it can't parse the Cont...
Status: RESOLVED NOT_ON_ROADMAP
Alias: None
Product: Class Libraries
Classification: Mono
Component: System ()
Version: unspecified
Hardware: All All
: --- normal
Target Milestone: Untriaged
Assignee: Bugzilla
URL:
Depends on:
Blocks:
 
Reported: 2013-10-24 16:37 UTC by Melvyn
Modified: 2013-10-24 21:37 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 GitHub or Developer Community 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 NOT_ON_ROADMAP

Description Melvyn 2013-10-24 16:37:04 UTC
The internal class WebConnectionStream that is used when reading data from an HttpWebResponse returns a Length of -1 when the Content-Length HTTP header cannot be parsed.

This leads to various bugs for other internal components assuming the Stream.Length property will be positive if it does not throw an exception.


For example, I'm currently using Xamarin.iOS with MonoGame and loading textures from web images.
Sometimes the Content-Length header is not present/invalid on the images I load from the network (I didn't check how the headers are parsed, this is not the point) and this ends up crashing my application with a segfault...

This is because I'm using Texture2D.FromStream(*) with the instance of WebConnectionStream, and this method in turn is calling NSData.FromStream(*) with the stream, which is finally trying to create a NSMutableData using NSMutableData.FromCapacity(*) with the length of the stream, which is negative!

The native method then crashes the entire program. This would not have happened if the Length property was throwing an exception instead of returning a negative value.

I'm filing this bug for WebConnectionStream instead of NSData because I think this is not the behaviour a .Net programmer would be expecting from this property, but NSData would probably need more checks too...
Comment 1 Melvyn 2013-10-24 16:54:32 UTC
I filed a bug for NSData too here: https://bugzilla.xamarin.com/show_bug.cgi?id=15672 (can't we edit the description of a bug?)
Comment 2 Jonathan Pryor 2013-10-24 17:37:20 UTC
I suspect the _real_ issue is that WebConnectionStream.Length doesn't just always throw NotSupportedException in the first place. This appears to be what .NET does, at least if I read between the lines:

http://stackoverflow.com/questions/4242058/how-to-know-the-length-of-response-httpwebrequest-getresponse-getresponsestre

(see image, and look at the value for the Length property.)

However, as per that answer you "should" use WebRequest.ContentLength, which likewise will return -1 if the Content-Length header isn't set.

So I really only see three solutions here:

1. Do nothing/keep current behavior. It's apparently a Mono extension, but allows you to (possibly) use Stream.Length if it's set in the header.

This is technically valid, as the Stream.Length docs don't actually constrain the values returned (at least not to my reading).

http://msdn.microsoft.com/en-us/library/system.io.stream.length.aspx

2. Adopt .NET behavior, which will result in WebConnectionStream.Length _always_ throwing NotSupportedException. This will likewise break your code, just in a different way.

3. If the Content-Length header is NOT specified, WebConnectionStream.Length throws NotSupportedException. Otherwise, returns the length.

This is at least tenable, but nothing screams to me that this is superior to (1) or (2), and this would _still_ break your code with a NotSupportedException.

if anything, I'd be inclined to do (2), except that's what we previously had before the current behavior. (I have no idea why we introduced the current behavior.)

https://github.com/mono/mono/commit/4b72b767

(Marking WONTFIX until a better solution becomes apparent.)
Comment 3 Melvyn 2013-10-24 17:50:45 UTC
The third solution makes more sense to me.

It is desirable to have the length when possible, but an exception should be thrown when it's not, instead of returning an arbitrary value no one is expecting.

In my example, the crash would not have happened if an exception had been thrown instead of -1, because the exception is an expected case and is properly handled in the method causing the crash...

In fact, I think the method in my example should check for CanSeek before doing anything with the length. But I think very few real life code do this, because CanSeek does not sound like it's related to the Length IMO. But that's another problem.


Besides, I just checked the implementation of WebConnectionStream.CanSeek, and it is always returning false. This is not the way it should work in this case: http://msdn.microsoft.com/en-us/library/system.io.stream.canseek.aspx


There are at least one bug here, and I don't see how marking it WONTFIX could be a good idea...
Comment 4 Jonathan Pryor 2013-10-24 18:03:13 UTC
CanSeek==false just further emphasizes that Length should throw NotSupportedException, which would rule out (1) and (3).

I'd still like to know why the change was done in the first place before removing it, as I have no idea what code it'll break if we change it. (Insert complaints about commit messages here.)

TL;DR: if code followed the Stream semantics properly in the first place, you'd never hit your bug, as CanSeek would be false, and thus you'd never access Length (requiring that you read into an intermediate buffer and do processing off that).

And the only "right" change is (2), which isn't going to help you at all.

I would suggest writing your code to follow the Stream semantics, in which case it won't hit this codepath, and you'll be forced to reading into an intermediate buffer, which will allow NSData/etc. to work properly.
Comment 5 Melvyn 2013-10-24 18:19:27 UTC
Just to clarify, the bug in my example is not in my code, but Xamarin's code!

I'm only passing a Stream I get from an http response (I'm not supposed to know the implementation) to a method...

And I still think 3 would be the best compromise. But it would indeed require a proper implementation of the CanSeek property...
Comment 6 Melvyn 2013-10-24 18:25:23 UTC
But if you are not willing to implement this (I understand that could be a lot of code), at least make the Length and CanSeek properties work consistently with each other, and go for number 2.

This would actually help, even if Length always throws an exception, because as I said, this case is handled properly in the method causing the crash I had.

Whatever you do, -1 really is not a good return value for Length...
Comment 7 Melvyn 2013-10-24 21:37:55 UTC
Ok, I thought about all of this a bit more, and I think I agree with you:

The best thing to do is probably to always throw an exception. This way, the implementation is coherent with CanSeek, and with .NET.

There is no magic -1 being returned, so should the user code fail, it will fail faster. And this kind of solves the bug I showed in my example.

The Content-Length header is not very reliable anyway, so it's probably a good idea not to trust it.

I made a pull request on github https://github.com/mono/mono/pull/789