bugGNU Octave - Bugs: bug #47232, zip fails if filename has...

 
 

bug #47232: zip fails if filename has whitespace

Submitter:  Andreas Weber <andy1978>
Submitted:  Tue 23 Feb 2016 08:41:09 AM UTC
   
 
Category:  None Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  andy1978
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 25 Mar 2016 05:49:23 PM UTC, comment #14: 

Grrr.  My changeset for this broke certain cases where the third option of having all files relative to a different rootdir was present.  I committed another cset which fixes that (http://hg.savannah.gnu.org/hgweb/octave/rev/6187f1d2ca13).  It definitely seems like improving system to allow a cell array of arguments would be a good thing.

Rik <rik5>
Group administrator
Tue 22 Mar 2016 09:28:19 PM UTC, comment #13: 

I pushed a cset (http://hg.savannah.gnu.org/hgweb/octave/rev/50255c612915) that wraps file names in double quotes before passing them to the shell.  This allows handling of filenames with whitespace.  I think this is a "good enough" solution.

If the feature request in #47239 is ever implemented then zip.m can be re-coded to make use of it.

Rik <rik5>
Group administrator
Tue 01 Mar 2016 10:52:20 PM UTC, comment #12: 

I acknowledge that this is a hack, but it is in the nature of the 80/20 rule.  I think if we get 99% of the stuff right then I'm prepared to let the 1% be done by a dedicated utility outside of Octave (Winzip, etc.).  If someone wants to put dollar signs, exclamation points, spaces, and other characters reserved by the shell into their filename then they will need to accept that the shell is going to make their life difficult.

Maybe take up Feature Request #47239 (which you reported).  If system is extended to accept a cell array of arguments then it would be simple to update this.

Rik <rik5>
Group administrator
Mon 29 Feb 2016 10:40:20 PM UTC, comment #11: 

I still don't think that addresses the real issue which is double guessing the shell.  There's still shell special characters even within quotes such as dollar sign and backticks.

Maybe I'm very biased today because I wasted several hours this weekend on this exact issue for another program.  This other program did exactly what you're suggesting; quotes around the arguments, then escaped the quotes inside the string, and also escaped several other special characters.  It even has a special extra logic for Windows because apparently escaping quotes in that OS is not always simple. In the end, it was overdoing it and escaped brackets before spaces within quotes (this whole thing was made worse because what the program runs and what shows to be running do not match (unknown to me, this was by design) which made debugging very hard, and me even more frustrated).

But even if I am biased, there's still a story there which is that escaping things correctly is harder than it looks. Googling for rules on how to escape shell commands in perl and python, point to avoid it completely in the first. If there's a solution that allows to bypass the problem, then it sounds like the right solution.

Are the issues with fork, exec, waitpid at the Octave interpreter in Windows also true in C++?

Carnë Draug <carandraug>
Group Member
Mon 29 Feb 2016 08:05:09 PM UTC, comment #10: 

Support for fork() is sketchy on all operating system other than Linux.  Even Macs, based on FreeBSD, have some issues.  I'd be scared of going down that path.

What about just escaping the quote characters if necessary?

I used the following code in zip.m


  files = glob (files);                   # to expand wildcards
  files = regexprep (files, '"', '\\"');  # to escape double quotes
  cmd = sprintf ('zip -r "%s" %s', zipfile, sprintf (' "%s"', files{:}));


I then ran it in a directory with these file names


simple test2
simple test1
this file "has a quote" in it


The code below ran fine and I verifed the zip archive.


zip ('my.zip', {'simple*', 'this file*'})




Rik <rik5>
Group administrator
Mon 29 Feb 2016 06:38:53 PM UTC, comment #9: 

The suggested fix of adding quotes to support spaces, will still cause issues if the filename has quotes.

While system() does not accept a list of arguments to avoid the shell, exec does.  The following works for me.


-  rootdir = tilde_expand (rootdir);
+  zipfile = make_absolute_filename (zipfile);
+  files = glob (files);

-  zipfile = make_absolute_filename (zipfile);
-
-  cmd = sprintf ("zip -r %s %s", zipfile, sprintf (" %s", files{:}));
-
-  origdir = pwd ();
-  cd (rootdir);
-  [status, output] = system (cmd);
-  cd (origdir);
-
-  if (status)
-    error ("zip: zip failed with exit status = %d", status);
+  [pid, msg] = fork ();
+  if (pid < 0)
+    error ("zip: unable to fork in prepartion to call zip");
+  elseif (pid == 0)
+    cd (tilde_expand (rootdir));
+    dup2 (fopen ("/dev/null", "w"), stdout);
+    err = exec ("zip", {"-r", zipfile, files{:}});
+    exit (err);
+  else
+    [pid, status, msg] = waitpid (pid);
+    if (pid < 0)
+      error ("zip: failed to exec zip (exit code %i): %s", status, msg);
+    endif


It has the following issues though:

  1. I'm guessing "/dev/null" is not portable
  2. when I force exec() to fail, it never exits
Carnë Draug <carandraug>
Group Member
Tue 23 Feb 2016 07:23:03 PM UTC, comment #8: 

The feature request seems like a good idea.  In the short term, Andreas' idea seems like an easy fix.

Rik <rik5>
Group administrator
Tue 23 Feb 2016 07:13:56 PM UTC, comment #7: 

I have proposed a feature request (see bug #47239), for a usage of system() without shell, that should prevent this type of bugs.

Carnë Draug <carandraug>
Group Member
Tue 23 Feb 2016 07:09:59 PM UTC, comment #6: 

@Rik: yes, seems a good idea. I'll have a look at zip and the other packing/unpacking functions the next days.

Andreas Weber <andy1978>
Group Member
Tue 23 Feb 2016 07:05:22 PM UTC, comment #5: 

@Andreas: Our comments passed within 30 seconds of each other.

What if you add a call to glob first to expand any wildcards?  This seems to work. 


cmd = sprintf ('zip -r "%s" %s', zipfile, sprintf (' "%s"', glob (files){:}));



Rik <rik5>
Group administrator
Tue 23 Feb 2016 06:49:41 PM UTC, comment #4: 

@Andreas: Your patch is correct.  Please apply it.

In addition, could you take a look at the related functions of gzip, bzip2, and tar?  They break down in a similar manner when there is a space in the input.

tar.m can use the exact same fix since it has the syntax:


cmd = ... sprintf (...);


gzip and bzip2 both rely on the private function _xzip_.m, but check line 73.  The fix below seems to work there as well.


cmd = sprintf (commandtemplate, sprintf (' "%s"', fnames{:}));



Rik <rik5>
Group administrator
Tue 23 Feb 2016 06:49:12 PM UTC, comment #3: 

@Rik: Thanks.

But I now see that my proposed fix breaks wildcard, for example


zip ("foo.zip", "*")


won't work anymore.

Andreas Weber <andy1978>
Group Member
Tue 23 Feb 2016 05:40:02 PM UTC, comment #2: 

Confirmed.  I'll take a look at your patch sometime today.

Rik <rik5>
Group administrator
Tue 23 Feb 2016 08:46:18 AM UTC, comment #1: 

in unpack.m the cmd is

'gzip -d -v -f -r "%s"'
'tar xvf "%s"'
'bzip2 -d -c "%s" | tar xvf -'


and so on. Thus I think

cmd = sprintf ('zip -r "%s" %s', zipfile, sprintf (' "%s"', files{:}));


would be consistent here. I would commit this change to default the next days.

Andreas Weber <andy1978>
Group Member
Tue 23 Feb 2016 08:41:09 AM UTC, original submission:  


fn1 = "simple test1";
fn2 = "simple test2";
tmp = pi; save (fn1, "tmp")
tmp = e; save (fn2, "tmp")

zip ("simp.zip", {fn1, fn2})


fails with

error: zip: zip failed with exit status = 12
error: called from
    zip at line 66 column 5


I suggest following change but I'm unsure if quoting should be done with single or double quotes.



Andreas Weber <andy1978>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #36434:  zip_quote.diff added by andy1978 (438B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by andy1978 (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-03-22 rik5 StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2016-02-23 rik5 StatusPatch Submitted In Progress
        Assigned toNone andy1978
    2016-02-23 andy1978 Attached File- Added zip_quote.diff, #36434

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code