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.

Sign up for GitHub

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

Jump to bottom

Throw exception on Store+Descriptor entries #517

Merged
merged 7 commits into from Feb 21, 2021
Merged

Throw exception on Store+Descriptor entries #517

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bf6b70e
Throw exception on Store+Descriptor entries
piksel Oct 3, 2020
937cd89
fix(zip): indicate that store/desc. entries cant be extracted
piksel Feb 13, 2021
6c19753
style(zip): simplify and fix spelling
piksel Feb 13, 2021
29d1817
fix(zip): check decompress support after specific exception handling
piksel Feb 13, 2021
dc43888
test: use random bad path instead of possible network drive
piksel Feb 13, 2021
bf5dfec
docs(zip): remove explicit mentions of methods in CompressionMethod
piksel Feb 13, 2021
45dbdd8
fix(zip): remove superfluous null check
piksel Feb 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 19 additions & 14 deletions src/ICSharpCode.SharpZipLib/Zip/ZipInputStream.cs
View file
Open in desktop
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it should be possible to get here in that case? (maybe auseful sanity check in case though)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
throw new ZipException("Library cannot extract this entry. Version required is (" + entry.Version + ")");
throw new ZipException("Current entry is null");
}

var usesDescriptor = (entry.Flags & (int)GeneralBitFlags.Descriptor) != 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can that use the HasFlag helper you added?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Handle encryption if required.
if (entry.IsCrypted)
{
Expand All @@ -543,21 +545,19 @@ private int InitialRead(byte[] destination, int offset, int count)
{
csize -= ZipConstants.CryptoHeaderSize;
}
else if ((entry.Flags & (int)GeneralBitFlags.Descriptor) == 0)
else if (!usesDescriptor)
{
throw new ZipException(string.Format("Entry compressed size {0} too small for encryption", csize));
throw new ZipException($"Entry compressed size {csize} too small for encryption");
}
}
else
{
inputBuffer.CryptoTransform = null;
}

var usesDescriptor = (flags & (int) GeneralBitFlags.Descriptor) != 0;

if ((csize > 0) || usesDescriptor)
if (csize > 0 || usesDescriptor)
{
if ((method == CompressionMethod.Deflated) && (inputBuffer.Available > 0))
if (method == CompressionMethod.Deflated && inputBuffer.Available > 0)
{
inputBuffer.SetInflaterInput(inf);
}
Expand All @@ -569,14 +569,19 @@ private int InitialRead(byte[] destination, int offset, int count)
return StoredDescriptorEntry(destination, offset, count);
}

internalReader = new ReadDataHandler(BodyRead);
if (!CanDecompressEntry)
{
internalReader = ReadingNotSupported;
return ReadingNotSupported(destination, offset, count);
}

internalReader = BodyRead;
return BodyRead(destination, offset, count);
}
else
{
internalReader = new ReadDataHandler(ReadingNotAvailable);
return 0;
}


internalReader = ReadingNotAvailable;
return 0;
}

/// <summary>
Expand Down