bugGNU Octave - Bugs: bug #47950, glob and dir functions are unable...

 
 

bug #47950: glob and dir functions are unable to escape square bracket characters in Windows

Submitter:  None
Submitted:  Wed 18 May 2016 02:48:46 AM UTC
   
 
Category:  Libraries Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  lachlan
Originator Name:  Richard Suchland Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * dev
Operating System:  * Microsoft Windows Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 21 Jul 2016 07:58:27 PM UTC, comment #40: 

Tried v3, worked fine, pushed here:
http://hg.savannah.gnu.org/hgweb/octave/rev/289409b2992d

closing report; thanks Lachlan for the patch & Rik for review

Philip Nienhuis <philipnienhuis>
Group Member
Mon 18 Jul 2016 06:09:31 PM UTC, comment #39: 

@Philip: Go ahead and push the patch and close the bug.

Rik <rik5>
Group administrator
Sat 16 Jul 2016 09:39:51 AM UTC, comment #38: 

With Lachlan's patch from comment #34, I get this (the OP's thing):

## cd to c:/tmp
>> status = mkdir ('c:\[test]')
status = 1
>> cd 'c:\[test]'
>> status = mkdir ('[test2]')
status = 1
>> cd c:/tmp
>> dir \[test]
error: Invalid call to test.  Correct usage is:

 -- test NAME
:
<snip>
:
>> dir /[test]
warning: dir: nonexistent directory '/[test]'
warning: called from
    dir at line 153 column 5
>> dir c:/[test]
.        ..       [test2]
>> dir c:\[test]
.        ..       [test2]
>>


from which I conclude:
(1) That this bug has been fixed.

(2) Apparently (I didn't know that) on Windows one needs to add the drive letter + colon to refer to files/directories in the root of a drive.

Do you guys want me to try the latest patch, or are you sufficiently confident to just push?

Philip Nienhuis <philipnienhuis>
Group Member
Sat 16 Jul 2016 04:04:03 AM UTC, comment #37: 

Thanks for the review, Rik.

I removed the redundant blank line, and replaced xstring_vector_value by string... but kept a check for nargin, albeit as


  if (args.length () == 0)
    return ovl ();


Although users has no cause to call _wglob_, they are able to.  If they call it with no arguments, then accessing argv(0) could lead to a segfault.

(file #37908)

Lachlan Andrew <lachlan>
Fri 15 Jul 2016 10:47:14 PM UTC, comment #36: 

Also, a double newline block.


+                            }
+
+
+                          retval[k++] = unescaped;
+                        }


Only one newline needed.

Rik <rik5>
Group administrator
Fri 15 Jul 2016 10:43:46 PM UTC, comment #35: 

@Lachlan: I think you can simplify the _wglob_ function.  It is an internal function only used by dir.m.  Since it will not be called directly by the user I think you can eliminated the input error checking.


+  if (args.length () != 1)
+    print_usage ();
+
+  string_vector pat = args(0).xstring_vector_value ("glob: PATTERN must be a string");
+


This could be just


+  string_vector pat = args(0).string_vector_value ();



Rik <rik5>
Group administrator
Fri 15 Jul 2016 12:36:21 PM UTC, comment #34: 

Oops.  I forgot to qrefresh.  Perhaps this time...



