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
stat() behaves differently when passed an array in argument list #14925
Comments
From jozef.mojzis@gmail.comThe following: use strict; my $file = $0; print "perl: $]\n"; prints different result on 5.23.3 and 5.22. It seems that stat() doesn't like array passed as arguments. Reference question on SO: http://stackoverflow.com/q/32760273/632407 |
From @dcollinsnThis is a side effect of 7f399b4, which was an attempt to fix some crashing bugs, including #126064 and #123846. According to perlfunc, stat can be called with either a filehandle, a dirhandle, or a filename. If not, it will use Whether stat(@_) is behavior that we should support or not is left as an exercise to the reader, however neither the docs nor the test suite appears to support this. I'm not even sure what I would expect to happen if I wrote my @files = ('/path/to/file1', '/path/to/file2'); stat(@files). Suggest either WONTFIX or add a warning and a note to perldelta. |
From [Unknown Contact. See original ticket]This is a side effect of 7f399b4, which was an attempt to fix some crashing bugs, including #126064 and #123846. According to perlfunc, stat can be called with either a filehandle, a dirhandle, or a filename. If not, it will use Whether stat(@_) is behavior that we should support or not is left as an exercise to the reader, however neither the docs nor the test suite appears to support this. I'm not even sure what I would expect to happen if I wrote my @files = ('/path/to/file1', '/path/to/file2'); stat(@files). Suggest either WONTFIX or add a warning and a note to perldelta. |
From @cpansproutOn Thu Sep 24 08:48:08 2015, dcollinsn@gmail.com wrote:
I think it would corrupt the stack in 5.22, but I haven’t bothered checking.
I think perhaps we should apply list context, because sub mystat{stat(@_)} isn’t a completely irrational thing to do (especially not if you add ($) to the mix). But this will require some more careful consideration. Specifically, we need to consider exactly which cases behaved reasonably before and might be broken either by application of list or scalar context to the arguments, instead of the ‘random context’ that we had before (which was mostly based on what context the sub containing the stat call was called in). -- Father Chrysostomos |
The RT System itself - Status changed from 'new' to 'open' |
From jozef.mojzis@gmail.com
If the final decistion will be change the behaviour (to the documented (and correct) one) Before asked a question I was read thru all 5.23.* deltas and found nothing. jm. |
From Eirik-Berg.Hanssen@allverden.noOn Thu, Sep 24, 2015 at 6:49 PM, Father Chrysostomos via RT <
prototype(\&CORE::stat) is (;*), and any user defined function with that In particular, if you use autodie qw/stat/, even stat will (in that I think providing scalar context is a bugfix. If you want stat to I'm pretty sure the perldelta needs to mention these changes. A warning Eirik |
From @dcollinsnOn Thu Sep 24 12:48:45 2015, Eirik-Berg.Hanssen@allverden.no wrote:
Something like the attached warning+perldelta+test should provide education in the necessary cases while ensuring that we know if the behavior of stat(@_) regresses in any way. Reasonably certain that this is *not* the right way to write warnings, as they seem to print even without -w. |
From @dcollinsnstat_array.patchdiff --git a/op.c b/op.c
index 745cb5f..3ad4909 100644
--- a/op.c
+++ b/op.c
@@ -9710,6 +9710,10 @@ Perl_ck_ftst(pTHX_ OP *o)
op_free(o);
return newop;
}
+
+ if (kidtype == OP_RV2AV) {
+ Perl_warner(aTHX_ packWARN(WARN_SYNTAX), "Array passed to stat will be coerced to a scalar (did you want stat $_[0]?)");
+ }
scalar((OP *) kid);
if ((PL_hints & HINT_FILETEST_ACCESS) && OP_IS_FILETEST_ACCESS(o->op_type))
o->op_private |= OPpFT_ACCESS;
diff --git a/pod/perldelta.pod b/pod/perldelta.pod
index db9e601..a14db85 100644
--- a/pod/perldelta.pod
+++ b/pod/perldelta.pod
@@ -45,6 +45,15 @@ XXX For a release on a stable branch, this section aspires to be:
[ List each incompatible change as a =head2 entry ]
+=head2 Stat can no longer be called with an array argument
+
+The core function stat() expects to be called with either a scalar argument,
+or no argument, in which case $_ is used. In prior versions, the argument was
+not coerced to a scalar. This was fixed in order to prevent a related bug
+which would corrupt the stack. stat(@_) used to behave like stat($_[0]), but
+now behaves like stat(scalar(@_)). This was never documented, and will now
+emit a warning.
+
=head1 Deprecations
XXX Any deprecated features, syntax, modules etc. should be listed here.
diff --git a/t/op/stat.t b/t/op/stat.t
index 2d7e3c7..44032b5 100644
--- a/t/op/stat.t
+++ b/t/op/stat.t
@@ -25,7 +25,7 @@ if ($^O eq 'MSWin32') {
${^WIN32_SLOPPY_STAT} = 0;
}
-plan tests => 116;
+plan tests => 118;
my $Perl = which_perl();
@@ -636,7 +636,16 @@ SKIP: {
is join("-", 1,2,3,(stat stat stat),4,5,6), "1-2-3-4-5-6",
'stat inside stat gets scalar context';
+# [perl #126162] stat an array should not work
+my $statfile = './op/stat.t';
+my @statarg = ($statfile, $statfile);
+ok !stat(@statarg),
+ 'stat on an array of valid paths should warn and should not return any data';
+is $!, 'No such file or directory',
+ 'stat on an array of valid paths should return "No such file or directory"';
+
END {
chmod 0666, $tmpfile;
unlink_all $tmpfile;
}
|
From [Unknown Contact. See original ticket]On Thu Sep 24 12:48:45 2015, Eirik-Berg.Hanssen@allverden.no wrote:
Something like the attached warning+perldelta+test should provide education in the necessary cases while ensuring that we know if the behavior of stat(@_) regresses in any way. Reasonably certain that this is *not* the right way to write warnings, as they seem to print even without -w. |
From @ap* Dan Collins via RT <perlbug-comment@perl.org> [2015-09-25 00:05]:
Aside from the message being too specific:
(What if the code was `stat @foo`?) |
From @tonycozOn Fri Sep 25 10:13:11 2015, aristotle wrote:
It also only warns on package variables. The error message check is fragile - not all systems use the same text for each errno value. The patch wouldn't apply for me - both patch and git apply complained it was corrupt. I applied it manually, and added a few changes, per attached. Smoke testing as smoke-me/tonyc/126162-stat-array Tony |
From @tonycoz0001-perl-126162-warn-if-stat-is-called-on-an-array.patchFrom 390408f1a2bc2154741d73d97f16d78be56b7925 Mon Sep 17 00:00:00 2001
From: Dan Collins <dcollinsn@gmail.com>
Date: Wed, 30 Mar 2016 16:42:39 +1100
Subject: [PATCH 1/4] (perl #126162) warn if stat() is called on an array
---
op.c | 4 ++++
t/op/stat.t | 10 +++++++++-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/op.c b/op.c
index e58f711..9bbe4f3 100644
--- a/op.c
+++ b/op.c
@@ -9733,6 +9733,10 @@ Perl_ck_ftst(pTHX_ OP *o)
op_free(o);
return newop;
}
+
+ if (kidtype == OP_RV2AV) {
+ Perl_warner(aTHX_ packWARN(WARN_SYNTAX), "Array passed to stat will be coerced to a scalar (did you want stat $_[0]?)");
+ }
scalar((OP *) kid);
if ((PL_hints & HINT_FILETEST_ACCESS) && OP_IS_FILETEST_ACCESS(o->op_type))
o->op_private |= OPpFT_ACCESS;
diff --git a/t/op/stat.t b/t/op/stat.t
index 2d7e3c7..c388083 100644
--- a/t/op/stat.t
+++ b/t/op/stat.t
@@ -25,7 +25,7 @@ if ($^O eq 'MSWin32') {
${^WIN32_SLOPPY_STAT} = 0;
}
-plan tests => 116;
+plan tests => 118;
my $Perl = which_perl();
@@ -636,6 +636,14 @@ SKIP: {
is join("-", 1,2,3,(stat stat stat),4,5,6), "1-2-3-4-5-6",
'stat inside stat gets scalar context';
+# [perl #126162] stat an array should not work
+my $statfile = './op/stat.t';
+my @statarg = ($statfile, $statfile);
+ok !stat(@statarg),
+ 'stat on an array of valid paths should warn and should not return any data';
+is $!, 'No such file or directory',
+ 'stat on an array of valid paths should return "No such file or directory"';
+
END {
chmod 0666, $tmpfile;
unlink_all $tmpfile;
--
2.1.4
|
From @tonycoz0002-add-the-new-stat-array-diagnostic-to-perldiag.patchFrom 0c8e907b840112f6d4a132a03bdb97a96ae1c299 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Wed, 30 Mar 2016 16:48:40 +1100
Subject: [PATCH 2/4] add the new stat(@array) diagnostic to perldiag
---
pod/perldiag.pod | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/pod/perldiag.pod b/pod/perldiag.pod
index 777226f..941a108 100644
--- a/pod/perldiag.pod
+++ b/pod/perldiag.pod
@@ -216,6 +216,11 @@ operator which expects either a number or a string matching
C</^[a-zA-Z]*[0-9]*\z/>. See L<perlop/Auto-increment and
Auto-decrement> for details.
+=item Array passed to stat will be coerced to a scalar (did you want stat $_[0]?)
+
+(W syntax) You called stat() on an array, but the array will be
+coerced to a scalar - the number of elements in the array.
+
=item assertion botched: %s
(X) The malloc package that comes with Perl had an internal failure.
--
2.1.4
|
From @tonycoz0003-error-messages-vary-between-platforms-use-errno-valu.patchFrom 4e51ab6beed6ed1158b76e74ff12d4e3da663fb3 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Thu, 31 Mar 2016 10:40:25 +1100
Subject: [PATCH 3/4] error messages vary between platforms, use errno values
instead
---
t/op/stat.t | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/t/op/stat.t b/t/op/stat.t
index c388083..637a902 100644
--- a/t/op/stat.t
+++ b/t/op/stat.t
@@ -637,12 +637,19 @@ is join("-", 1,2,3,(stat stat stat),4,5,6), "1-2-3-4-5-6",
'stat inside stat gets scalar context';
# [perl #126162] stat an array should not work
+my $Errno_loaded = eval { require Errno };
my $statfile = './op/stat.t';
my @statarg = ($statfile, $statfile);
ok !stat(@statarg),
'stat on an array of valid paths should warn and should not return any data';
-is $!, 'No such file or directory',
- 'stat on an array of valid paths should return "No such file or directory"';
+my $error = 0+$!;
+SKIP:
+{
+ skip "Errno not available", 1
+ unless $Errno_loaded;
+ is $error, &Errno::ENOENT,
+ 'stat on an array of valid paths should return ENOENT';
+}
END {
chmod 0666, $tmpfile;
--
2.1.4
|
From @tonycoz0004-perl-126162-improve-stat-array-handling.patchFrom 26ad17bd29556736e5e4aa43e9c70ba125acdded Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Thu, 31 Mar 2016 11:18:53 +1100
Subject: [PATCH 4/4] (perl #126162) improve stat @array handling
- warn on lexical arrays too
- limit the warning to under C<use warnings 'syntax';>
- test the warnings
- include the (correct) variable name where possible
---
op.c | 27 ++++++++++++++++++++++-----
pod/perldiag.pod | 2 +-
t/lib/warnings/op | 17 +++++++++++++++++
3 files changed, 40 insertions(+), 6 deletions(-)
diff --git a/op.c b/op.c
index 9bbe4f3..fcb1bc6 100644
--- a/op.c
+++ b/op.c
@@ -109,6 +109,8 @@ recursive, but it's recursive on basic blocks, not on tree nodes.
#define CALL_RPEEP(o) PL_rpeepp(aTHX_ o)
#define CALL_OPFREEHOOK(o) if (PL_opfreehook) PL_opfreehook(aTHX_ o)
+static char array_passed_to_stat[] = "Array passed to stat will be coerced to a scalar";
+
/* Used to avoid recursion through the op tree in scalarvoid() and
op_free()
*/
@@ -1548,7 +1550,7 @@ S_scalarboolean(pTHX_ OP *o)
}
static SV *
-S_op_varname(pTHX_ const OP *o)
+S_op_varname_subscript(pTHX_ const OP *o, int subscript_type)
{
assert(o);
assert(o->op_type == OP_PADAV || o->op_type == OP_RV2AV ||
@@ -1561,13 +1563,19 @@ S_op_varname(pTHX_ const OP *o)
if (cUNOPo->op_first->op_type != OP_GV
|| !(gv = cGVOPx_gv(cUNOPo->op_first)))
return NULL;
- return varname(gv, funny, 0, NULL, 0, 1);
+ return varname(gv, funny, 0, NULL, 0, subscript_type);
}
return
- varname(MUTABLE_GV(PL_compcv), funny, o->op_targ, NULL, 0, 1);
+ varname(MUTABLE_GV(PL_compcv), funny, o->op_targ, NULL, 0, subscript_type);
}
}
+static SV *
+S_op_varname(pTHX_ const OP *o)
+{
+ return S_op_varname_subscript(aTHX_ o, 1);
+}
+
static void
S_op_pretty(pTHX_ const OP *o, SV **retsv, const char **retpv)
{ /* or not so pretty :-) */
@@ -9734,8 +9742,17 @@ Perl_ck_ftst(pTHX_ OP *o)
return newop;
}
- if (kidtype == OP_RV2AV) {
- Perl_warner(aTHX_ packWARN(WARN_SYNTAX), "Array passed to stat will be coerced to a scalar (did you want stat $_[0]?)");
+ if ((kidtype == OP_RV2AV || kidtype == OP_PADAV) && ckWARN(WARN_SYNTAX)) {
+ SV *name = S_op_varname_subscript(aTHX_ (OP*)kid, 2);
+ if (name) {
+ /* diag_listed_as: Array passed to stat will be coerced to a scalar%s */
+ Perl_warner(aTHX_ packWARN(WARN_SYNTAX), "%s (did you want stat %" SVf "?)",
+ array_passed_to_stat, name);
+ }
+ else {
+ /* diag_listed_as: Array passed to stat will be coerced to a scalar%s */
+ Perl_warner(aTHX_ packWARN(WARN_SYNTAX), array_passed_to_stat);
+ }
}
scalar((OP *) kid);
if ((PL_hints & HINT_FILETEST_ACCESS) && OP_IS_FILETEST_ACCESS(o->op_type))
diff --git a/pod/perldiag.pod b/pod/perldiag.pod
index 941a108..78aeb16 100644
--- a/pod/perldiag.pod
+++ b/pod/perldiag.pod
@@ -216,7 +216,7 @@ operator which expects either a number or a string matching
C</^[a-zA-Z]*[0-9]*\z/>. See L<perlop/Auto-increment and
Auto-decrement> for details.
-=item Array passed to stat will be coerced to a scalar (did you want stat $_[0]?)
+=item Array passed to stat will be coerced to a scalar%s
(W syntax) You called stat() on an array, but the array will be
coerced to a scalar - the number of elements in the array.
diff --git a/t/lib/warnings/op b/t/lib/warnings/op
index 8256c23..528639e 100644
--- a/t/lib/warnings/op
+++ b/t/lib/warnings/op
@@ -2040,3 +2040,20 @@ EXPECT
Non-finite repeat count does nothing at - line 5.
Non-finite repeat count does nothing at - line 6.
Non-finite repeat count does nothing at - line 7.
+########
+# NAME warn on stat @array
+@foo = ("op/stat.t");
+stat @foo;
+my @bar = @foo;
+stat @bar;
+my $ref = \@foo;
+stat @$ref;
+use warnings 'syntax';
+stat @foo;
+stat @bar;
+stat @$ref;
+EXPECT
+Array passed to stat will be coerced to a scalar (did you want stat $foo[0]?) at - line 8.
+Array passed to stat will be coerced to a scalar (did you want stat $bar[0]?) at - line 9.
+Array passed to stat will be coerced to a scalar at - line 10.
+
--
2.1.4
|
From @rjbsOn Wed Mar 30 17:24:32 2016, tonyc wrote:
I see a little black smoke, but I think it's all unrelated..? Thoughts on applying this now so we have it in blead as early as possible? -- |
From @tonycozOn Fri Apr 01 05:38:31 2016, rjbs wrote:
The smoke results look good. There were some new errors in darwin[1] and cygwin[2] but they were related to a previous change (fixed in 67083b7) and a change in cygwin (48d9c42 and dfe3adb). I don't have a strong opinion either way as to whether it should go in. Tony |
From @rjbsI've applied these patches, thanks! -- |
@rjbs - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank you for submitting this report. You have helped make Perl better. Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0 |
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#126162 (status was 'resolved')
Searchable as RT126162$
The text was updated successfully, but these errors were encountered: