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.

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

Rewrite gzip component to support trailing bytes without external tool and compress with PIGZ. Addresses #476 #485

Merged
merged 20 commits into from
Aug 15, 2024

Conversation

alchzh
Copy link
Contributor

@alchzh alchzh commented Jul 17, 2024

Rewrite gzip component to support trailing bytes without external tool.

Link to Related Issue(s)

#476
python/cpython#68489

Please describe the changes in your request.

  • Replaces the use of the gzip library with calls to the underlying zlib library directly, in a way that doesn't fail with trailing garbage at the end of files.
  • Substantially rewrites of gzip test cases to use a common template
    • adds a test for multi member gzip files
    • the trailing bytes test actually tests the unpack

ofrak_core/test_ofrak/components/test_gzip_component.py Outdated Show resolved Hide resolved
ofrak_core/ofrak/core/gzip.py Outdated Show resolved Hide resolved
@alchzh alchzh marked this pull request as ready for review July 18, 2024 17:50
@alchzh alchzh requested a review from rbs-jacob July 18, 2024 17:51
Copy link
Member

@rbs-jacob rbs-jacob left a comment

Choose a reason for hiding this comment

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

This needs a changelog entry, but after that, should probably be good to go.

@alchzh alchzh requested a review from rbs-jacob August 7, 2024 14:43
Copy link
Contributor

@whyitfor whyitfor left a comment

Choose a reason for hiding this comment

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

See comments.

ofrak_core/test_ofrak/components/test_gzip_component.py Outdated Show resolved Hide resolved
ofrak_core/ofrak/core/gzip.py Outdated Show resolved Hide resolved
ofrak_core/ofrak/core/gzip.py Show resolved Hide resolved
@alchzh alchzh changed the title Rewrite gzip component to support trailing bytes without external tool. Addresses #476 Rewrite gzip component to support trailing bytes without external tool and compress with PIGZ. Addresses #476 Aug 14, 2024
@alchzh
Copy link
Contributor Author

alchzh commented Aug 14, 2024

Based on benchmarks of zlib module vs pigz, no benefit to using pigz for decompression, but significant benefit to using it for compression.

https://gist.github.com/alchzh/24ca62a6f87548ea2b1911c174e0ed54

Copy link
Contributor

@whyitfor whyitfor left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Especially like the profiling and data-driven approach.

ofrak_core/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@rbs-jacob rbs-jacob left a comment

Choose a reason for hiding this comment

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

I did not review the tests in great detail, but I like the way that this implementation ended up. Seems like a big improvement!

Copy link
Member

@rbs-jacob rbs-jacob left a comment

Choose a reason for hiding this comment

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

I relented and included one minor change that I would like to see before merging this.

ofrak_core/ofrak/core/gzip.py Outdated Show resolved Hide resolved
Copy link
Member

@rbs-jacob rbs-jacob left a comment

Choose a reason for hiding this comment

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

Thanks for this fix. Looks ready to merge once the tests pass!

@rbs-jacob rbs-jacob merged commit 89a9ae9 into redballoonsecurity:master Aug 15, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants