Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File::Spec::VMS->canonpath() incorrect with ODS-5 style directory specs #15499

Closed
p5pRT opened this issue Aug 6, 2016 · 15 comments
Closed

File::Spec::VMS->canonpath() incorrect with ODS-5 style directory specs #15499

p5pRT opened this issue Aug 6, 2016 · 15 comments

Comments

@p5pRT
Copy link

p5pRT commented Aug 6, 2016

Migrated from rt.perl.org#128865 (status was 'resolved')

Searchable as RT128865$

@p5pRT
Copy link
Author

p5pRT commented Aug 6, 2016

From levitte@openssl.org

Hi,

I've found a bug in File​::Spec​::VMS->canonpath(), when a dash
component in a directory spec is preceded by a name containing an
escaped period.

My examples show quite clearly where things go wrong​:

  $ perl -e "use File​::Spec​::Functions; print canonpath('foo​:[bar.coo.kie.--]')"
  foo​:[bar]

  check!

  $ perl -e "use File​::Spec​::Functions; print canonpath('foo​:[bar^.coo.kie.--]')"
  foo​:[bar^.coo.-]

  WRONG!

  $ perl -e "use File​::Spec​::Functions; print canonpath('foo​:[bar.coo^.kie.--]')"
  foo​:[bar.coo^.kie.--]

  WRONG!

  $ perl -e "use File​::Spec​::Functions; print canonpath('foo​:[bar.coo.kie.-]')"
  foo​:[bar.coo]

  check!

  $ perl -e "use File​::Spec​::Functions; print canonpath('foo​:[bar^.coo.kie.-]')"
  foo​:[bar^.coo]

  check!

  $ perl -e "use File​::Spec​::Functions; print canonpath('foo​:[bar.coo^.kie.-]')"
  foo​:[bar.coo^.kie.-]

  WRONG!

I've created a patch (attached) that solves the issue, with these
correct results (same paths)​:

  $ perl "-I." -e "require 'VMS.pm'; print File​::Spec​::VMS->canonpath('foo​:[bar.coo.kie.--]')"
  foo​:[bar]
  $ perl "-I." -e "require 'VMS.pm'; print File​::Spec​::VMS->canonpath('foo​:[bar^.coo.kie.--]')"
  foo​:[000000]
  $ perl "-I." -e "require 'VMS.pm'; print File​::Spec​::VMS->canonpath('foo​:[bar.coo^.kie.--]')"
  foo​:[000000]
  $ perl "-I." -e "require 'VMS.pm'; print File​::Spec​::VMS->canonpath('foo​:[bar.coo.kie.-]')"
  foo​:[bar.coo]
  $ perl "-I." -e "require 'VMS.pm'; print File​::Spec​::VMS->canonpath('foo​:[bar^.coo.kie.-]')"
  foo​:[bar^.coo]
  $ perl "-I." -e "require 'VMS.pm'; print File​::Spec​::VMS->canonpath('foo​:[bar.coo^.kie.-]')"
  foo​:[bar]

The tested perl is 5.20.1, but comparing its VMS.pm to the one in the
5.24.0 tarball (perl-5.24.0/dist/PathTools/lib/File/Spec/VMS.pm) shows
that canonpath() hasn't changed at all between those versions.

Cheers,
Richard

--
Richard Levitte levitte@​openssl.org
OpenSSL Project http​://www.openssl.org/~levitte/

@p5pRT
Copy link
Author

p5pRT commented Aug 6, 2016

From levitte@openssl.org

VMS.pm.patch
--- VMS.pm.1	2016-08-06 22:26:22.987294406 +0200
+++ VMS.pm.2	2016-08-06 23:15:22.226631337 +0200
@@ -94,7 +94,7 @@
 						# [-.-.		==> [--.
 						# .-.-]		==> .--]
 						# [-.-]		==> [--]
-    1 while ($path =~ s/(?<!\^)([\[\.])[^\]\.]+\.-(-+)([\]\.])/$1$2$3/);
+    1 while ($path =~ s/(?<!\^)([\[\.])(?:\^.|[^\]\.])+\.-(-+)([\]\.])/$1$2$3/);
 						# That loop does the following
 						# with any amount (minimum 2)
 						# of dashes:
@@ -105,11 +105,11 @@
 						#
 						# And then, the remaining cases
     $path =~ s/(?<!\^)\[\.-/[-/;		# [.-		==> [-
-    $path =~ s/(?<!\^)\.[^\]\.]+\.-\./\./g;	# .foo.-.	==> .
-    $path =~ s/(?<!\^)\[[^\]\.]+\.-\./\[/g;	# [foo.-.	==> [
-    $path =~ s/(?<!\^)\.[^\]\.]+\.-\]/\]/g;	# .foo.-]	==> ]
+    $path =~ s/(?<!\^)\.(?:\^.|[^\]\.])+\.-\./\./g;	# .foo.-.	==> .
+    $path =~ s/(?<!\^)\[(?:\^.|[^\]\.])+\.-\./\[/g;	# [foo.-.	==> [
+    $path =~ s/(?<!\^)\.(?:\^.|[^\]\.])+\.-\]/\]/g;	# .foo.-]	==> ]
 						# [foo.-]       ==> [000000]
-    $path =~ s/(?<!\^)\[[^\]\.]+\.-\]/\[000000\]/g;
+    $path =~ s/(?<!\^)\[(?:\^.|[^\]\.])+\.-\]/\[000000\]/g;
 						# []		==>
     $path =~ s/(?<!\^)\[\]// unless $path eq '[]';
     return $unix_rpt ? unixify($path) : $path;

@p5pRT
Copy link
Author

p5pRT commented Aug 6, 2016

From @cpansprout

On Sat Aug 06 14​:37​:04 2016, levitte@​openssl.org wrote​:

I've created a patch (attached) that solves the issue, with these
correct results (same paths)​:

Any chance you could also patch t/Spec.t and add your failing examples?

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Aug 6, 2016

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Aug 8, 2016

From levitte@openssl.org

In message <rt-4.0.24-13765-1470525270-1011.128865-94-0@​perl.org> on Sat, 06 Aug 2016 16​:14​:30 -0700, "Father Chrysostomos via RT" <perlbug-followup@​perl.org> said​:

perlbug-followup> On Sat Aug 06 14​:37​:04 2016, levitte@​openssl.org wrote​:
perlbug-followup> > I've created a patch (attached) that solves the issue, with these
perlbug-followup> > correct results (same paths)​:
perlbug-followup>
perlbug-followup> Any chance you could also patch t/Spec.t and add your failing examples?

Certainly! Attached. I hope I got it right.

--
Richard Levitte levitte@​openssl.org
OpenSSL Project http​://www.openssl.org/~levitte/

@p5pRT
Copy link
Author

p5pRT commented Aug 8, 2016

From levitte@openssl.org

Spec.t.patch
--- Spec.t	2016-03-01 13:33:02.000000000 +0100
+++ Spec.t.new	2016-08-07 12:08:19.682779725 +0200
@@ -448,6 +448,13 @@
 # During the Perl 5.8 era, FS::Unix stopped eliminating redundant path elements, so mimic that here.
 [ "VMS->canonpath('a/../../b/c.dat')",                  $vms_unix_rpt ? 'a/../../b/c.dat'              : '[-.b]c.dat'                      ],
 [ "VMS->canonpath('^<test^.new.-.caret^ escapes^>')",   $vms_unix_rpt ? '/<test.new.-.caret escapes>' : '^<test^.new.-.caret^ escapes^>'                                                   ],
+# Check that directory specs with caret-dot component is treated correctly
+[ "VMS->canonpath('foo:[bar.coo.kie.--]file.txt')",     $vms_unix_rpt ? '/foo/bar/file.txt'            : "foo:[bar]file.txt" ],
+[ "VMS->canonpath('foo:[bar^.coo.kie.--]file.txt')",    $vms_unix_rpt ? '/foo/file.txt'                : "foo:[000000]file.txt" ],
+[ "VMS->canonpath('foo:[bar.coo^.kie.--]file.txt')",    $vms_unix_rpt ? '/foo/file.txt'                : "foo:[000000]file.txt" ],
+[ "VMS->canonpath('foo:[bar.coo.kie.-]file.txt')",      $vms_unix_rpt ? '/foo/bar/coo/file.txt'        : "foo:[bar.coo]file.txt" ],
+[ "VMS->canonpath('foo:[bar^.coo.kie.-]file.txt')",     $vms_unix_rpt ? '/foo/bar.coo/file.txt'        : "foo:[bar^.coo]file.txt" ],
+[ "VMS->canonpath('foo:[bar.coo^.kie.-]file.txt')",     $vms_unix_rpt ? '/foo/bar/file.txt'            : "foo:[bar]file.txt" ],
 
 [ "VMS->splitdir('')",            ''          ],
 [ "VMS->splitdir('[]')",          ''          ],

@p5pRT
Copy link
Author

p5pRT commented Aug 8, 2016

From @craigberry

On Sun, Aug 7, 2016 at 5​:22 AM, Richard Levitte <levitte@​openssl.org> wrote​:

In message <rt-4.0.24-13765-1470525270-1011.128865-94-0@​perl.org> on Sat, 06 Aug 2016 16​:14​:30 -0700, "Father Chrysostomos via RT" <perlbug-followup@​perl.org> said​:

