bugGNU Octave - Bugs: bug #58547, gzip()/bzip2() do not handle...

 
 

bug #58547: gzip()/bzip2() do not handle relative paths in first argument correctly

Submitter:  None
Submitted:  Thu 11 Jun 2020 10:08:25 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Harold Gutch Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * 6.0.90
Operating System:  * Any Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 16 Jun 2020 07:46:12 PM UTC, comment #9: 

I'm sure that I copied things like absolute_pathname from bash.  In bash version 5, the definition is as follows, but it has been similar for many years.


/* Return 1 if STRING contains an absolute pathname, else 0.  Used by `cd'
   to decide whether or not to look up a directory name in $CDPATH. */
int
absolute_pathname (string)
     const char *string;
{
  if (string == 0 || *string == '\0')
    return (0);

  if (ABSPATH(string))
    return (1);

  if (string[0] == '.' && PATHSEP(string[1]))   /* . and ./ */
    return (1);

  if (string[0] == '.' && string[1] == '.' && PATHSEP(string[2]))       /* .. and ../ */
    return (1);

  return (0);
}


I probably extracted the check for ./ into a separate function because I thought that would be useful by itself (I could have been wrong).

In the past, I thought it was useful to have the load-path handle "foo/bar" as a relative path name.  Now I think it makes better sense (and is much simpler) to always use absolute names even if the user specifies a relative name.  Re-scanning relative directories when the current directory changes seems like trouble (even for ".", but we are stuck with that one).

So maybe the whole idea of a rooted_relative filename is not needed in Octave?

John W. Eaton <jwe>
Group administrator
Tue 16 Jun 2020 07:08:07 PM UTC, comment #8: 

I can be more precise, "foo/bar" is neither rooted relative nor absolute.  Octave is currently checking for rooted relative paths for which the fragment returns false.

I suppose the question is why Octave has this test at all:


  if (! (do_absolute_pathname (s) || do_rooted_relative_pathname (s)))
    return s;


If you comment this out, does do_base_pathname now do what you expect?

If you comment this out, does 'make check' pass?

I tried using 'hg blame' to figure out when this bit of code was added, but it didn't particularly help.  It's been there since at least 2012, but possibly goes even further back.

Rik <rik5>
Group administrator
Tue 16 Jun 2020 03:44:02 PM UTC, comment #7: 

I would argue that "foo/bar" is a relative path, just like "./foo/bar". Why is it not a relative path? And what kind of path it is then?

> This is not an absolute path, and let's assume there is no subdirectory of the current directory named "Mickey_Mouse".  In that case, both tests return false--this is not a pathname--and the string is returned unaltered.


There being no such subdirectory is irrelevant. The behaviour of such functions should not be dependent on what directories exist at the moment on the current working directory. We may be manipulating paths with the aim of creating them or we may be handling paths relative to somewhere else. This should return the string unaltered because there is no file separator, therefore its value is the basename part of the filepath. This is the behaviour in Linux basename command, on perl, on php, on python, and C++17 std::filesystem::path so it's very surprising that Octave does something different.

Carnë Draug <carandraug>
Group Member
Mon 15 Jun 2020 03:34:11 PM UTC, comment #6: 

"full pathname" is usually a synonym for absolute pathname.

"foo/bar" is neither relative nor absolute.

"/home/rik/foo/bar" is an example of an absolute pathname
"./foo/bar" is an example of a relative pathname

I think it is just the comment for the code that needs to be adjusted.

The code is


// Return the 'basename' of the pathname in STRING (the stuff after
// the last directory separator).  If STRING is not a full pathname,
// simply return it.

std::string
env::do_base_pathname (const std::string& s) const
{
  if (! (do_absolute_pathname (s) || do_rooted_relative_pathname (s)))
    return s;

  size_t pos = s.find_last_of (sys::file_ops::dir_sep_chars ());

  if (pos == std::string::npos)
    return s;
  else
    return s.substr (pos+1);
}


It looks to me that the first two checks, do_absolute_pathname and do_rooted_relative_pathname) are meant to check whether the string 's' even corresponds to a pathname.  Imagine calling


do_base_pathname ("Mickey_Mouse")


This is not an absolute path, and let's assume there is no subdirectory of the current directory named "Mickey_Mouse".  In that case, both tests return false--this is not a pathname--and the string is returned unaltered.


Rik <rik5>
Group administrator
Mon 15 Jun 2020 10:02:46 AM UTC, comment #5: 

Does anyone understand why  sys::env::base_pathname even calls do_rooted_relative_pathname ? It's documentation states:


    // [...] If STRING is not a full pathname,
    // simply return it.


But I don't know what is a full pathname. Why is 'foo/bar' not a full pathname and './foo/bar' is a full pathname? I would guess that the issue is there since this behaviour is not common in basename implementations. How many other functions are calling base_pathname and expect 'foo/bar' to return 'foo/bar' instead of 'bar'?

Carnë Draug <carandraug>
Group Member
Mon 15 Jun 2020 03:13:10 AM UTC, comment #4: 

Harold Gutch, I confirmed that your patch indeed fixes this bug, thank you very much for the problem analysis.

Thank you for pushing Rik.  The OP says in comment #0 that this issue seems to occur when using `pkg build`, thus a widely used feature by all package maintainers.  Therefore I grafted it back to stable to have it fixed as early as possible.

https://hg.savannah.gnu.org/hgweb/octave/rev/5a85e42a1be7

Kai Torben Ohlhus <siko1056>
Group Member
Sun 14 Jun 2020 03:14:52 AM UTC, comment #3: 

I reviewed and pushed the patch to the development branch here: https://hg.savannah.gnu.org/hgweb/octave/rev/4d7232bcf6c1.

Marking sa Ready for Test.


Rik <rik5>
Group administrator
Fri 12 Jun 2020 07:06:59 AM UTC, comment #2: 

Thanks for your comments, I had not considered the case where the first argument to gzip() is a directory.  I just tested it and my patched version (again only verified with 5.2.0) behaves like MATLAB in all four cases.

Anonymous
Fri 12 Jun 2020 06:14:59 AM UTC, comment #1: 

Confirmed.  Octave 5 is no longer under development => change release to Octave 6 (bug also present in Octave 7).

All tests performed on openSUSE 15.1 manually compiled Octave versions.


% Setup
mkdir ('foo/bar');
A = rand (4);
save foo/A.mat     A
save foo/bar/B.mat A



% Matlab R2019a
gzip ('foo')      % [1] creates foo/A.mat.gz
                  %         and foo/bar/B.mat.gz
gzip ('foo/bar')  % [2] creates foo/bar/B.mat.gz

gzip ('foo',     'outdir') % [3] creates outdir/A.mat.gz
                           %         and outdir/B.mat.gz
gzip ('foo/bar', 'outdir') % [4] creates outdir/B.mat.gz


In Octave 5.2.0, 6, and 7 (compiled today):

- calls [1] and [2] work
- calls [3] and [4] not (an empty directory "outdir" is created for each call)

Had no time to try your patch yet.

Kai Torben Ohlhus <siko1056>
Group Member
Thu 11 Jun 2020 10:08:25 PM UTC, original submission:  

(For the sake of completeness, this all this is on FreeBSD 11.3-STABLE on amd64 with Octave compiled manually, i.e., not from the FreeBSD ports collection)

When calling gzip as

gzip("foo/bar", "outdir")

the code will try to store the output of the gzip operation in the file outdir/foo/bar.gz .  An example where this happens is if you call

pkg build builddir image-1.0.0.tar.gz

at the end of the build, after tar'ing up the build.  If the first parameter instead is the full path ( "/path/to/foo/bar" ) or a relative path starting with . or .. it tries to store the output in outdir/bar.gz , as expected.

The reason for this is that libinterp/dldfcn/gzip.cc around line 556 calls sys::env::base_pathname() which (in l. 369 of liboctave/system/oct-env.cc ) first calls do_rooted_relative_pathname() -nomarkup+ .  That tests if the parameter is one of . , .. , or if it starts with ./ or ../ but it does not test for other types of relative paths (like foo/bar ).  It appears to me this is intentional and should indeed not return True for a "normal" relative pathname, and thus gzip() should not go down this path.

This patch here seems to fix the issue for me by determining the basename manually in gzip.cc instead of calling base_pathname():


--- octave-5.2.0/libinterp/dldfcn/gzip.cc.orig  2020-01-28 02:57:35.000000000 +0100
+++ octave-5.2.0/libinterp/dldfcn/gzip.cc       2020-06-11 23:14:28.226471000 +0200
@@ -553,7 +553,9 @@
     const std::function<std::string(const std::string&)> mk_dest_path
       = [&out_dir, &ext] (const std::string& source_path) -> std::string
       {
-        const std::string basename = sys::env::base_pathname (source_path);
+        size_t pos = source_path.find_last_of (sys::file_ops::dir_sep_str ());
+        const std::string basename = (pos == std::string::npos ? source_path :
+                                      source_path.substr (pos+1));
         return sys::file_ops::concat (out_dir, basename + ext);
       };


(this is mildly inspired by ~l.8598 of libinterp/parse-tree/oct-parse.cc ).

Anonymous

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2020-06-15 siko1056 StatusReady For Test Fixed
        Open/ClosedOpen Closed
        Releasedev 6.0.90
    2020-06-14 rik5 Item GroupUnexpected Error or Warning Matlab Compatibility
        StatusPatch Submitted Ready For Test
        Release6.0.90 dev
        Operating SystemBSD Any
        Summarygzip() does not handle relative paths in first argument correctly gzip()/bzip2() do not handle relative paths in first argument correctly
    2020-06-12 siko1056 StatusNone Patch Submitted
        Release5.2.0 6.0.90

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code