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.
|
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.
|
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.
|
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++?
|
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.
|
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:
- I'm guessing "/dev/null" is not portable
- when I force exec() to fail, it never exits
|
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.
|
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.
|
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.
|
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.
|
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.
|
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.
|
Tue 23 Feb 2016 05:40:02 PM UTC, comment #2:
Confirmed. I'll take a look at your patch sometime today.
|
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.
|
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.
|