bugGNU Octave - Bugs: bug #44682, Fullfile does not support network...

 
 

bug #44682: Fullfile does not support network share addresses

Submitter:  None
Submitted:  Tue 31 Mar 2015 09:40:43 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  philipnienhuis
Originator Name:  bmgf Originator Email:  -email is unavailable-
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

Sat 12 Dec 2015 11:53:15 AM UTC, comment #26: 

Backporting to stable would be fine with me, but see comment #17.

I can imagine that users perceive this as a bug fix rather than a new feature or some added functionality.

([OT] reading back the whole thread after I forgot all about it I find itremarkable that such an apparently tiny issue required so much attention 8-) )

Philip Nienhuis <philipnienhuis>
Group Member
Fri 11 Dec 2015 11:13:45 PM UTC, comment #25: 

This bug was fixed on the default branch and not on the stable.
Consider fix it in 4.0.1 version

Avinoam Kalma <avinoam>
Group Member
Sun 31 May 2015 08:04:27 PM UTC, comment #24: 

I changed the cset to use strncmp to find the '\\' prefix since it was 10X faster.  I checked in the overall cset on the development branch here (http://hg.savannah.gnu.org/hgweb/octave/rev/8b1884214798).

Rik <rik5>
Group administrator
Mon 25 May 2015 06:09:52 PM UTC, comment #23: 

Sorry here's the patch

(file #34092)

Philip Nienhuis <philipnienhuis>
Group Member
Mon 25 May 2015 06:09:14 PM UTC, comment #22: 

OK new changeset attached.

I've tried to keep the elegancy of the OP's solution. Tested on Windows and Linux, all 26 tests pass.

@Rik, will you review the patch?  If OK, I can push, or you can if you want.
Thanks

release => development cf. comment #17

Philip Nienhuis <philipnienhuis>
Group Member
Mon 25 May 2015 04:21:52 PM UTC, comment #21: 

I'm fine with the simpler solution, but then I'll add a stanza in the texinfo header that fullfile.m does not do any validation whatsoever.

Anyway the proposed (by the OP) solution fails several tests, due to Carnë's (fullfile's author) trick of strjoin-ing file parts with fileseps and afterward removing all double fileseps. That trick, ingenious in itself, introduces double fileseps all over the place, incl. in pos. 1 and 2.

So I'm afraid the final fix is going to be a little uglier :-(

I'll take some more time to think up a good solution. Maybe a preliminary count of leading fileseps (before the strjoin) plus an "if (nr_of_leading_fileseps >= 2)" would do the trick.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 25 May 2015 03:55:00 PM UTC, comment #20: 

I think it is okay to be simpler since it doesn't appear that Matlab does any better in this regard.

If the path needs to be verified afterwards then canonicalize_path_name() or exist() might be used.

Rik <rik5>
Group administrator
Mon 25 May 2015 09:51:49 AM UTC, comment #19: 

I think that it would be ok to go with simpler solution for few reasons:

- fullfile () in general is not validating paths, it expects that path parts are already valid.

- In MATLAB, fullfile('\\server') will return '\\server' which is maybe ok for someone as intermediate full path result.




Anonymous
Mon 25 May 2015 08:36:13 AM UTC, comment #18: 

(picking up again)

@Rik:
The OP's suggestion is a bit too simple as even on Windows, UNC file path constructs should have a first part of the form '\\server\sharename', at the very least ~ '\\a\b'. Also some tests have to be adapted.

So, should we include a check for valid UNC full filenames, incl. a warning/error message, or let valid filenames be the responsibility of the user?

In case of the former a fairly simple regexp would do but then I'd also check for valid characters in at least 'server' and 'share', something like:

! isempty (regexp (filename, '\\\\[^<>:"/\\|\?\*]+\\[^<>:"/\\|\?\*]+.*', "match"))


In the latter case even '\\' would be a possible result of fullfile.m.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 21 May 2015 08:38:43 PM UTC, comment #17: 

It's simple enough, but I would put in on the development branch.  It's more along the lines of an improvement, and closer conformity to Matlab, than a bug fix where 2 + 2 = 5.

Rik <rik5>
Group administrator
Thu 21 May 2015 08:33:15 PM UTC, comment #16: 

Right, I'll prepare a changeset according to the OP's suggestion as it the fix is only required for Windows.

My original cs is a bit overdone and I think invalid or incomplete anyway for *nix.

Rik,
1. - You want the fix on stable?
2. - Shall I also add a check for the max file/path name length on Windows? (260 chars incl. drive letter IIRC, I'll look it up) ?

I'm not sure I can fix this the coming Easter weekend, next week is stuffed as well, but this looks to be a minor fix.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 21 May 2015 12:05:30 AM UTC, comment #15: 

@Philip: The promotion of the logical returned from ispc to numeric is automatic in Octave and is fine.  Did you want to re-work your patch, or is it good already?

Rik <rik5>
Group administrator
Thu 30 Apr 2015 09:15:49 PM UTC, comment #14: 

AFAICS now, on Linux server/share constructs aren't valid. Samba seems to require

smb://server/share

syntax, which that can be shared in the URL syntaxes like file:///some/file, ftp://server, etc. that aren't supported by Matlab anyway

That means the fix by the OP would in principle be fine.
My only (minor) concern is that ispc() returns a logical but is supposed to be interpreted as integer 1 or 0 on Windows. If that is fine with the core devs I'll prepare a changeset.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 07 Apr 2015 09:29:14 AM UTC, comment #13: 

Rik
@comment #11:
IMO responsibility for valid paths in case of ('/', '/abc')-constructs should be the responsibility of the user, not of fullfile.m; after all the form of the proposed fullfile.m result represents valid path name (UNC) syntax on Windows.
I foresee that such constructs could be cooked up by scripts, so your question is quite right.


@comment #12:
What usually happens is that on at least Windows network paths, drive letters (and partial paths from there like "f:\path\to\shared\folder") are replaced by share names that appear right after the server name:
\\server\sharename\path\to\subdir\relative\to\network\share

\\serverc:\tmp\dir1\dir2 is syntax I've never seen on Windows.
I'm familiar with the other two \\?\ UNC forms, but they're rarely if ever used by ordinary users, I've only used them when poking around in the mountmgr section of the Windows registry.

And then we have the URL forms I mentioned in a previous post:

file:///some/path
ftp://server/path
etc


I think those \\?\ and file:/// and URL path forms had better be left for a future fullfile.m upgrade, as soon as Matlab supports it as well.


Right now the relevant thing is whether server/share/file is valid on Linux (e.g., samba etc when referring to shares on Windows hosts).
If not, the suggestion by the OP will work fine and is the simplest; yet I do not like adding integers and logicals (1 + ispc()).
BTW the OP's mod would also morph ('\', \ abc') into '\\abc'.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 06 Apr 2015 11:59:14 PM UTC, comment #12: 

UNC looks more cumbersome than just '\\'.  See http://en.wikipedia.org/wiki/UNC_path.


fullfile ('\\server\c:\tmp', 'dir1', 'dir2')
ans = \\serverc:\tmp\dir1\dir2


If this is valid syntax, then there is a problem as the '\' between 'server' and 'c:' is eaten.

It also appears that these are valid UNC forms


\\?\UNC\Server01\user\docs\Letter.txt
\\?\C:\user\docs\Letter.txt



Rik <rik5>
Group administrator
Mon 06 Apr 2015 11:46:04 PM UTC, comment #11: 

What should the behavior of the following be:


fullfile ('/', '/abc', 'def')
ans = //abc/def               # new
ans = /abc/def                # old


THe usual UNIX behavior is that excess slashes are collapsed to one.  The old function did this, the new one does not.

Rik <rik5>
Group administrator
Mon 06 Apr 2015 04:19:54 PM UTC, comment #10: 

@Rik:
See dicussion below.

Matlab r2012a's fullfile() accepts & returns '\\server\path' (literally) so I surmise that is a valid outcome.
That would be in favor of Matlab compatibility.

@Mike:
On the page you mention it is stated that ML accepts "full path names including  ".... UNC path '\\' string"  which includes a server name - please correct me if I'm wrong but a UNC path is a network path.
It may be (I just do not know) that on *nix such forms are not legal - is that so? Not even with Samba accessing Windows shares?

I cannot test neither at work, as there are no network "shares" (at least not accessible in the form '\\svr\path' by me as ordinary user), nor at home as Matlab r2015a prerelease has timed out (just a few days ago :-( ).

BTW it just occurred to me that fullfile.m doesn't check for the 260 char length limit on UNC paths. Maybe something for another bug report & -fix.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 06 Apr 2015 03:11:43 PM UTC, comment #9: 

I can't test, but according to:

https://www.mathworks.com/help/matlab/matlab_env/specify-file-names.html

Matlab treats paths beginning with a double-backslash as absolute directories, just as if it were a path beginning with a drive letter.

Mike Miller <mtmiller>
Group Member
Mon 06 Apr 2015 03:07:51 PM UTC, comment #8: 

Is this a Matlab compatibility issue, or is this an Octave extension beyond Matlab to support network shares?

Rik <rik5>
Group administrator
Mon 06 Apr 2015 08:56:56 AM UTC, comment #7: 

Here's a patch. I'll cc Rik for review

(BTW, title adapted as this bug shows on any platform)

(file #33562)

Philip Nienhuis <philipnienhuis>
Group Member
Sun 05 Apr 2015 09:17:53 PM UTC, comment #6: 

I have a fix; need to test on Linux.

It's not going to look pretty; that is unavoidable because of the way fullfile.m has been written:
Using strjoin(), introducing intermediate consecutive fileseps when input partial file paths have leading or trailing fileseps. Those intermediate consecutive fileseps have to be distinguished from leading fileseps for network paths, but '//' isn't acceptable; etc.

Philip Nienhuis <philipnienhuis>
Group Member
Wed 01 Apr 2015 05:32:44 PM UTC, comment #5: 

I'd swear I've seen or maybe even fixed a similar bug a while ago.....
Yes: http://hg.savannah.gnu.org/hgweb/octave/rev/c304687571c8

OK I'll have a look later on, if nobody beats me to it.

This one is different as it also fails on *nix; it looks nastier as there's no reliable way to distinguish an erroneous path ('\\wrong\path\name') from a valid network path ('\\server\d\rive\path'). fullfile.m simply strips multiple leading fileseps away.
Maybe just assuming that two consecutive fileseps always constitute a network path would do the trick? (with a check on Windows that there's no <driveletter><colon> sequence> ?)

In addition fullfile.m doesn't know about e.g.,

file:///rootdir/path
ftp://server/rootdir/path
www://server/rootdir/path
http://server/rootdir/path

...and other path forms. But that's a separate issue.

OS -> Any; Status -> confirmed

Philip Nienhuis <philipnienhuis>
Group Member
Wed 01 Apr 2015 12:21:06 PM UTC, comment #4: 

fullfile('//server', 'file.txt') returns string back with the / changed to a backslash, and with only one of them before the server name.

John Donoghue <lostbard>
Group Member
Wed 01 Apr 2015 06:20:01 AM UTC, comment #3: 

You could also try if it works with forward slashes.
From the Octave prompt, forward slashes generally work fine as file separator on Windows systems.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 31 Mar 2015 09:48:50 PM UTC, comment #2: 

Oughh, now backspash is not escape character anymore..

I hope that you can understand what was an issue.

Anonymous
Tue 31 Mar 2015 09:46:48 PM UTC, comment #1: 

Backslashes have been removed since they were parsed as escape characters.

Sample code should look like:

fullfile('\\\\nas.lan\\share1', 'folder', file.ext')

% Should be:
\\\\nas.lan\\share1\\file.ext

% instead of:
\\nas.lan\\share1\\file.ext


Anonymous
Tue 31 Mar 2015 09:40:43 PM UTC, original submission:  

Function fullfile should allow double backslash to apear at the begining of the path to support network drive paths on Windows platform.

e.g.

fullfile('\nas.lanshare1', 'folder', file.ext')

shoud return:


\nas.lanshare1file.ext

not

nas.lanshare1file.ext


Simplest way to achive that functionality would be by changing fullfile.m:62 from:

filename(strfind (filename, [filesep filesep])) = "";

to

filename(ispc() + strfind (filename(1 + ispc():end), [filesep filesep])) = "";


Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #34092:  fullfile_44682.cs added by philipnienhuis (2KiB - text/x-csharp)
file #33562:  fullfile_bug44682.cs added by philipnienhuis (2KiB - text/x-csharp)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by avinoam (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 philipnienhuis
  • -email is unavailable- added by lostbard (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 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-05-31 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2015-05-25 philipnienhuis Attached File- Added fullfile_44682.cs, #34092
    2015-05-25 philipnienhuis StatusIn Progress Patch Submitted
        Release4.0.0-rc2 dev
    2015-05-25 philipnienhuis StatusPatch Submitted In Progress
    2015-04-06 philipnienhuis Attached File- Added fullfile_bug44682.cs, #33562
        StatusIn Progress Patch Submitted
        SummaryFullfile does not support network share addresses on windows platofrm Fullfile does not support network share addresses
        Carbon-Copy- Added rik5
    2015-04-05 philipnienhuis StatusConfirmed In Progress
        Assigned toNone philipnienhuis
    2015-04-01 philipnienhuis StatusNone Confirmed
        Operating SystemMicrosoft Windows Any

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code