Throw exception on Store+Descriptor entries #517
Conversation
|
Should |
Good point! It should. That is the most elegant way to handle it for the consumer. |
| @@ -861,42 +722,29 @@ public long Crc | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Gets/Sets the compression method. Only Deflated and Stored are supported. | |||
| /// Gets/Sets the compression method. Only <see cref="CompressionMethod.Deflated">Deflated</see> | |||
There was a problem hiding this comment.
Not related to this actual issue, but that's out of date - BZip2 is supported in the ZipFile case now
There was a problem hiding this comment.
Yeah, that's the problem with having this logic in ZipEntry. I wonder if we even should limit this here. It should be up to ZipFile and ZipOutputStream to throw on adding such an entry.
| /// <summary> | ||
| /// General ZipEntry helper extensions | ||
| /// </summary> | ||
| public static class ZipEntryExtensions |
There was a problem hiding this comment.
Does it need to be public? (not sure if it's useful for other users or not?)
There was a problem hiding this comment.
Maybe not, but I know that I would appreciate having it when consuming the library (since the flag is exposed in a public property as an int).
| @@ -512,11 +512,13 @@ private int ReadingNotSupported(byte[] destination, int offset, int count) | |||
| /// <returns>The actual number of bytes read.</returns> | |||
| private int InitialRead(byte[] destination, int offset, int count) | |||
| { | |||
| if (!CanDecompressEntry) | |||
| if (entry == null) | |||
There was a problem hiding this comment.
I don't think it should be possible to get here in that case? (maybe auseful sanity check in case though)
There was a problem hiding this comment.
No, I don't think so either, but since the CanDecompressEntry used to check it, I didn't want to remove it since the rest of the body dereferences entity a lot. It shouldn't be too hard to verify though.
There was a problem hiding this comment.
I was going to query if changing this effects the handling of AES entries (where the 'entry.IsCrypted' path only handles ZipCrypto entries), but I don't think it can get this far for those because of the earlier checks in GetNextEntry.
There was a problem hiding this comment.
Yeah, exactly. There is only one reference to the method, and that method sets and dereferences entity just before invoking this one. Hence I removed the check.
|
On another trivial not-directly-related-to-this-issue thought: looking at the code just made me wonder if there enough instances of use purely as a 'should the entry be AES encrypted' check to make it worth adding something like a property to ZipEntry, even if only as an internal helper? |
|
Changes seem ok otherwise though |
|
Note that the reason for it immediately throwing in |
| { | ||
| throw new ZipException("Library cannot extract this entry. Version required is (" + entry.Version + ")"); | ||
| } | ||
| var usesDescriptor = (entry.Flags & (int)GeneralBitFlags.Descriptor) != 0; |
There was a problem hiding this comment.
Can that use the HasFlag helper you added?
There was a problem hiding this comment.
There's a lot of places where it can be used, but this is essential the same as using that method. It might be a bit easier to read, but the main issue I had were the places where the flags where compared to a magic number instead. Really opaque to the reader.
Anyhow, I won't add any more changes of that type to this PR, but I'll instead merge it so that it can be used in subsequent PRs.
Due to the way that ZipInputStream is designed, reading entries that are using
Storecompression method andDescriptorflag is not possible. This change will throw aStreamUnsupportedExceptioninstead of failing on incorrect CRC.Fixes #15 (as much as a fix as is possible for this).
I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.