perlbug-followup> On Sat Aug 06 14​:37​:04 2016, levitte@​openssl.org wrote​:
perlbug-followup> > I've created a patch (attached) that solves the issue, with these
perlbug-followup> > correct results (same paths)​:
perlbug-followup>
perlbug-followup> Any chance you could also patch t/Spec.t and add your failing examples?

Certainly! Attached. I hope I got it right.

This looks right to me, thanks. It will probably be a couple days
before I can test and apply it.

@p5pRT
Copy link
Author

p5pRT commented Aug 9, 2016

From @craigberry

On Mon, Aug 8, 2016 at 11​:10 AM, Craig A. Berry <craig.a.berry@​gmail.com> wrote​:

On Sun, Aug 7, 2016 at 5​:22 AM, Richard Levitte <levitte@​openssl.org> wrote​:

In message <rt-4.0.24-13765-1470525270-1011.128865-94-0@​perl.org> on Sat, 06 Aug 2016 16​:14​:30 -0700, "Father Chrysostomos via RT" <perlbug-followup@​perl.org> said​:

perlbug-followup> On Sat Aug 06 14​:37​:04 2016, levitte@​openssl.org wrote​:
perlbug-followup> > I've created a patch (attached) that solves the issue, with these
perlbug-followup> > correct results (same paths)​:
perlbug-followup>
perlbug-followup> Any chance you could also patch t/Spec.t and add your failing examples?

Certainly! Attached. I hope I got it right.

This looks right to me, thanks. It will probably be a couple days
before I can test and apply it.

The patch with tests is in at​:

<http​://perl5.git.perl.org/perl.git/commitdiff/e11f7a82751877fddd83adf7de4c5c15d0df3d33>

The ticket can be closed, but I will just note that other platforms
have long since given up removing redundant path components in
canonpath. For example, on macOS​:

$ perl -MFile​::Spec​::Functions -e 'print
canonpath("/foo/bar/coo.kie/../../file.txt") . "\n";'
/foo/bar/coo.kie/../../file.txt

