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

t/harness can mistakenly run tests outside of the perl source tree #14578

Closed
p5pRT opened this issue Mar 12, 2015 · 9 comments
Closed

t/harness can mistakenly run tests outside of the perl source tree #14578

p5pRT opened this issue Mar 12, 2015 · 9 comments
Labels

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Mar 12, 2015

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

Searchable as RT124050$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 12, 2015

From @steve-m-hay

If I have a cpan/Text-Balanced folder (containing a Git workspace of the CPAN Text-Balanced distro) alongside my perl source tree then t/harness will mistakenly run the tests in ../../cpan/Text-Balanced/t/ (outside the perl source tree) instead of the intended ../cpan/Text-Balanced/t/ (inside the perl source tree).

It happens because of this little snippet in t/harness​:

# Allow eg ./perl t/harness t/op/lc.t
for (@​tests) {
  if (-f "../$_") {
  $_ = "../$_";
  s{^\.\./t/}{};
  }
}

but I'm not sure off-hand how best to fix it to be absolutely sure that it doesn't end up outside of the perl source tree.

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 14, 2015

From @jkeenan

On Thu Mar 12 02​:24​:36 2015, shay wrote​:

If I have a cpan/Text-Balanced folder (containing a Git workspace of
the CPAN Text-Balanced distro) alongside my perl source tree then
t/harness will mistakenly run the tests in ../../cpan/Text-Balanced/t/
(outside the perl source tree) instead of the intended ../cpan/Text-
Balanced/t/ (inside the perl source tree).

It happens because of this little snippet in t/harness​:

# Allow eg ./perl t/harness t/op/lc.t
for (@​tests) {
if (-f "../$_") {
$_ = "../$_";
s{^\.\./t/}{};
}
}

but I'm not sure off-hand how best to fix it to be absolutely sure
that it doesn't end up outside of the perl source tree.

Unless someone has a brainstorm about this, I would suggest simply documenting it in pod/perlhack.pod as a known limitation of t/harness.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 14, 2015

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Mar 16, 2015

From @bulk88

On Sat Mar 14 16​:14​:00 2015, jkeenan wrote​:

Unless someone has a brainstorm about this, I would suggest simply
documenting it in pod/perlhack.pod as a known limitation of t/harness.

I've either reported (and someone else fixed) or fixed a dozen of these bugs in the last 3 years. This is a common class of bugs with the perl distro.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 24, 2016

From @wolfsage

On Thu, Mar 12, 2015 at 5​:24 AM, Steve Hay <perlbug-followup@​perl.org> wrote​:

If I have a cpan/Text-Balanced folder (containing a Git workspace of the CPAN Text-Balanced distro) alongside my perl source tree then t/harness will mistakenly run the tests in ../../cpan/Text-Balanced/t/ (outside the perl source tree) instead of the intended ../cpan/Text-Balanced/t/ (inside the perl source tree).

It happens because of this little snippet in t/harness​:

# Allow eg ./perl t/harness t/op/lc.t
for (@​tests) {
if (-f "../$_") {
$_ = "../$_";
s{^\.\./t/}{};
}
}

but I'm not sure off-hand how best to fix it to be absolutely sure that it doesn't end up outside of the perl source tree.

