Skip to content

Avoid IndexOutOfRangeException in ZipArchive - #109168

Merged
ViktorHofer merged 6 commits into
dotnet:mainfrom
peter15914:fix-ziparchive-outofrange
Jan 15, 2025
Merged

Avoid IndexOutOfRangeException in ZipArchive#109168
ViktorHofer merged 6 commits into
dotnet:mainfrom
peter15914:fix-ziparchive-outofrange

Conversation

@peter15914

Copy link
Copy Markdown
Contributor

Fixes #109167

This pull request fixes unexpected IndexOutOfRangeException in ZipArchive.
Additional check added that array index is not out of range.
It is done in a similar way to the code in HuffmanTree.c:230:

if (value > 0)
{
    // prevent an IndexOutOfRangeException from array[index]
    throw new InvalidDataException(SR.InvalidHuffmanData);
}

Found by Linux Verification Center (linuxtesting.org).

@peter15914

Copy link
Copy Markdown
Contributor Author

Hello, @ericstj , @dotnet/area-system-io-compression !
Can you review this PR?

peter15914 added a commit to peter15914/runtime-assets that referenced this pull request Nov 20, 2024
An IndexOutOfRangeException exception is thrown when this invalid zip file is opened with ZipArchive.
This file is generated while fuzzing ZipArchive with AFL++.

More details in issue and pull request:
dotnet/runtime#109167
dotnet/runtime#109168
akoeplinger pushed a commit to dotnet/runtime-assets that referenced this pull request Nov 21, 2024
An IndexOutOfRangeException exception is thrown when this invalid zip file is opened with ZipArchive.
This file is generated while fuzzing ZipArchive with AFL++.

More details in issue and pull request:
dotnet/runtime#109167
dotnet/runtime#109168
@akoeplinger

akoeplinger commented Nov 21, 2024

Copy link
Copy Markdown
Member

@peter15914 FYI you'll need to wait until #109477 is merged to be able to consume the runtime-assets changes. Once that PR is in you can merge main into your branch and you should be able to access the new file.

Add test to cover problem with invalid zip file.
Currently an IndexOutOfRangeException is thrown when invalid zip file is opened with ZipArchive.
@peter15914

Copy link
Copy Markdown
Contributor Author

@dotnet-policy-service agree

@peter15914

Copy link
Copy Markdown
Contributor Author

Hello @stephentoub !
I added unit test for this scenario.
Method ZipArchive_InvalidHuffmanData is made similar to existing functions in zip_InvalidParametersAndStrangeFiles.cs.

@stephentoub

Copy link
Copy Markdown
Member

Hello @stephentoub ! I added unit test for this scenario. Method ZipArchive_InvalidHuffmanData is made similar to existing functions in zip_InvalidParametersAndStrangeFiles.cs.

Thanks!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

@ericstj ericstj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left a small suggestion, but I'd like @carlossanlop to review too

Change "Theory" to "Fact"

Co-authored-by: Eric StJohn <ericstj@microsoft.com>
@ViktorHofer

Copy link
Copy Markdown
Member

@carlossanlop PTAL

@carlossanlop carlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. The condition makes sense and the test verifies the reported scenario.

@ViktorHofer

ViktorHofer commented Jan 15, 2025

Copy link
Copy Markdown
Member

/ba-g a lot of timeouts in different legs for which we already have dnceng issues

@ViktorHofer
ViktorHofer merged commit 1ce82e7 into dotnet:main Jan 15, 2025
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 15, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IO.Compression community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IndexOutOfRangeException in System.IO.Compression.ZipArchive

7 participants