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

 
 

bug #47232: zip fails if filename has whitespace

Submitted by:  Andreas Weber <andy1978>
Submitted on:  Tue 23 Feb 2016 08:41:09 AM UTC  
 
Category: NoneSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: Andreas Weber <andy1978>
Originator Name: Open/Closed: Closed
Release: devOperating System: Any

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project 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>
Project 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>
Project 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>
Project 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

I then ran it in a directory with these file names

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

Rik <rik5>
Project 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.

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>
Project 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>
Project 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>
Project 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>
Project MemberIn charge of this item.
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.

Rik <rik5>
Project 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:

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

Rik <rik5>
Project 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

won't work anymore.

Andreas Weber <andy1978>
Project MemberIn charge of this item.
Tue 23 Feb 2016 05:40:02 PM UTC, comment #2:

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

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

in unpack.m the cmd is

and so on. Thus I think

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

Andreas Weber <andy1978>
Project MemberIn charge of this item.
Tue 23 Feb 2016 08:41:09 AM UTC, original submission:

fails with

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

Andreas Weber <andy1978>
Project MemberIn charge of this item.

 

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

Attach File(s):
   
   
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
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by andy1978 (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 22 Mar 2016 09:28:19 PM UTCrik5StatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Tue 23 Feb 2016 06:49:41 PM UTCrik5StatusPatch Submitted=>In Progress
      Assigned toNone=>andy1978
    Tue 23 Feb 2016 08:41:09 AM UTCandy1978Attached File-=>Added zip_quote.diff, #36434

    Back to the top


    Powered by Savane 3.1-cleanup1