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

 
 

bug #48351: make_absolute_filename should canonicalize absolute filename inputs

Submitted by:  <jens>
Submitted on:  Thu 30 Jun 2016 01:20:24 PM UTC  
 
Category: Octave FunctionSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: 4.0.1Operating System: Microsoft Windows

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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

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>
Project 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

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:

Mike Miller <mtmiller>
Project Administrator
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>
Project 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>
Project 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>
Project Administrator
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.

Linking to bug #45231 which is related.

Rik <rik5>
Project 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.
it never returns to the original working directory due to a bug in run.m line 70:

<jens>

 

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

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Digest:
   bug dependencies.

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by jens (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 04 Jul 2016 02:55:48 PM UTCrik5StatusConfirmed=>Fixed
      Open/ClosedOpen=>Closed
    Sat 02 Jul 2016 10:08:03 PM UTCrik5Summarymake_absolute_filename should strip trailing file separator characters=>make_absolute_filename should canonicalize absolute filename inputs
    Sat 02 Jul 2016 07:18:47 PM UTCrik5StatusNone=>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.1-cleanup1