Possible patch attached. This may be 'better than nothing' (provided I
didn't break something else).

-- Matthew Horsfall (alh)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 24, 2016

From @wolfsage

0001-RT-124050-Try-really-hard-not-to-run-tests-outside-o.patch
From 8999852eaf7db1cb193127c786127a0bb2a32a1e Mon Sep 17 00:00:00 2001
From: Matthew Horsfall <wolfsage@gmail.com>
Date: Sun, 24 Apr 2016 13:43:00 -0400
Subject: [PATCH] RT: #124050 - Try really hard not to run tests outside of
 source tree.

Assume if the user passed in a path with '../' that they were in t/
aiming for a test a level up somewhere.
---
 t/harness | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/harness b/t/harness
index d069472..f0cbcd6 100644
--- a/t/harness
+++ b/t/harness
@@ -188,7 +188,7 @@ if ($^O eq 'MSWin32') {
 
 # Allow eg ./perl t/harness t/op/lc.t
 for (@tests) {
-    if (-f "../$_") {
+    if ($_ !~ /^\.\./ && -f "../$_") {
         $_ = "../$_";
         s{^\.\./t/}{};
     }
-- 
1.9.1

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 20, 2016

From @iabyn

On Sun, Apr 24, 2016 at 01​:46​:59PM -0400, Matthew Horsfall (alh) wrote​:

On Thu, Mar 12, 2015 at 5​:24 AM, Steve Hay <perlbug-followup@​perl.org> wrote​:

If I have a cpan/Text-Balanced folder (containing a Git workspace of the CPAN Text-Balanced distro) alongside my perl source tree then t/harness will mistakenly run the tests in ../../cpan/Text-Balanced/t/ (outside the perl source tree) instead of the intended ../cpan/Text-Balanced/t/ (inside the perl source tree).

It happens because of this little snippet in t/harness​:

# Allow eg ./perl t/harness t/op/lc.t
for (@​tests) {
if (-f "../$_") {
$_ = "../$_";
s{^\.\./t/}{};
}
}

but I'm not sure off-hand how best to fix it to be absolutely sure that it doesn't end up outside of the perl source tree.

Possible patch attached. This may be 'better than nothing' (provided I
didn't break something else).

-- Matthew Horsfall (alh)

From 8999852eaf7db1cb193127c786127a0bb2a32a1e Mon Sep 17 00​:00​:00 2001
From​: Matthew Horsfall <wolfsage@​gmail.com>
Date​: Sun, 24 Apr 2016 13​:43​:00 -0400
Subject​: [PATCH] RT​: #124050 - Try really hard not to run tests outside of
source tree.

Assume if the user passed in a path with '../' that they were in t/
aiming for a test a level up somewhere.
---
t/harness | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/harness b/t/harness
index d069472..f0cbcd6 100644
--- a/t/harness
+++ b/t/harness
@​@​ -188,7 +188,7 @​@​ if ($^O eq 'MSWin32') {

# Allow eg ./perl t/harness t/op/lc.t
for (@​tests) {
- if (-f "../$_") {
+ if ($_ !~ /^\.\./ && -f "../$_") {
$_ = "../$_";
s{^\.\./t/}{};
}

I've applied a slightly enhanced version of that​:

commit 683433b
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Mon Jun 20 11​:48​:35 2016 +0100
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Mon Jun 20 11​:48​:35 2016 +0100

  t/harness​: avoid tests outside the src tree
 
  [perl #124050] t/harness can mistakenly run tests outside of the perl
  source tree
 
  cfa5625 made t/harness prepend '../' to test filenames if such a
  file existed. This allowed things like
 
  ./perl t/harness cpan/foo/t/foo.t
 
  to work even after harness had done a chdir("t"). However, it was then
  picking up a ../cpan/foo/t/foo.t file outside the src tree in preference
  to the one inside.
 
  Add belt-and-brace conditions​: only modify the filename if the unmodified
  file doesn't exist, and only only if it doesn't already start with ../.
 
  Based on an earlier patch by Matthew Horsfall

Affected files ...
  M t/harness

Differences ...

Inline Patch
diff --git a/t/harness b/t/harness
index d069472..b46582d 100644
--- a/t/harness
+++ b/t/harness
@@ -188,7 +188,7 @@ if ($^O eq 'MSWin32') {
 
 # Allow eg ./perl t/harness t/op/lc.t
 for (@tests) {
-    if (-f "../$_") {
+    if (! -f $_ && !/^\.\./ && -f "../$_") {
         $_ = "../$_";
         s{^\.\./t/}{};
     }





-- 

Indomitable in retreat, invincible in advance, insufferable in victory
  -- Churchill on Montgomery

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 20, 2016

From @wolfsage

On Mon, Jun 20, 2016 at 7​:00 AM, Dave Mitchell <davem@​iabyn.com> wrote​:

I've applied a slightly enhanced version of that​:

Ah yes, much better. Thanks.

-- Matthew Horsfall (alh)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 20, 2016

@iabyn - Status changed from 'open' to 'resolved'

@p5pRT p5pRT closed this Jun 20, 2016
@p5pRT p5pRT added the Severity Low label Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.