Monday, 27 November 2017

A bug hidden for 4 years !

I have to admit I've abandoned my file uploader for some time, and I've been revisiting the code recently as part of some experiments with .NET Core 2.

After some work done to make it work with the new framework, to my surprise, it just looked to work perfectly until I tried to validate one of the basic implicit requirements: the uploaded copy must be exactly the same as the original file. It turned out that even if the files had the same exact size, to the byte, they weren't identical, how was that possible ?

Symptom:

After uploading a file larger than the packet size (default to 4 MB), the resulting copy was not identical to the original.

Simple test:

Upload a zip file larger than 4 MB, try to open it. It was corrupt.

The most uprising fact was that if I ran the .NET 4 version it still worked as expected ! Next, I migrated the solution to .NET 4.7 and still worked as expected, apparently it was only affecting to .NET Core version. So, under the circumstance, I tried to compare request / response side by side working version vs bugged version.

A few things looked a bit suspicious:

  • some extra headers sent on .NET working version
  • Transfer-Encoding: chunked from .NET Core bugged version

None of those were related. Back to square one.

I tried reducing the packet size to 1024 bytes and upload a text file so I could see how the content differed but no luck, it was indeed the same (verified comparing documents with Diff tool) Well now deep debugging showed that the buffer wasn't being written completely to output file, that would explain the corrupted resulting file but not the size difference. Let's see the code.

Original version

var buffer = new byte[contentLength];
var bytesread = inputStream.Read(buffer, 0, buffer.Length);
if (bytesread > 0)
{
    using (var fs = File.Open(actualfile, FileMode.Append, FileAccess.Write))
    {
        fs.Write(buffer, 0, buffer.Length);
    }
    //json.Data = new {guid, packet};
    return new UploadPacketResult(guid, packet);
}

Spot something weird ?

It turns out that Stream.Read can perform a blocking operation that reads up to the length specified but not necessarily the whole buffer, now it's obvious! but how it didn't fail before !! I don't have an answer for that :)

Well, here is the fixed version

var buffer = new byte[contentLength];
using (var fs = File.Open(actualfile, FileMode.Append, FileAccess.Write))
{
    int read = 0;
    while ((read = inputStream.Read(buffer, 0, buffer.Length)) > 0)
    {
        fs.Write(buffer, 0, read);
    }
}
//json.Data = new {guid, packet};
return new UploadPacketResult(guid, packet);

And that's the way it was supposed to be done in the first place, but there was no test case that proved otherwise (until now, 4 years later)