(file #37891)

Lachlan Andrew <lachlan>
Fri 15 Jul 2016 12:14:56 PM UTC, comment #33: 

Thanks for the testing Philip, and the diagnosis, Mike.  You're absolutely right -- the std::replace_if should have been removed from the code I copied-and-pasted.

A revised patch is attached.  Mike, did you want a version that has less repetition in oct-glob.cc by using flags there (comment #27)?  Let me know if this should be ready in time for 4.2.0.

(file #37890)

Lachlan Andrew <lachlan>
Fri 15 Jul 2016 10:48:42 AM UTC, comment #32: 

After a rebuild with the offending patch included (isn't even a cset) I have to confirm comment #28:

>> dname = 'c:\users\philip\java'
dname = c:\users\philip\java
>> __wglob__ (dname)
ans =
{
  [1,1] = c:usersphilipjava
}


Philip Nienhuis <philipnienhuis>
Group Member
Fri 15 Jul 2016 08:07:55 AM UTC, comment #31: 

I found I had that faulty build still installed at work.

What I get is:

>> dname = 'C:\users\Nienhu01\java'
dname = C:\users\Nienhu01\java
>> __wglob__ (dname)
ans =
{
  [1,1] = C:usersNienhu01java
}


To set the record straight:
It seems javaaddpath invoked directly works all right =>  it is the dir() command invoked through PKG_ADD (when loading io package) that gives problems.

I do intend to try Lachlan's patch again.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 15 Jul 2016 07:32:41 AM UTC, comment #30: 

You mean, with the "faulty" windows build?
I'd have to find it (I may have wiped it already), reinstall (I didn't keep it as it didn't "work" all right for me) it and try your suggestion. I'm two builds further now in the mean time.
Backing out + rebuild solved it for me. But I understand your question. As I wrote, previously the same cset didn't give problems.
If I can't find that build installer I'll make another test build, apply the patch, and try again.

Hang on.

[FYI / OT]
Keeping track of all Windows build variants for dev Octave is a bit of a pain.
They can't be installed side-by-side easily (there's a bug report about that) as Windows keeps versions in its registry and doesn't allow multiple invocations of the same SW with identical version numbers to be installed.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 14 Jul 2016 10:39:02 PM UTC, comment #29: 

Philip, so does this produce the same effect (assuming I got the real directory right)?


>> x = __wglob__ ('C:\Users\philip\java')
x =
{
  [1,1] = C:Usersphilipjava
}


If I'm reading the patch right the intent is to translate all backslash characters into slashes, call glob, and then translate slashes in the results back into backslashes.

Lachlan, is that right? Am I reading something wrong or does it appear that this patch has two blocks at the end that are doing the same thing, one using std::replace_if and one using a for loop?

Mike Miller <mtmiller>
Group Member
Thu 14 Jul 2016 07:52:00 PM UTC, comment #28: 

On Windows, with current dev sources, the cset has side effects:

>> pkg load io
warning: dir: 'stat (C:Usersphilipjava)' failed: No such file or directory
warning: dir: 'lstat (C:Usersphilipjavapoi-3.9-20121203.jar)' failed: No such file or
directory
warning: dir: 'lstat (C:Usersphilipjavapoi-3.11-20141221.jar)' failed: No such file or
 directory
:


(These are all calls to javaaddpath)
As you can see the file separators ("\") are gone.

Backing out the cset restores proper operation of javaaddpath.

Note: this is with hg id :67a5341edf15 ("mkstemp ....")

IIRC in earlier builds I also had this cset but I didn't note the issue above.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 30 Jun 2016 06:38:19 PM UTC, comment #27: 

Rik, is this what you meant by an entirely different function? That amount of duplication looks hard to maintain to me. I think adding an option to the octave::sys::glob function in liboctave would be clearer. Keeping the DEFUN function separate is good.

Other than that, patch looks good overall (untested).

Mike Miller <mtmiller>
Group Member
Mon 27 Jun 2016 10:43:09 AM UTC, comment #26: 

The attached patch does what Rik and Mike suggested in comment #25 and comment #24.

Since the goal is to imitate Windows dir, it also make "dir ." show all entries rather than only those containing a dot.

My understanding is that Windows "dir *" only lists those that have no extension.  I don't think we should copy that, but I'm happy to try.

(file #37595)

Lachlan Andrew <lachlan>
Mon 23 May 2016 10:15:58 PM UTC, comment #25: 

I would vote for an entirely different function.  Handling options is clunky.  If the function is only to be used by dir.m then it can have an internal name like _wglob_ for Windows glob.

Rik <rik5>
Group administrator
Mon 23 May 2016 09:53:18 PM UTC, comment #24: 

Discussion with jwe on #octave about this bug resulted in us thinking we should not introduce more system-dependent functionality, and instead make the dir() function behave differently from the glob() function, regardless of the OS. I think the core problem, that dir() should work the same as the DOS command DIR, is addressed either way, but this way avoids the risk of accidentally introducing some inadvertent system-dependent behavior because of some other function or script using dir() or glob() in an unanticipated way.

So our thinking is to either introduce a new function that is similar to glob but only allows the asterisk and question mark wildcard characters to be passed down to the real glob C function, or to add an option that does the same such that it is only called from dir.m to do DOS/Windows-style wildcards.

This means the dir() function will behave the same on Unixy operating systems as well, and its documented wildcard matching behavior will be different from the glob() and ls() functions.

So I think the first step would be either duplicating octave::sys::glob or adding an optional parameter that changes its behavior as described and exposing that such that it can be called that way by dir.m.

Mike Miller <mtmiller>
Group Member
Mon 23 May 2016 09:37:45 PM UTC, comment #23: 

@Philip: Unfortunately it's not quite that simple.  The replace_if algorithm searchs for an element and replaces it with another element in a container.  For strings, the base element is a single character so you cannot replace it with two characters.

It still shouldn't be that hard, but given that there are two modifications, changing '\' to '/' and '[' to "\[", it may be better to use a single iterator loop over the string and copy the string into a new variable built up character by character.

Rik <rik5>
Group administrator
Mon 23 May 2016 08:52:39 PM UTC, comment #22: 

It's doubtful that Windows users are even aware that dir.m invokes glob().

But I'd expect glob(), when invoked explicitly from the command line or in user scripts, to work identically on all platforms.
If glob() can be made to work properly on Windows using [..] patterns, there's no reason to cripple dir.m for that platform other than the extra work to get glob() going.


Anyway, my C++ skills are a bit lacking so hopefully someone will pick this up.

Just curious, would it be s/th like:

:
              std::replace_if (xpat.begin (), xpat.end (),
                               std::bind2nd (std::equal_to<char> (), '['),
                               '\[');
:


and similarly for ']' ?

Philip Nienhuis <philipnienhuis>
Group Member
Mon 23 May 2016 07:05:19 PM UTC, comment #21: 

I think the change needs to be made in the octave::sys::glob function

http://hg.savannah.gnu.org/hgweb/octave/file/7190a26977cb/liboctave/util/oct-glob.cc#l69

in the same loop where backslashes are replaced with forward slashes. If the system is Windows but not Cygwin (the same condition that is already used there) square brackets should be prefixed with a backslash. It's unclear to me whether Windows users would like this change to apply to both the dir() and glob() functions, or whether glob() is expected to use Unix paths and Unix wildcards, while dir() is expected to behave like the Windows command of the same name.

Mike Miller <mtmiller>
Group Member
Mon 23 May 2016 07:00:39 PM UTC, comment #20: 

I agree with Mike's 2nd suggestion as well.

The current behavior is unacceptable as '[' and ']' are valid file name characters on Windows.
I played a little with glob() and dir.m on both Windows and Linux but it still isn't clear to me where the change should be made.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 23 May 2016 06:39:08 PM UTC, comment #19: 

I agree with comment #18.  I very much doubt that most Windows users even know about the use of brackets to represent character classes.  I think we should fix this in the simplest, most intuitive method possible.  For me, that would be adding a rule to escape the brackets so that a '[' is always a literal character on Windows platforms.

Rik <rik5>
Group administrator
Mon 23 May 2016 05:44:15 AM UTC, comment #18: 

Yes, this bug is about the inability to properly escape the left and right square bracket characters so they can be passed as literal filename characters on Windows.

On Windows, as you've confirmed, asterisk and question mark are not allowed as literal characters, so there is no need to escape them. That leaves only the left and right square brackets (updated summary again).

The normal behavior of the Windows shell and DIR command doesn't cover the use of bracketed character sets, so I would argue that that is a Unix extension and not something that most Windows users would expect to work (unless they are working in a Cygwin or Msys bash shell).

I see two ways to address this issue, either document that there is no way to use the glob() or dir() functions on filenames that contain square brackets (document the current behavior), or add a rule that always escapes square brackets in filenames on Windows (not Cygwin) before passing the string to the glob function, and document that OS-specific difference.

Mike Miller <mtmiller>
Group Member
Sun 22 May 2016 08:05:11 PM UTC, comment #17: 

@Mike:

AFAIK, no.
See here (https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247%28v=vs.85%29.aspx#naming_conventions) for an overview of forbidden filename characters:

<
>
:
"
/
\
|
?
*
...and anything with a uint8 value below 32.


I got the impression that this bug isn't so much about allowed characters in path & file names but rather about characters allowed/used in globbing patterns (hence the issue with '[' and ']').
In MSYS, globbing simply works. For a directory tree:

c:\tmp\octave\dirab.txt
c:\tmp\octave\dirac.txt

I get:

philip@deskprn /c/tmp/octave
$ ls dira[bd].txt
dirab.txt

philip@deskprn /c/tmp/octave
$ ls dira[bdef].txt
dirab.txt

philip@deskprn /c/tmp/octave
$ ls dira[acf].txt
dirac.txt


so I'd somewhat naively guess that if Octave invokes the file I/O in the mingw libraries, globbing in Octave should simply work on Windows as well.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 22 May 2016 06:27:42 PM UTC, comment #16: 

Here's a more basic question, does Windows let you create file names that contain an asterisk or a question mark? Let's put aside left and right square brackets for now.

If Windows lets you create a file literally named 'all*.txt', how would you escape the asterisk to differentiate from a wildcard pattern? If Windows does not allow those characters in filenames at all, then there is no issue with escaping them.

According to a couple of pages, Windows only calls '*' and '?' wildcard characters, it does not include character sets in square brackets as is normally expected on Unix-like systems.

https://technet.microsoft.com/en-us/library/bb490639.aspx
https://en.wikipedia.org/wiki/Wildcard_character

So it sounds like this may be an OS-specific expectation that we could support for a better experience. Maybe we need to make the set of wildcard characters supported by the 'dir' function system-dependent, maybe by adding an option to glob to not interpreter left and right square brackets as a character class (basically always escape them on Windows if and only if called from the dir function).

I am not that motivated to work on such a change, but if anyone agrees that this would be best for native Windows users and wants to work on such a change I think it would be acceptable.

Mike Miller <mtmiller>
Group Member
Sun 22 May 2016 03:55:20 PM UTC, comment #15: 

@Lachlan, comment #14:
On MinGW both '\' and '/' are valid in (Windows) file names and most Octave commands working on/with files and directories treat  '\' and '/' as file separators in single-quoted path/file names.

Escaping characters preceded by a backslash in Windows file/path names can be avoided by using sq-strings. But if escaping characters is required, dq-strings, with double backslashes for backslashes that need to survive escaping, is the best and maybe the only option AFAIK.
I think this is an unavoidable (IMO little) issue that Windows users have to be aware of.

Could it help if on Windows, glob() would only convert '\' into '/' for sq-strings?  Or is sq-to-dq string conversion performed before handing the input string to glob() (as the code snippet in #comment 8 seems to suggest)?

Philip Nienhuis <philipnienhuis>
Group Member
Sat 21 May 2016 12:16:23 AM UTC, comment #14: 

We could use the fact that '/' is not valid in Windows file names.
A possible rule would be

- In any string that contains a '/', don't translate '\', regardless of OS.  That way, if a string is using unix-like directory separators, it can use unix-like escapes even on Windows systems.  The only ambiguity would arise if the string mixed the two on a Windows system.

This would fail in Mike's example of comment #3, but that could easily be replaced by  dir ('./\[abc\]').

Lachlan Andrew <lachlan>
Fri 20 May 2016 06:42:36 PM UTC, comment #13: 

That actually sounds easy, but the problem is with a string like


glob ('dir\*a*.txt')


Is the user looking for all files named '*a*.txt' in a directory named 'dir'? Or is the user looking for a match against files starting with the literal string 'dir*a' and ending with '.txt'? I don't think there is a way to resolve this ambiguity on a system where the same character is used for escaping and for directory separators.

Mike Miller <mtmiller>
Group Member
Fri 20 May 2016 06:23:00 PM UTC, comment #12: 

Yes, there is no easy fix.  We could attempt to write a more intelligent replacement function that would look at the character after the (file separator|escape character) and try to choose which one it was.  Doesn't sound like fun.

Rik <rik5>
Group administrator
Fri 20 May 2016 05:15:55 PM UTC, comment #11: 

Oh, presumably because glob doesn't handle \ as a directory separator.

But then if it did handle \ as a directory separator, it won't work as an escape character either.

So I don't see how to fix this other than using a different type of quoting.

John W. Eaton <jwe>
Group administrator
Fri 20 May 2016 05:06:15 PM UTC, comment #10: 

What was the purpose of translating all \ to /?

John W. Eaton <jwe>
Group administrator
Fri 20 May 2016 04:54:54 PM UTC, comment #9: 

Confirmed.  I just fired up a virtual machine and I can see this behavior.  I now really believe it is all about the replacement of the file separator as posted in comment #8.

This code, which avoids any backslashes, works.


>> glob ('[[]test]')
ans =
{
  [1,1] = [test]
}



Rik <rik5>
Group administrator
Fri 20 May 2016 03:37:09 PM UTC, comment #8: 

Just a guess, looking in liboctave/util/oct-glob.cc I see the following:


#if (defined (OCTAVE_HAVE_WINDOWS_FILESYSTEM) \
     && ! defined (OCTAVE_HAVE_POSIX_FILESYSTEM))
              std::replace_if (xpat.begin (), xpat.end (),
                               std::bind2nd (std::equal_to<char> (), '\\'),
                               '/');
#endif

              int err = gnulib::glob (xpat.c_str (), GLOB_NOSORT, 0, &glob_info);


It appears that on Windows systems we change all path names that use the '\' character as a file separator in to '/' before we call glob so that the backslash is not treated as an escape character.

In this case, we want '\[' to escape the special meaning of the bracket but it is being turned in to '/['.

Rik <rik5>
Group administrator
Wed 18 May 2016 01:33:47 PM UTC, comment #7: 


>> glob('c:/\[test\]')

ans = {}(0x0)
No Joy

Richard Suchland <suchland>
Wed 18 May 2016 05:46:55 AM UTC, comment #6: 

The dir function uses glob to get the list of files in a directory and it seems to be failing on Windows systems.

So you should see the issue if you do


mkdir ('[test]')
glob ('\[test\]')


If glob is fixed, then dir should work.

John W. Eaton <jwe>
Group administrator
Wed 18 May 2016 04:09:46 AM UTC, comment #5: 


>> # per Mike Miller's first suggestion
>> dir('c:/\[test\]');

warning: dir: nonexistent directory 'c:/\[test\]'
warning: called from
    dir at line 153 column 5
No Joy

>> # per Mike Miller's second suggestion
>> dir("c:/\\[test\\]");

warning: dir: nonexistent directory 'c:/\[test\]'
warning: called from
    dir at line 153 column 5
No Joy

Richard Suchland <suchland>
Wed 18 May 2016 03:59:05 AM UTC, comment #4: 

Since double quotes are the "preferred" quotes in Octave, perhaps the manual should explicitly mention that the backslash needs to be quoted.

If Richard reports that that fixes it for him, I'll write the patch.

Lachlan Andrew <lachlan>
Wed 18 May 2016 03:06:23 AM UTC, comment #3: 

Can you test with either single quotes


>> dir ('\[abc\]')


or with double quotes and double backslashes


>> dir ("\\[abc\\]")


If the backslash is quoted or escaped properly it works for me on Debian.

Mike Miller <mtmiller>
Group Member
Wed 18 May 2016 02:56:02 AM UTC, comment #2: 

쳌⟓ꇇ>> status=mkdir('c:/[test]');

>> status=mkdir('c:/[test]/abc');
>> #from the Octave 4.0 User Manual
>> #directory is subject to shell expansion if it contains any wildcard characters `*', `?',
>> #`[]'. To ?nd a literal example of a wildcard character the wildcard must be escaped
>> #using the backslash operator `\'.
>> dir("c:/\[test\]");

warning: dir: nonexistent directory 'c:/[test]'
warning: called from
    dir at line 153 column 5

>> # no joy
>> #other interpretation of escaped wildcard
>> dir("c:/\x5Btest\x5D");

warning: dir: nonexistent directory 'c:/[test]'
warning: called from
    dir at line 153 column 5

>> # no joy

Richard Suchland <suchland>
Wed 18 May 2016 02:54:43 AM UTC, comment #1: 

Forgot to login when creating this report.
The correct Name and Email are:
Submitted by: Richard Suchland
Email: -email is unavailable-

Richard Suchland <suchland>
Wed 18 May 2016 02:48:46 AM UTC, original submission:  

Here is an example of the dir function failing:


status=mkdir('c:/[test]');
status=mkdir('c:/[test]/abc');

#from the Octave 4.0 User Manual
#directory is subject to shell expansion if it contains any wildcard characters ‘*’, ‘?’,
#‘[]’. To find a literal example of a wildcard character the wildcard must be escaped
#using the backslash operator ‘\’.
dir("c:/\[test\]");
# no joy

#other interpretation of escaped wildcard
dir("c:/\x5Btest\x5D");
# no joy

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #37908:  bug_47950_wglob.v3.cset added by lachlan (7KiB - text/x-diff)
file #37891:  bug_47950_wglob.v2.cset added by lachlan (7KiB - text/x-diff)
file #37595:  bug_47950_wglob.cset added by lachlan (7KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by suchland (Posted a comment)
  •  

    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 20 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-21 philipnienhuis StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2016-07-16 lachlan Attached File- Added bug_47950_wglob.v3.cset, #37908
    2016-07-15 lachlan Attached File- Added bug_47950_wglob.v2.cset, #37891
    2016-07-15 lachlan Attached File#37890 Removed
    2016-07-15 lachlan Attached File- Added bug_47950_wglob.v2.cset, #37890
    2016-06-27 lachlan Attached File- Added bug_47950_wglob.cset, #37595
        StatusConfirmed Patch Submitted
        Assigned toNone lachlan
        Release4.0.0 dev
    2016-05-23 mtmiller Summaryglob and dir functions fail with escaped wildcard characters in Windows glob and dir functions are unable to escape square bracket characters in Windows
    2016-05-22 philipnienhuis Carbon-Copy- Added philipnienhuis
    2016-05-20 rik5 StatusNeed Info Confirmed
    2016-05-18 mtmiller CategoryOctave Function Libraries
        Summarydir function fails with wildcard characters glob and dir functions fail with escaped wildcard characters in Windows
    2016-05-18 lachlan Originator Name Richard Suchland
        Originator Email -email is unavailable-
        Operating SystemAny Microsoft Windows
    2016-05-18 mtmiller StatusNone Need Info
        Operating SystemMicrosoft Windows Any

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code