search

zip-rs / zip-old

Zip implementation in Rust
MIT License
732 stars 204 forks source link

Wrong path separator in creating zip on Windows #103

Closed jonpas closed 5 years ago

jonpas commented 5 years ago

In https://github.com/synixebrett/HEMTT (https://github.com/synixebrett/HEMTT/issues/91) we discovered zip-rs uses path separator of whatever system zip writing is on (/ on Linux, \ on Windows). This is strictly about creating a zip file, not unpacking it.

This is a problem when creating a zip on Windows and then unzipping it using unzip CLI tool, which will read all paths as file names due to \ use. Other way around is not a problem, as zip specification expects forward slashes / on all operating systems.

zip-rs doesn't seem to do any changes from my quick check, therefore it uses OS-specific separator as dictated by Rust itself. It should however replace all backslashes in paths with forward slashes to comply with zip specification. (Correct me if I missed something.)

I can make a PR that fixes that as soon as I get some input from authors and/or other contributors on this project, to make sure I am going in the right direction.

mvdnes commented 5 years ago

This library takes filenames as they come from the caller. It only modifies them when creating a directory, in which case it adds a '/' when the name does not end on '/' or '\'.

I see that in your code you just call to_str, so I believe that the error lies there. On Windows the path seperator should be changed to '/'.

However, I do think that accepting a Path as input to 'start_file' and 'start_directory' might be a good idea to prevent these issues in the future.

mvdnes commented 5 years ago

I have updated the library to add the functions start_file_from_path and add_directory_from_path. This should do the thing you where expecting to happen.

jonpas commented 5 years ago

I see, thanks for that. I mostly copied it from example, might be worth using that in the examples or adding a comment about that.

jonpas commented 5 years ago

I missed that example was updated, looks good to me, can close!

mvdnes commented 5 years ago

Yes, but I had only updated it after your bug report. Thank you for noticing it!