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

Replace lz4 with small-lz4 #19849

Closed
trufae opened this issue Mar 21, 2022 · 6 comments
Closed

Replace lz4 with small-lz4 #19849

trufae opened this issue Mar 21, 2022 · 6 comments

Comments

@trufae
Copy link
Collaborator

trufae commented Mar 21, 2022

The official lz4 implementation is unnecessarily complex and hard to read, not to mention it segfaults when compiled with tinycc, probably because it depends on lots of tricky ifdefs. The algorithm is pretty simple, so it would be better if r2 ships a smaller, more readable and maintainable version of it. and the only option out there is this one:

https://create.stephan-brumme.com/smallz4/#git1

We can keep supporting system-wide lz4 dependency, or even expose the same public api, but right now, the tcc testsuite is disabled because the lz4 impl overflows

@trufae trufae added this to the 5.7.0 milestone Mar 21, 2022
@trufae
Copy link
Collaborator Author

trufae commented Apr 4, 2022

@trufae trufae modified the milestones: 5.7.0, 5.8.0 May 18, 2022
@MewtR
Copy link
Contributor

MewtR commented Feb 12, 2023

Is this issue still relevant? The problem with smallz4 is that it assumes that your input is a full LZ4 Frame. For example, if I try using it on the NSO binary from the testbins repository, it fails because it can't find the magic bytes.

Also, I was able to compile things with tcc and didn't face any issues. Can you provide a specific example of something that fails when compiling with tcc?

@trufae
Copy link
Collaborator Author

trufae commented Feb 17, 2023

i would say that is probably better to ship our own implementation of lz4, the current one is about 2500LOC which is not a lot, and most of it is wrongly indented and ifdefed code that we can probably cleanup. this code also contains asserts which are against the r2 developer guidelines. But small LZ4 is about 400 LOC and it probably fits better in r2 for our custom implementation. the less lines to maintain the better imho

If its just the magic header check we can probably skip the check and see if it doesnt breaks anything. not sure if we have many tests for that nso file 🤔

MewtR added a commit to MewtR/radare2 that referenced this issue Feb 25, 2023
* Patch smallz4 to return -1 on error (as opposed to just exit)
* Add new function 'unlz4Block_userPtr' that can decompress an lz4 block
* New '--with-smallz4' flag to compile and use smallz4 instead of lz4
MewtR added a commit to MewtR/radare2 that referenced this issue Feb 28, 2023
* Patch smallz4 to return -1 on error (as opposed to just exit)
* Add new function 'unlz4Block_userPtr' that can decompress an lz4 block
* New '--with-smallz4' flag to compile and use smallz4 instead of lz4
MewtR added a commit to MewtR/radare2 that referenced this issue Mar 5, 2023
* Patch smallz4 to return -1 on error (as opposed to just exit)
* Add new function 'unlz4Block_userPtr' that can decompress an lz4 block
* New '--with-smallz4' flag to compile and use smallz4 instead of lz4
trufae pushed a commit that referenced this issue Mar 15, 2023
* Initial smallz4 support, related to #19849
* Patch smallz4 to return -1 on error (as opposed to just exit)
* Add new function 'unlz4Block_userPtr' that can decompress an lz4 block
* New '--with-smallz4' flag to compile and use smallz4 instead of lz4
* Reuse the new unlz4Block_userPtr function to remove duplicate code from unlz4_userPtr in smallz4
* smallz4: remove patch, format small4cat.c properly
MewtR added a commit to MewtR/radare2 that referenced this issue Mar 18, 2023
@trufae
Copy link
Collaborator Author

trufae commented Apr 9, 2023

I think we are good to get rid of the non small implementation @MewtR do you want to go finish the task? Thanks!

@MewtR
Copy link
Contributor

MewtR commented Jun 15, 2023

Apologies for the lengthy absence. I'm on it.

@trufae
Copy link
Collaborator Author

trufae commented Jul 17, 2023

we can close this now :) thank you!! @MewtR

@trufae trufae closed this as completed Jul 17, 2023
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

No branches or pull requests

2 participants