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

Arpy based ar extractor #1134

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Arpy based ar extractor #1134

wants to merge 3 commits into from

Conversation

e3krisztian
Copy link
Contributor

No description provided.

@e3krisztian e3krisztian requested a review from qkaiser February 20, 2025 20:36
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 20, 2025
View reviewed changes
qkaiser
qkaiser reviewed Feb 21, 2025
View reviewed changes
Copy link
Contributor

@qkaiser qkaiser left a comment

Choose a reason for hiding this comment

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

Do you think it would be possible to integrate this patch viraptor/arpy@23faf88 in this MR ? Maybe in a similar way than what we did when fixing traversals in tarfile ?

@qkaiser
Copy link
Contributor

qkaiser commented Feb 21, 2025

@vlaci I'm confused, why are we pulling gdb as a dependency here ? https://github.com/onekey-sec/unblob/actions/runs/13444205574/job/37565632238

@qkaiser qkaiser requested a review from kukovecz February 21, 2025 08:13
@vlaci
Copy link
Contributor

vlaci commented Feb 21, 2025

@vlaci I'm confused, why are we pulling gdb as a dependency here ? https://github.com/onekey-sec/unblob/actions/runs/13444205574/job/37565632238

It comes in through devenv's languages.rust feature. I am considering going back to the good old days of our own nix-shell definition, as there is a bit much of a churn around devenv stuff.

@e3krisztian
Copy link
Contributor Author

Do you think it would be possible to integrate this patch viraptor/arpy@23faf88 in this MR ? Maybe in a similar way than what we did when fixing traversals in tarfile ?

Unfortunately it requires patching arpy itself.

I know, the PR is there, and there was no response to it, but maybe we could ask @viraptor to take a look at this PR.

If we get no response, we could also use a patched fork, or a patched vendored version.

@e3krisztian
refact(file_utils): relax iterate_file and carve requirements
ce1d748 
File is a too strict requirement for custom extractors, which might
depend on other file-like objects, like arpy.ArchiveFileData.
@e3krisztian
refact(archive/ar): extract with pure-Python arpy instead of unar
ed9e21f 
Although arpy looks unmaintained, it is a readable implementation.
It will be easier to fix problems there/in a fork, than in unar/7z.
@e3krisztian
test(handlers/archive/ar): copy over arpy test files
0a684d0
@e3krisztian e3krisztian force-pushed the arpy-based-ar-extractor branch from 61a1fcb to 0a684d0 Compare February 21, 2025 15:35
@e3krisztian
Copy link
Contributor Author

Renamed Seekable to RandomReader, as I did not like the name.

github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 21, 2025
View reviewed changes
@qkaiser
Copy link
Contributor

qkaiser commented Feb 21, 2025

@e3krisztian left a message on the inital PR to arpy, let's wait for devenv to fix their stuff (or use less of it) so the Nix CI passes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qkaiser qkaiser qkaiser left review comments

@kukovecz kukovecz Awaiting requested review from kukovecz

At least 1 approving review is required to merge this pull request.

Assignees

@e3krisztian e3krisztian

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@e3krisztian @qkaiser @vlaci