bugGNU Octave - Bugs: bug #45816, canonicalize_file_name() changed...

 
 

bug #45816: canonicalize_file_name() changed behavior between 3.8.2 and 4.0.0 on windows

Submitter:  None
Submitted:  Mon 24 Aug 2015 07:08:42 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  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

Tue 22 Sep 2015 02:06:30 PM UTC, comment #25: 

I committed the patch here http://hg.savannah.gnu.org/hgweb/octave/rev/0829b6ff3ac9.  Closing report.

Rik <rik5>
Group administrator
Wed 16 Sep 2015 05:22:40 PM UTC, comment #24: 

@Mike: The patch seems fine if you want to go ahead and commit it.

Rik <rik5>
Group administrator
Mon 14 Sep 2015 08:05:36 PM UTC, comment #23: 

I went ahead and updated Rik's patch based on the feedback so far, completely untested but attached here for review. The more verbose #if conditional is intentionally copied from other conditionals in the same file for consistency, and to apply to Windows but not cygwin as we discussed.

(file #34879)

Mike Miller <mtmiller>
Group Member
Fri 04 Sep 2015 07:40:31 AM UTC, comment #22: 

@Mike:
My feeling from reading the cygwin docs (a.o. the stanzas quoted below in comment #20) is that your recommendation would especially apply to cygwin.
On Windows itself it isn't all that bad, provided Octave core functions that handle file paths invoke proper fileseps.

With this bug fix I suppose the OF-geometry issue should be solved as well.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 03 Sep 2015 08:30:33 PM UTC, comment #21: 

My own 2¢: I think the include should be moved to the top, easy to search on the same conditional showing up twice in the file rather than needing to keep it with the conditional code. Many other parts of our baseline do it that way.

My reading of the code for filesep is that it should be a forward slash ("/") on cygwin.

I also see that filesep has an "all" option to return multiple separators where it's ambiguous. This should be a string containing both forward and backward slashes, on both native Windows and cygwin environments.

So I think the recommendation would be for functions that need to parse path strings to use filesep("all") as the delimiter (see the fileparts.m function file). I'll copy this to the dependent bug as well for the geometry package.

Mike Miller <mtmiller>
Group Member
Thu 03 Sep 2015 07:57:05 PM UTC, comment #20: 

Great.  I will wait for word from jwe about whether the #include is just weird, or needs to be changed.

I checked the Cygwin documentation and I see this:


Mapping path names
Introduction

Cygwin supports both POSIX- and Win32-style paths. Directory delimiters may be either forward slashes or backslashes. Paths using backslashes or starting with a drive letter are always handled as Win32 paths. POSIX paths must only use forward slashes as delimiter, otherwise they are treated as Win32 paths and file access might fail in surprising ways.
Note

The usage of Win32 paths, though possible, is deprecated, since it circumvents important internal path handling mechanisms. See the section called “Using native Win32 paths” and the section called “Using the Win32 file API in Cygwin applications” for more information.


And the section "Using native Win32 paths" has this


Using native Win32 paths

Using native Win32 paths in Cygwin, while possible, is generally inadvisable. Those paths circumvent all internal integrity checking and bypass the information given in the Cygwin mount table.

The following paths are treated as native Win32 paths in Cygwin:

    All paths starting with a drive specifier

      C:\foo
      C:/foo

    All paths containing at least one backslash as path component

      C:/foo/bar\baz/...

    UNC paths using backslashes

      \\server\share\...

When accessing files using native Win32 paths as above, Cygwin uses a default setting for the mount flags. All paths using DOS notation will be treated as case insensitive, and permissions are just faked as if the underlying drive is a FAT drive. This also applies to NTFS and other filesystems which usually are capable of case sensitivity and storing permissions.


So it seems that it is generally advisable to use the Unix file separator on Cygwin.

Since this is a regression, I will push this to stable.

Rik <rik5>
Group administrator
Thu 03 Sep 2015 07:20:49 PM UTC, comment #19: 

@Rik:
Your patch works fine, both on Linux and Windows. On Windows it neatly converts forward slashes in its argument to backslashes.
I have no opinion placement of #include.

@comment #16:
I think what counts is cygwin's "native" filesep as reported by Octave, i.e., what does "filesep" return and what fileseps does e.g., "pwd" return?
(I don't cygwin anymore since several years so I wouldn't know)

If it were indeed '/' I agree this patch shouldn't work on cygwin.

Perusing the cygwin docs I get no clear answer; cygwin prefers '/' but does accept '\' and emulates some dumb file mode then (case-insensitive, FAT-like permissions etc).

Philip Nienhuis <philipnienhuis>
Group Member
Thu 03 Sep 2015 10:29:51 AM UTC, comment #18: 

I'll try to have a go at testing it tonight.

Philip Nienhuis <philipnienhuis>
Group Member
Wed 02 Sep 2015 08:42:04 PM UTC, comment #17: 

@jwe: Regarding the #include, I felt it was best to encapsulate all the the code related to this change within a single #if defined and marked by a comment about what the code was for.  I wasn't even sure the compiler was going to accept it, but it did.  Of course, the #include could be separated and put at the top of the file along with another #if defined you prefer.

Mostly hoping Philip or someone can test it and see if it works.

Rik <rik5>
Group administrator
Wed 02 Sep 2015 07:52:35 PM UTC, comment #16: 

I find it a little strange to #include a file in the middle of a function...

Maybe use


#if ! defined (OCTAVE_HAVE_POSIX_FILESYSTEM)


?  That would limit it to just Windows systems and not Cygwin.  I think people who are using Cygwin would generally be OK with using /.

John W. Eaton <jwe>
Group administrator
Wed 02 Sep 2015 07:27:10 PM UTC, comment #15: 

@Philip: Attached is an attempt at a solution.  Currently it applies to any system that defines _WIN32_ which would include Windows, MinGW, and Cygwin.  If users of those two systems like to have Unix file separators we could exclude them from this patch.

(file #34796)

Rik <rik5>
Group administrator
Wed 02 Sep 2015 11:34:31 AM UTC, comment #14: 

"filesep.tst" - maybe "windoze.tst"?  pathseps come to mind, drive letters, and possibly other M$ idiosyncrasies.

I'll make a start with it.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 01 Sep 2015 09:07:01 PM UTC, comment #13: 

It's already marked as a regression so that's fine.

Adding BIST tests for filesep consistency to make_absolute_filename and canonicalize_file_name would be good.  If there are a lot of tests, including other functions that you want to check like pwd or fullfile, it would be better to create a new general .tst file in the test/ directory to house them.  Maybe filesep.tst?


Rik <rik5>
Group administrator
Tue 01 Sep 2015 08:31:25 PM UTC, comment #12: 

@Rik comment #7
Based on e.g., bug #43979, it is Octave itself that got "peeved" on canonicalize_file_name's output, isn't it.

The problem is outlined in my post in the maintainers ML:
On Windows, Octave gets paths with backslash fileseps returned from e.g. "pwd" or "fullfile" commands that -once combined with canonicalize_file_name's output- induce problems when fed to yet other regular Octave functions that rely on consistent fileseps.
IOW, functions returning / or \ fileseps at will are unacceptable for several other often used Octave functions.

Too bad that there seem to be no filesep consistency tests in place for make_absolute_filename and canonicalize_file_name - if there were this would have been catched immediately at the time. If you want I'll try to come up with something.

BTW I suppose this is a real regression. Shall I mark it as such?

Philip Nienhuis <philipnienhuis>
Group Member
Tue 01 Sep 2015 05:20:01 PM UTC, comment #11: 

Wow, impressive use of Mercurial conditional syntax.  Mike is right, 18066 isn't in 3.8.2.

Rik <rik5>
Group administrator
Tue 01 Sep 2015 03:45:50 PM UTC, comment #10: 

No, the change was made on the default branch while 3.8 was on stable.

This is a better query:


$ hg log -r "9f6c9f3f1b62:: and tag()" --template "{tags}\\n"rc-4-0-0-1
rc-4-0-0-2
rc-4-0-0-3
rc-4-0-0-4
release-4-0-0



Mike Miller <mtmiller>
Group Member
Tue 01 Sep 2015 03:30:43 PM UTC, comment #9: 

Changeset 18066:9f6c9f3f1b62 should be in release 3.8.2. 


hg tags | grep 3-8-2
release-3-8-2                  18991:a66548dc07b0
rc-3-8-2-2                     18883:46df2c7b8f4d
rc-3-8-2-1                     18807:492c56149535


Rik <rik5>
Group administrator
Tue 01 Sep 2015 04:58:31 AM UTC, comment #8: 

Did 18066:9f6c9f3f1b62 make it into 3.8.2?

Anonymous
Mon 31 Aug 2015 10:41:42 PM UTC, comment #7: 

I agree with Mike that we don't want to re-write canonicalize_file_name() and that it should just be a thin wrapper over the gnulib function of the same name.  If everyone is really peeved about which way the slashes lean then Octave could use a second pass on top of canonicalize_file_name to sort them out as suggested by the gnulib maintainers.

There is a difference between canonicalize_file_name and make_absolute_filename in that the first checks for the existence of the file or directory.  If it does not exist it returns the empty string.  make_absolute_filename does no checking whatsoever and is only concerned with whether the input string looks like a valid path.

For those who need a cheap replacement,


tmp = canonicalize_file_name (str);
path = strrep (tmp, '/', '\');


Or, if you're feeling crafty, you can override the existing implementation with your own function in a .octaverc file.


canonicalize_file_name = @(x) strrep (builtin ('canonicalize_file_name', x), '/', '\')



Rik <rik5>
Group administrator
Mon 31 Aug 2015 05:25:27 PM UTC, comment #6: 

@Mike, comment #4:
Indeed, why have several functions for one and the same goal. Maybe one of the two should be deprecated (I'd favor make_absolute_filename to stay).

But IIRC there is a subtle difference between make_absolute_filename and canonicalize_file_name on Windows (apart from fileseps) that hit me some years ago in the io package.

BTW, none of the two is supported in Matlab (r2015b prerelese) => +1 for Octave there :-)
There's a GetFullPath on Matlab's File Exchange to fill the gap

Philip Nienhuis <philipnienhuis>
Group Member
Mon 31 Aug 2015 05:17:55 PM UTC, comment #5: 

@Rik, comment #3:

Maybe, but there's still the fact that Octave-3.8.2 does it right:

GNU Octave, version 3.8.2
Copyright (C) 2014 John W. Eaton and others.
This is free software; see the source code for copying conditions.
There is ABSOLUTELY NO WARRANTY; not even for MERCHANTABILITY or
FITNESS FOR A PARTICULAR PURPOSE.  For details, type 'warranty'.

Octave was configured for "x86_64-w64-mingw32".

Additional information about Octave is available at http://www.octave.org.

Please contribute if you find this software useful.
For more information, visit http://www.octave.org/get-involved.html

Read http://www.octave.org/bugs.html to learn how to submit bug reports.
For information about changes from previous versions, type 'news'.

>> filname = "tmp/a.txt"
filname = tmp/a.txt
>> canonicalize_file_name (filname)
ans = C:\Programs\Octave\octave64-3.8.2\tmp\a.txt
>>


Or does Octave use another gnulib branch than "master" that did change between 3.82 and 4.0.0 ?

Philip Nienhuis <philipnienhuis>
Group Member
Mon 31 Aug 2015 04:10:54 PM UTC, comment #4: 

Based on the response from upstream, it seems to me we should probably leave canonicalize_file_name alone. I would advocate for keeping the Octave function a thin wrapper around the C function, as we do for other functions.

If users want a function to return a canonical absolute path that also respects the native filesep conventions, that should probably be make_absolute_filename or some other not-yet-written post-processing wrapper around canonicalize_file_name.

And of course document the differences and why we have 2 or 3 different functions to construct an absolute path name :)

Mike Miller <mtmiller>
Group Member
Sun 30 Aug 2015 10:36:22 PM UTC, comment #3: 

Attached is canonicalize.c which I extracted from libgnu.  Everywhere in the code it seems to append '/' when it has to put in a file separator.  There does not seem to be any ifdef based on platform.  An example is line 205


if (!ISSLASH (dest[-1]))
  *dest++ = '/';


This still feels like an upstream bug, but with gdb someone should be able to step through the code and see what is happening.


(file #34770)

Rik <rik5>
Group administrator
Sun 30 Aug 2015 07:26:47 PM UTC, comment #2: 

(Too bad I hadn't noted this issue before I fixed bug #43979)

@Rik:
I've built Octave cf. your instructions. Indeed, cstdlib is included a few lines higher up in file-ops.cc.

But alas, it didn't help - the bug is still there.

I had a brief look in the gnulib repo where canonicalize.* live (http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib). It seems the last real code changes (apart from copyright updates) that affect their behavior date from December 2012 (canonicalize-lgpl from 2013 but only for Solaris). Octave-3.8.2 was released in summer 2014.
As both Octave-3.8.2 and 4.0.0 are from (much) later than the latest real code changes in the canonicalize*.* files in gnulib, but behave differently, my conclusion is that the issue must lie within Octave itself.

I'm not familiar with gdb (and the next weeks I lack time to dive into it); yet for those who are familiar with gdb and want to help I have an MXE Octave-4.1.0+ build f. 64-bit Windows with unstripped Octave in my Dropbox, the link is in patch #8656 comment #36. (Note that the dependencies are stripped.)
That snapshot was made for Michael Barnes (the variable editor "dev"), but to help solve this bug I'll wait a little longer with wiping that version from Dropbox - normally I'm not so in favor of distributing development snapshots.

Release => dev

Philip Nienhuis <philipnienhuis>
Group Member
Tue 25 Aug 2015 01:07:50 AM UTC, comment #1: 

Octave makes use of the gnulib module canonicalize to provide canonicalize_file_name.  There is also the module canonicalize-lgpl that might be used instead.  It's a long test, but conceptually simple.  Change bootstrap.conf to use the -lgpl module.  Change liboctave/system/file-ops.cc to remove 'include "canonicalize.h"' and replace it with "include <cstdlib>" which might already be included anyways, bootstrap the build, run configure, run make, run make dist to create a tarball, transfer over the tarball to an MXE directory and then run the build process there, and finally test the result in Windows.

My guess is that it is an upstream bug in gnulib and will need to be reported there.

Rik <rik5>
Group administrator
Mon 24 Aug 2015 07:08:42 PM UTC, original submission:  

Preface: I don't know what the correct behavoir is, but since there are users relying on the old behavoir (e.g.: #45813), I submitted this report here.

There has already been atleast one other bugs due to this change, but it got solved by moving away from canonicalize_file_name() to make_absolute_filename() (bug #43979

In short, the 3.8.2 version returned strictly backslashed paths on windows while the 4.0.0 version might mix slashes and backslashes depending on the source of the path fragment. Users (falsely?) relying on only having native path separators will fail with 4.0.0

system: Win7-64

3.8.2 (MXE build)

>> cd D:\folderA\folderB\

>> file="file_a.txt"
file = file_a.txt
>> canonicalize_file_name (file)
ans = D:\folderA\folderB\file_a.txt

>> cd ..

>> file="folderB\\file_a.txt"
file = folderB\file_a.txt
>> canonicalize_file_name (file)
ans = D:\folderA\folderB\file_a.txt

>> file="folderB/file_a.txt"
file = folderB/file_a.txt
>> canonicalize_file_name (file)
ans = D:\folderA\folderB\file_a.txt


4.0.0 (official build)

>> cd D:\folderA\folderB\

>> file="file_a.txt"
file = file_a.txt
>> canonicalize_file_name (file)
ans = D:\folderA\folderB/file_a.txt

>> cd ..

>> file="folderB\\file_a.txt"
file = folderB\file_a.txt
>> canonicalize_file_name (file)
ans = D:\folderA/folderB/file_a.txt

>> file="folderB/file_a.txt"
file = folderB/file_a.txt
>> canonicalize_file_name (file)
ans = D:\folderA/folderB/file_a.txt


Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #34879:  winslash.diff added by mtmiller (996B - application/octet-stream)
file #34796:  winslash.diff added by rik5 (753B - text/x-diff)
file #34770:  canonicalize.c added by rik5 (10KiB - text/x-csrc)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by None (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-09-22 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2015-09-14 mtmiller Attached File- Added winslash.diff, #34879
    2015-09-02 rik5 Attached File- Added winslash.diff, #34796
        StatusConfirmed Patch Submitted
    2015-08-31 philipnienhuis Dependencies- bugs #45813 is dependent
    2015-08-30 rik5 Attached File- Added canonicalize.c, #34770
        Summarycanonicalize_file_name() changed behavoir between 3.8.2 and 4.0.0 on windows canonicalize_file_name() changed behavior between 3.8.2 and 4.0.0 on windows
    2015-08-30 philipnienhuis StatusNone Confirmed
        Release4.0.0 dev

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code