Bug 17655 - BinaryFormatter does not read from Streams correctly
Summary: BinaryFormatter does not read from Streams correctly
Status: RESOLVED DUPLICATE of bug 15143
Alias: None
Product: Class Libraries
Classification: Mono
Component: mscorlib ()
Version: unspecified
Hardware: PC Mac OS
: --- normal
Target Milestone: Untriaged
Assignee: Bugzilla
URL:
Depends on:
Blocks:
 
Reported: 2014-02-07 10:02 UTC by Alan McGovern
Modified: 2014-02-12 18:36 UTC (History)
4 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 DUPLICATE of bug 15143

Description Alan McGovern 2014-02-07 10:02:37 UTC
BinaryFormatter has a bunch of places where it assumes that Stream.Read (x) always returns x bytes. This is not true as (for example) network streams commonly read less bytes than are requested as the underlying socket may not have all the bytes.

As a result, using a BinaryFormatter directly on a NetworkStream/Socket will frequently throw exceptions. Wrapping the NetworkStream in another stream which always returns the amount of bytes requested (unless an error occurs) works around the issue.

An example of our workaround is: https://gist.github.com/alanmcgovern/b7b698a65538db83d810
Comment 1 Miguel de Icaza [MSFT] 2014-02-11 12:41:49 UTC
Confirmed: .NET version of the BinaryFormatter does not assume Read will read all the requested data.
Comment 2 Miguel de Icaza [MSFT] 2014-02-11 16:17:32 UTC
So BinaryFormatter does not directly deal with a Stream, but it uses a BinaryReader.

And reviewing BinaryReader, I can not find a single place that makes the assumption that Read returns the number of requested bytes.

Alan, do you have a specific test that shows the problem?   Perhaps this is something else.
Comment 3 Alan McGovern 2014-02-12 12:16:09 UTC
Repro:

https://gist.github.com/alanmcgovern/185cfb502b48ef328696
Comment 4 Alan McGovern 2014-02-12 12:27:48 UTC
BinaryReader.ReadString is broken - it does not read 'count' bytes from the stream. the implementation is also broken as it should read the entire range of bytes into memory before converting it to a string to properly account for multibyte chars.
Comment 5 Alan McGovern 2014-02-12 12:38:22 UTC
Patch to fix the Read issue, but i don't address the string decoder issue.

diff --git a/mcs/class/corlib/System.IO/BinaryReader.cs b/mcs/class/corlib/System.IO/BinaryReader.cs
index d48bb26..a5bccee 100644
--- a/mcs/class/corlib/System.IO/BinaryReader.cs
+++ b/mcs/class/corlib/System.IO/BinaryReader.cs
@@ -532,7 +532,7 @@ namespace System.IO {
                                        sb = new StringBuilder (len);
                                
                                sb.Append (charBuffer, 0, cch);
-                               len -= readLen;
+                               len -= n;
                        } while (len > 0);
 
                        return sb.ToString();
Comment 6 Alan McGovern 2014-02-12 12:39:14 UTC
There's a testcase and patch to fix 1/2 of the issue if one of the mono guys can take it and run with it. cc'ing them in
Comment 7 Miguel de Icaza [MSFT] 2014-02-12 18:00:32 UTC
Alan,

There is no "n" defined in that function, and reviewing the code, it looks like it is fine.

Perhaps you meant to patch something else?
Comment 8 Alan McGovern 2014-02-12 18:27:55 UTC
Yeah, the patch is completely wrong. I'm 99% certain that this function, or the code that executes immediately after it, is incorrect because if you use SDB to put a breakpoint on SlowStream.Read you'll find that this is the last framework code to read from the stream before it errors out. You can 'fix' the bug by changing SlowStream.Read to actually read the requested bytes instead of returning the hardcoded 1 byte.
Comment 9 Alan McGovern 2014-02-12 18:29:46 UTC
Also since the string is 4 bytes long and we return 1 byte from every read call, we should see 4 invocations to 'SlowStream.Read' with 'ReadString' in the stack. However we only get one invocation before it errors out.
Comment 10 Alan McGovern 2014-02-12 18:35:54 UTC
So this is why my patch was wrong :p https://github.com/mono/mono/commit/4de39e0eac282e4efd23305e5e6c170d5ccfbb1c
Comment 11 Alan McGovern 2014-02-12 18:36:05 UTC

*** This bug has been marked as a duplicate of bug 15143 ***