The '../../' is still there after canonization. Perhaps we should
consider following suit in VMS.pm, but at least we're now closer to
doing it right (which we should do if we're going to do it at all).

@p5pRT
Copy link
Author

p5pRT commented Aug 9, 2016

@cpansprout - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Author

p5pRT commented Aug 9, 2016

From levitte@openssl.org

Craig Berry via RT <perlbug-followup@​perl.org> skrev​: (9 augusti 2016 22​:34​:03 CEST)

On Mon, Aug 8, 2016 at 11​:10 AM, Craig A. Berry
<craig.a.berry@​gmail.com> wrote​:

On Sun, Aug 7, 2016 at 5​:22 AM, Richard Levitte <levitte@​openssl.org>
wrote​:

In message <rt-4.0.24-13765-1470525270-1011.128865-94-0@​perl.org> on
Sat, 06 Aug 2016 16​:14​:30 -0700, "Father Chrysostomos via RT"
<perlbug-followup@​perl.org> said​:

perlbug-followup> On Sat Aug 06 14​:37​:04 2016, levitte@​openssl.org
wrote​:
perlbug-followup> > I've created a patch (attached) that solves the
issue, with these
perlbug-followup> > correct results (same paths)​:
perlbug-followup>
perlbug-followup> Any chance you could also patch t/Spec.t and add
your failing examples?

Certainly! Attached. I hope I got it right.

This looks right to me, thanks. It will probably be a couple days
before I can test and apply it.

The patch with tests is in at​:

<http​://perl5.git.perl.org/perl.git/commitdiff/e11f7a82751877fddd83adf7de4c5c15d0df3d33>

The ticket can be closed, but I will just note that other platforms
have long since given up removing redundant path components in
canonpath. For example, on macOS​:

$ perl -MFile​::Spec​::Functions -e 'print
canonpath("/foo/bar/coo.kie/../../file.txt") . "\n";'
/foo/bar/coo.kie/../../file.txt

The '../../' is still there after canonization.

Frankly, I'm not sure I understand why it's done that way. Maybe something with samplings and what "going up" would mean, semantically?

Perhaps we should consider following suit in VMS.pm,

I think that's a real bad idea. Just my opinion, but I'd ask the VMS community before making such a drastic change...

but at least we're now closer to
doing it right (which we should do if we're going to do it at all).

Great.

Cheers
Richard

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

@p5pRT
Copy link
Author

p5pRT commented Aug 10, 2016

From @craigberry

On Tue, Aug 9, 2016 at 4​:14 PM, Richard Levitte <levitte@​openssl.org> wrote​:

Craig Berry via RT <perlbug-followup@​perl.org> skrev​: (9 augusti 2016 22​:34​:03 CEST)

The ticket can be closed, but I will just note that other platforms
have long since given up removing redundant path components in
canonpath. For example, on macOS​:

$ perl -MFile​::Spec​::Functions -e 'print
canonpath("/foo/bar/coo.kie/../../file.txt") . "\n";'
/foo/bar/coo.kie/../../file.txt

The '../../' is still there after canonization.

Frankly, I'm not sure I understand why it's done that way. Maybe something with samplings and what "going up" would mean, semantically?

I don't know either, but apparently it was done in the 5.8 era based
on a comment I made in Spec.t after doing some archive diving.

Perhaps we should consider following suit in VMS.pm,

I think that's a real bad idea. Just my opinion, but I'd ask the VMS community before making such a drastic change...

I don't really have a dog in this fight and am short on tuits to do
anything at all near-term, but I'll just note that whenever the
behavior is different on VMS it usually causes trouble in some way.
If we're producing paths that are correct (which thanks to you we are
now doing better), though, then I agree nothing further should be done
without very careful consideration and consultation.

@p5pRT
Copy link
Author

p5pRT commented Aug 10, 2016

From @tonycoz

On Tue Aug 09 17​:07​:05 2016, craig.a.berry@​gmail.com wrote​:

On Tue, Aug 9, 2016 at 4​:14 PM, Richard Levitte <levitte@​openssl.org>
wrote​:

Craig Berry via RT <perlbug-followup@​perl.org> skrev​: (9 augusti 2016
22​:34​:03 CEST)

The ticket can be closed, but I will just note that other platforms
have long since given up removing redundant path components in
canonpath. For example, on macOS​:

$ perl -MFile​::Spec​::Functions -e 'print
canonpath("/foo/bar/coo.kie/../../file.txt") . "\n";'
/foo/bar/coo.kie/../../file.txt

The '../../' is still there after canonization.

Frankly, I'm not sure I understand why it's done that way. Maybe
something with samplings and what "going up" would mean,
semantically?

I don't know either, but apparently it was done in the 5.8 era based
on a comment I made in Spec.t after doing some archive diving.

If coo.kie is a symbolic link to /bar/foo/quux the original path refers to

/bar/file.txt

If you do a naïve resolution of the ../ you get​:

/foo/file.txt

(remembering canonpath doesn't check the filesystem.)

This is noted in the File​::Spec documentation for canonpath() - I
don't know if that's an issue on VMS.

Tony

@p5pRT
Copy link
Author

p5pRT commented Aug 12, 2016

From levitte@openssl.org

Tony Cook via RT <perlbug-followup@​perl.org> skrev​: (10 augusti 2016 02​:26​:33 CEST)

On Tue Aug 09 17​:07​:05 2016, craig.a.berry@​gmail.com wrote​:

On Tue, Aug 9, 2016 at 4​:14 PM, Richard Levitte <levitte@​openssl.org>
wrote​:

Craig Berry via RT <perlbug-followup@​perl.org> skrev​: (9 augusti
2016
22​:34​:03 CEST)

The ticket can be closed, but I will just note that other
platforms
have long since given up removing redundant path components in
canonpath. For example, on macOS​:

$ perl -MFile​::Spec​::Functions -e 'print
canonpath("/foo/bar/coo.kie/../../file.txt") . "\n";'
/foo/bar/coo.kie/../../file.txt

The '../../' is still there after canonization.

Frankly, I'm not sure I understand why it's done that way. Maybe
something with samplings and what "going up" would mean,
semantically?

I don't know either, but apparently it was done in the 5.8 era based
on a comment I made in Spec.t after doing some archive diving.

If coo.kie is a symbolic link to /bar/foo/quux the original path refers
to

/bar/file.txt

If you do a naïve resolution of the ../ you get​:

/foo/file.txt

(remembering canonpath doesn't check the filesystem.)

This is noted in the File​::Spec documentation for canonpath() - I
don't know if that's an issue on VMS.

I suggest closing this ticket if it hasn't already been done.

As for symbolic links, issue understood on the Unix side. As for the VMS side, symbolic links do exist but are a very recent addition. I'm thinking of looking into that in the coming weeks.

Cheers
Richard

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

@p5pRT
Copy link
Author

p5pRT commented May 30, 2017

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release today of Perl 5.26.0, this and 210 other issues have been
resolved.

Perl 5.26.0 may be downloaded via​:
https://metacpan.org/release/XSAWYERX/perl-5.26.0

If you find that the problem persists, feel free to reopen this ticket.

@p5pRT
Copy link
Author

p5pRT commented May 30, 2017

@khwilliamson - Status changed from 'pending release' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant