0 byte buffers writing 0 bytes fails CRC #183

Activity

1. christophwille commented on Aug 17, 2017

   

   christophwille

   on Aug 17, 2017

   Member

   More actions

   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.
2. michaelaird commented on Aug 29, 2017

   

   michaelaird

   on Aug 29, 2017

   ContributorAuthor

   More actions

   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
3. michaelaird commented on Aug 29, 2017

   

   michaelaird

   on Aug 29, 2017

   Last edited by michaelaird

   ContributorAuthor

   More actions

   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
4. michaelaird commented on Sep 7, 2017

   

   michaelaird

   on Sep 7, 2017

   ContributorAuthor

   More actions

   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.
5. christophwille commented on Sep 8, 2017

   

   christophwille

   on Sep 8, 2017

   Member

   More actions

   Me neither - don't know which implementation would be correct.
6. michaelaird commented on Sep 8, 2017

   

   michaelaird

   on Sep 8, 2017

   ContributorAuthor

   More actions

   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.
7. Stevie-O commented on Nov 27, 2017

   

   Stevie-O

   on Nov 27, 2017

   Last edited by Stevie-O

   Contributor

   More actions

   @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.
8. michaelaird commented on Nov 27, 2017

   

   michaelaird

   on Nov 27, 2017

   ContributorAuthor

   More actions

   @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.
9. piksel

   closed this as completedin 6327312on Jul 12, 2018
10. blueszs

    added a commit that references this issue on Jan 13, 2025

    Merge PR icsharpcode#202, Convert CRC checking to ArraySegment

    ...

    This issue will close once commit 6327312 is merged into the 'master' branch.

    6327312