search

r-lib / zip

Platform independent zip compression via miniz
https://r-lib.github.io/zip/
Other
83 stars 19 forks source link

Windows: Crash of cmdunzip when executable on different drive than exdir #45

Closed fproske closed 4 years ago

fproske commented 4 years ago

First of all, thank you a lot for this awesome package!

I just stumbled across a bug in Windows 10 that causes cmdunzip to crash. This crash occurs when the executable file cmdunzip is stored in a different drive (e.g. drive D:) than the exdir (e.g. drive C:). Example command: D:\r_lib\zip\bin\x64\cmdunzip.exe C:/test/test.zip C:/test I noticed that this only occurs if the path separator of the exdir is / instead of \.. This causes process_unzip to crash while unzip works just fine!

fproske commented 4 years ago

This probably happens because winslash="/" is used in the normalizePath function in unzip_process, while unzip() uses the default winslash="\\". Any particular reason why this is the case?

gaborcsardi commented 4 years ago

I don't quite follow. Are you calling the cmdunzip.exe file from outside of R? That is not really a supported use case.

fproske commented 4 years ago

No, I was calling unzip_process() and nothing got extracted. get_exit_status() returned something like -12309. My R installation including the library is located on the D: drive while the exdir is on the C: drive. When trying to isolate the problem, I noticed that running the executable cmdunzip with the same paths causes the executable to crash which explains the return code. However, running unzip() with the same arguments (zipfile and exdir) works just fine. I guess the issue is related to using forward slashes in combination with different partitions on Windows.

gaborcsardi commented 4 years ago

Got it, thanks. I'll try to reproduce and fix.

AshesITR commented 4 years ago

I can repro this using the usual unit test because my R development workspace is on D: and %TEMP% is on C:

Using normalizePath(exdir, winslash = "\") (the default) fixes the issue. Why use "/" in the first place?

gaborcsardi commented 4 years ago

Many of the tools that came from Unix work better with /. But in this we are definitely better off with \\.

gaborcsardi commented 4 years ago

@AshesITR Do you want to submit that as a PR?

AshesITR commented 4 years ago

@gaborcsardi I was lazy and pushed it into the keys PR already. If that's necessary, I could separate the patch, though.