Skip to content

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

Merged
rbs-jacob merged 20 commits into
redballoonsecurity:masterfrom
alchzh:python-gzip-trailing-bytes
Aug 15, 2024
Merged

Rewrite gzip component to support trailing bytes without external tool and compress with PIGZ. Addresses #476#485
rbs-jacob merged 20 commits into
redballoonsecurity:masterfrom
alchzh:python-gzip-trailing-bytes

Conversation

@alchzh
Copy link
Copy Markdown
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

Comment thread ofrak_core/test_ofrak/components/test_gzip_component.py Outdated
Comment thread ofrak_core/ofrak/core/gzip.py Outdated
@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
Copy Markdown
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
Copy Markdown
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.

Comment thread ofrak_core/test_ofrak/components/test_gzip_component.py Outdated
Comment thread ofrak_core/test_ofrak/components/test_gzip_component.py
Comment thread ofrak_core/ofrak/core/gzip.py Outdated
Comment thread ofrak_core/ofrak/core/gzip.py
Comment thread ofrak_core/ofrak/model/component_model.py Outdated
@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
Copy Markdown
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

Comment thread ofrak_core/ofrak/core/gzip.py
Copy link
Copy Markdown
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.

Comment thread ofrak_core/CHANGELOG.md Outdated
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread ofrak_core/ofrak/core/gzip.py Outdated
Copy link
Copy Markdown
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
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