Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for System.Buffers.ArrayPool #52

Merged
merged 1 commit into from
Feb 27, 2020

Conversation

jonathanpeppers
Copy link
Member

Context: dotnet/android#4320

After using libZipSharp 1.0.9 in xamarin-android, one of our tests
surfaced a bug:

Xamarin.Tools.Zip.ZipIOException : Stream is not a ZIP archive

The test created a small text file inside a zip using a MemoryStream.

Something like:

using (var zip = ZipArchive.Create (zipStream)) {
    zip.AddEntry ("foo", "bar", encoding);
}

In 750c780, there was one mistake:

case SourceCommand.Write:
    buffer = ArrayPool<byte>.Shared.Rent (length);
    try {
        Marshal.Copy (data, buffer, 0, length);
        stream.Write (buffer, 0, length);
        return buffer.Length; // Whoops!
    } finally {
        ArrayPool<byte>.Shared.Return (buffer);
    }

In the test case above, the size of the buffer will be very small: the
equivalent of Encoding.UTF8.GetBytes("bar"). If ArrayPool returns
a larger buffer than we asked for, the code is actually wrong.

We need to return length instead of buffer.Length. I would suspect
that almost everything worked with this bug in place, except for this
case. xaprepare was able to do its work: downloading and unzipping
the Android SDK/NDK.

I was able to add a unit test showing this problem.

Context: dotnet/android#4320

After using libZipSharp 1.0.9 in xamarin-android, one of our tests
surfaced a bug:

    Xamarin.Tools.Zip.ZipIOException : Stream is not a ZIP archive

The test created a small text file inside a zip using a `MemoryStream`.

Something like:

    using (var zip = ZipArchive.Create (zipStream)) {
        zip.AddEntry ("foo", "bar", encoding);
    }

In 750c780, there was one mistake:

    case SourceCommand.Write:
        buffer = ArrayPool<byte>.Shared.Rent (length);
        try {
            Marshal.Copy (data, buffer, 0, length);
            stream.Write (buffer, 0, length);
            return buffer.Length; // Whoops!
        } finally {
            ArrayPool<byte>.Shared.Return (buffer);
        }

In the test case above, the size of the buffer will be very small: the
equivalent of `Encoding.UTF8.GetBytes("bar")`. If `ArrayPool` returns
a *larger* buffer than we asked for, the code is actually wrong.

We need to return `length` instead of `buffer.Length`. I would suspect
that almost everything worked with this bug in place, except for this
case. `xaprepare` was able to do its work: downloading and unzipping
the Android SDK/NDK.

I was able to add a unit test showing this problem.
@grendello
Copy link
Contributor

@jonathanpeppers xaprepare doesn't use LibZipSharp for its unzipping work, it uses 7zip from a nuget.

@dellis1972
Copy link
Contributor

This fix looks good. We should be returning the actual length of the expected data not the length of the buffer (which might be Huge).

@dellis1972 dellis1972 merged commit 05c43d0 into dotnet:master Feb 27, 2020
@jonathanpeppers jonathanpeppers deleted the arraypool-fix branch February 27, 2020 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants