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

Fix for Names truncated at 256 bytes #716

Closed
wants to merge 5 commits into from
Closed

Fix for Names truncated at 256 bytes #716

wants to merge 5 commits into from

Conversation

pmqs
Copy link
Contributor

@pmqs pmqs commented Jun 11, 2023

Partial fix for #690

Heavy use of malloc/realloc in this, so have tested this for leaks/corruption by hacking sanitizer into the build. There are some leaks around mz_crypt_sha_create and mz_zip_writer_entry_open that were already there, so haven't made it worse.

Can post the sanitizer patch if you like.

@pmqs
Copy link
Contributor Author

pmqs commented Jun 11, 2023

Created #717 to report the leaks and hacked sanitizer build is in https://github.com/pmqs/minizip-ng/tree/sanitizer

@nmoinvaz
Copy link
Member

I am second guessing whether or not just to use malloc or not. Because alloca requires headers depending on the platform. And also can cause problem/exeception if there isn't enough stack space. I agree it is unlikely, but who knows. alloca is more efficent if using lots of small files in a zip though.

@pmqs
Copy link
Contributor Author

pmqs commented Jun 11, 2023

Yep, I was in two minds about that myself. In the end I decided to avoid hitting the heap when I could keep it all on the stack with alloca. It will be exceptional use-cases of very long filenames/paths (which this fix is designed to address) that will be the problem.

If you think malloc is likely to have less portability issues, I can redo the change easily enough.

@pmqs
Copy link
Contributor Author

pmqs commented Jun 12, 2023

Changed the alloca to malloc for now.

@nmoinvaz
Copy link
Member

I have cleaned up and incorporated these changes.

@nmoinvaz nmoinvaz closed this Jul 31, 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

Successfully merging this pull request may close these issues.

2 participants