bugGNU Octave - Bugs: bug #48351, make_absolute_filename should...

 
 

bug #48351: make_absolute_filename should canonicalize absolute filename inputs

Submitter:  <jens>
Submitted:  Thu 30 Jun 2016 01:20:24 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 4.0.1 Operating System:  * Microsoft Windows
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 04 Jul 2016 02:55:48 PM UTC, comment #7: 

@Mike: I like it.  I took the liberty of using your code and also modifying run.m to put the input validation first.  In addition, we no longer need the call to make_absolute_filename at all.  The 'cd' command can handle relative paths just fine.  That also fixes the case you mentioned of


run //tmp/foo.m


See http://hg.savannah.gnu.org/hgweb/octave/rev/98ee8b1ebbeb.

I also optimized do_make_absolute for the common occurrence (every single time a prompt is generated) where it is called with '.' as an input.  See http://hg.savannah.gnu.org/hgweb/octave/rev/8df31c24dce3.

Closing report.

Rik <rik5>
Group administrator
Sat 02 Jul 2016 10:40:41 PM UTC, comment #6: 

Thanks! So the filesep differences are definitely not safe, my mistake. Confirmed that there is still a problem with something like


run //tmp/foo.m


There is also a problem with running a script in a symlinked directory. Octave reports the destination of the symlink when pwd() is run, so the name passed in to the function does not match the pwd value after cd'ing.

How about a change like this instead of relying on directory canonicalization:


diff --git a/scripts/miscellaneous/run.m b/scripts/miscellaneous/run.m
--- a/scripts/miscellaneous/run.m
+++ b/scripts/miscellaneous/run.m
@@ -60,14 +60,16 @@ function run (script)

   if (! isempty (d))
     if (exist (d, "dir"))
+      scriptdir = "";
       startdir = pwd ();
       d = make_absolute_filename (d);
       unwind_protect
         cd (d);
+        scriptdir = pwd ();
         evalin ("caller", sprintf ("source ('%s%s');", f, ext),
                 "rethrow (lasterror ())");
       unwind_protect_cleanup
-        if (strcmp (d, pwd ()))
+        if (strcmp (scriptdir, pwd ()))
           cd (startdir);
         endif
       end_unwind_protect


Mike Miller <mtmiller>
Group Member
Sat 02 Jul 2016 10:08:03 PM UTC, comment #5: 

I pushed a change to make_absolute_filename here (http://hg.savannah.gnu.org/hgweb/octave/rev/6e618b8277ad).

This fixes the issue with run.m, however, it still doesn't fix an input which is already an absolute filename such as '///'.  In that case, do_make_absolute uses do_absolute_pathname() to determine that it is already absolute so it doesn't process it.  We could remove that check, but that might carry a performance penalty.  While using gdb, I found that do_make_absolute is called every time Octave returns to the command line.

Rik <rik5>
Group administrator
Sat 02 Jul 2016 09:53:54 PM UTC, comment #4: 

@Mike: See the fix for bug #45231 where you suggested, and the fix implemented, to swap canonicalize_file_name for make_absolute_filename. :)

I've got a fix for make_absolute_filename which I am about to commit as soon as 'make check' finishes.

Rik <rik5>
Group administrator
Sat 02 Jul 2016 09:22:39 PM UTC, comment #3: 

Why not just change run.m to use canonicalize_file_name? The difference between the two is that make_absolute_filename() works on paths as strings, the files and directories don't have to actually exist. And canonicalize_file_name requires files and directories to exist, follows symlinks, and converts filesep characters. But in run.m we already know the directory exists, so it should be safe.

Mike Miller <mtmiller>
Group Member
Sat 02 Jul 2016 07:18:47 PM UTC, comment #2: 

The root problem is that make_absolute_filename should strip trailing directory separators.  canonicalize_file_name() does that correctly as shown below for the root directory.


debug> make_absolute_filename ("///")
ans = ///
debug> canonicalize_file_name ("///")
ans = /


Linking to bug #45231 which is related.

Rik <rik5>
Group administrator
Thu 30 Jun 2016 01:24:09 PM UTC, comment #1: 

Sorry, wrong usage of Markup in original submission, again:
if run is used with path and file e.g.

run('../toBeRunned.m');

it never returns to the original working directory due to a bug in run.m line
70:

        if (strcmp (d, pwd ()))
          cd (startdir);
        endif
-verbatim-
The path in 'd' contains a tailing file separator '\' while pwd() does not.
Therefore the condition is false, cd(startdir) is not executed.

<jens>
Thu 30 Jun 2016 01:20:24 PM UTC, original submission:  

if run is used with path and file e.g.

run('../toBeRunned.m');

it never returns to the original working directory due to a bug in run.m line 70:

        if (strcmp (d, pwd ()))
          cd (startdir);
        endif
-verbatim
The path in 'd' contains a tailing file separator '\' while pwd() does not.
Therefore the condition is false, cd(startdir) is not executed.

<jens>

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Digest:
   bug dependencies.

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-04 rik5 StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2016-07-02 rik5 Summarymake_absolute_filename should strip trailing file separator characters make_absolute_filename should canonicalize absolute filename inputs
    2016-07-02 rik5 StatusNone Confirmed
        Summaryrun does not return to original working directory make_absolute_filename should strip trailing file separator characters
        Dependencies- Depends on bugs #45231

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code