0 byte buffers writing 0 bytes fails CRC

[michaelaird]

Steps to reproduce

I'm not entirely sure what causes this. We're streaming encrypted blobs from Azure asynchronously into a ZipOutputStream. I think it depends on the file size. When the final block is being flushed, it tries to call write with a 0 length buffer, an offset of 0 and a count of 0.

Expected behavior

Stream should be zipped successfully.

Actual behavior

CRC check throws a "not a valid index into buffer" exception

I think either
-the CRC should allow for 0 length byte[], 0 offset, 0 count or,
-the . Write(byte[] buffer, int offset, int count) method should just return if the arguments are 0 length byte[], 0 offset, 0 count (nothing passed in, nothing to do)

Version of SharpZipLib

ICSharpCode.SharpZipLib-dogfood.1.0.296

Obtained from (place an x between the brackets for all that apply)

• Package installed using:
• NuGet

[christophwille]

Could you provide a small sample and/or comment on whether this happens with the old version from Nuget too? We did some history digging yesterday (on another issue), and kind of fear that a couple of things changed (unintentionally) over the years.

[michaelaird]

Hmmmm. I could probably pull together a sample that shows the issue but would need encryption keys, azure storage accounts, etc to be configured.

Not sure if it was an issue with the original nuget package. We moved to the "dogfood" branch to get async stream copying to work properly

[michaelaird]

This commit: 3615be2 by @McNeight

made some significant changes to the argument checking of CRC32.Update.

the previous check of

if (offset < 0 || offset + count > buffer.Length) {
   throw new ArgumentOutOfRangeException(nameof(offset));
}

would have allowed 0 length buffer, an offset of 0 and a count of 0

[michaelaird]

TBH, I don't know all the implications of going back to the previous check but I would gladly submit a PR for the change if that would be helpful.

[christophwille]

Me neither - don't know which implementation would be correct.

[michaelaird]

old

if (count < 0) {
  	throw new ArgumentOutOfRangeException(nameof(count), "Count cannot be less than zero");
}
  
if (offset < 0 || offset + count > buffer.Length) {
	throw new ArgumentOutOfRangeException(nameof(offset));
}

current

if (offset < 0) {
	throw new ArgumentOutOfRangeException(nameof(offset), "cannot be less than zero");
}
  
if (offset >= buffer.Length) {
	throw new ArgumentOutOfRangeException(nameof(offset), "not a valid index into buffer");
}

if (count < 0) {
	throw new ArgumentOutOfRangeException(nameof(count), "cannot be less than zero");
}

proposed

if (offset < 0) {
	throw new ArgumentOutOfRangeException(nameof(offset), "cannot be less than zero");
}

if (count < 0) {
	throw new ArgumentOutOfRangeException(nameof(count), "cannot be less than zero");
}
  
if (offset + count > buffer.Length) {
	throw new ArgumentOutOfRangeException(nameof(offset), "not a valid index into buffer");
}

I think this is still safe but allows my specific 0/0/0 case. If you're ok with it, I will submit a PR.

[Stevie-O]

@michaelaird: Your version is vulnerable to integer overflow, if (offset + count) > int.MaxValue.
Consider this case:

int offset, count;
offset = count = int.MaxValue;

byte[] buffer = new byte[10];

In this case, (offset + count) is -2, which is less than buffer.Length, incorrectly passing the third check.

This should instead be written as:

if (offset < 0) {
	throw new ArgumentOutOfRangeException(nameof(offset), "cannot be less than zero");
}

if (count < 0) {
	throw new ArgumentOutOfRangeException(nameof(count), "cannot be less than zero");
}
  
if (offset > buffer.Length - count) {
	throw new ArgumentOutOfRangeException(nameof(offset), "not a valid index into buffer");
}

(Additionally, the .NET built-ins that do this sort of check use an ArgumentException for the third check, because there's no sure way to know whether offset or count is at fault.)

Even better, you could just construct a dummy ArraySegment instance:

var dummy = new ArraySegment<byte>(buffer, offset, count);

This will automatically perform a null-check on buffer and a bounds-check on offset and count, as documented here: https://msdn.microsoft.com/en-us/library/9cc4bx8k(v=vs.110).aspx

Proof:

The expression 'buffer.Length - count' can only be problematic if the true value is greater than int.MaxValue or less than int.MinValue. All other values will be represented without loss.

• The largest possible value for buffer.Length is int.MaxValue, and the smallest possible value for count is 0 (because the second check tests for count less than zero). Therefore, the largest possible true value of the expression buffer.Length - count is int.MaxValue.
• The smallest possible value for buffer.Length is 0, and the largest possible value for count is int.MaxValue, so the smallest possible true value of buffer.Length - count is -int.MaxValue, which is int.MinValue + 1.

[michaelaird]

@Stevie-O I like the ArraySegment solution. I think that actually covers all the cases correctly using a "built-in" mechanism.

I will submit a PR for review shortly.