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

"exec PROGRAM LIST" does not work with empty list #16075

Closed
p5pRT opened this issue Jul 10, 2017 · 10 comments
Closed

"exec PROGRAM LIST" does not work with empty list #16075

p5pRT opened this issue Jul 10, 2017 · 10 comments

Comments

@p5pRT
Copy link

p5pRT commented Jul 10, 2017

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

Searchable as RT131730$

@p5pRT
Copy link
Author

p5pRT commented Jul 10, 2017

From @pali

When exec is called with empty list, either via explicit () or implicit,
from empty array variable, then it always fail and set $! to empty
string. See examples​:

$ perl -e 'exec {"binary"} () or die "exec failed​: \"$!\"\n";'
exec failed​: ""

$ perl -e 'my @​args; exec {"binary"} @​args or die "exec failed​: \"$!\"\n";'
exec failed​: ""

Here is patch which fixes it​:

Inline Patch
diff --git a/doio.c b/doio.c
index 6f4cd84f8c..32f6e3416f 100644
--- a/doio.c
+++ b/doio.c
@@ -1583,7 +1583,7 @@ Perl_do_aexec5(pTHX_ SV *really, SV **mark, SV **sp,
 #if defined(__SYMBIAN32__) || defined(__LIBCATAMOUNT__)
     Perl_croak(aTHX_ "exec? I'm not *that* kind of operating system");
 #else
-    if (sp > mark) {
+    if (sp >= mark) {
 	const char **a;
 	const char *tmps = NULL;
 	Newx(PL_Argv, sp - mark + 1, const char*);

With above patch it is working fine:

perl -e 'exec {"true"} () or die "exec failed​: \"$!\"\n";'; echo $?
0

perl -e 'exec {"false"} () or die "exec failed​: \"$!\"\n";'; echo $?
1

@p5pRT
Copy link
Author

p5pRT commented Jul 10, 2017

From @ilmari

(via RT) <perlbug-followup@​perl.org> writes​:

When exec is called with empty list, either via explicit () or implicit,
from empty array variable, then it always fail and set $! to empty
string. See examples​:

$ perl -e 'exec {"binary"} () or die "exec failed​: \"$!\"\n";'
exec failed​: ""

$ perl -e 'my @​args; exec {"binary"} @​args or die "exec failed​: \"$!\"\n";'
exec failed​: ""

Here is patch which fixes it​:

diff --git a/doio.c b/doio.c
index 6f4cd84f8c..32f6e3416f 100644
--- a/doio.c
+++ b/doio.c
@​@​ -1583,7 +1583,7 @​@​ Perl_do_aexec5(pTHX_ SV *really, SV **mark, SV **sp,
#if defined(__SYMBIAN32__) || defined(__LIBCATAMOUNT__)
Perl_croak(aTHX_ "exec? I'm not *that* kind of operating system");
#else
- if (sp > mark) {
+ if (sp >= mark) {
const char **a;
const char *tmps = NULL;
Newx(PL_Argv, sp - mark + 1, const char*);

With above patch it is working fine​:

perl -e 'exec {"true"} () or die "exec failed​: \"$!\"\n";'; echo $?
0

perl -e 'exec {"false"} () or die "exec failed​: \"$!\"\n";'; echo $?
1

This causes calling execvp() with an empty argv array, which is not
strictly POSIX-conforming (it "should point to a filename"), and might
break programs that expect to be able to access argv[0] without checking
argc, but I agree perl should still allow it.

However, with this patch "exec {''} ()" and "exec()" segfault a bit
further down, trying to dereference PL_Argv[0], which is the terminating
NULL pointer.

exec() crashes on line 1601 while exec {''} () crashes on line 1608​:

  if (really)
  tmps = SvPV_nolen_const(really);
1601​: if ((!really && *PL_Argv[0] != '/') ||
  [...]
  if (really && *tmps) {
  PerlProc_execvp(tmps,EXEC_ARGV_CAST(PL_Argv));
  } else {
1608​: PerlProc_execvp(PL_Argv[0],EXEC_ARGV_CAST(PL_Argv));
  }

It gets to that path insted of the "really" branch, because tmps is an
empty string so, *tmps is the terminating NUL byte (this behaviour of
ignoring the return value of the block if it's empty is not documented,
but has been there since perl 3.0).

I propose explicitly returning ENOENT (which is what execvp() returns if
the file name is an empty string) in both of the above failing cases,
i.e. something like this​:

Inline Patch
diff --git a/doio.c b/doio.c
index 6f4cd84f8c..cbaf7e6a33 100644
--- a/doio.c
+++ b/doio.c
@@ -1583,7 +1583,7 @@ Perl_do_aexec5(pTHX_ SV *really, SV **mark, SV **sp,
 #if defined(__SYMBIAN32__) || defined(__LIBCATAMOUNT__)
     Perl_croak(aTHX_ "exec? I'm not *that* kind of operating system");
 #else
-    if (sp > mark) {
+    if (sp >= mark) {
 	const char **a;
 	const char *tmps = NULL;
 	Newx(PL_Argv, sp - mark + 1, const char*);
@@ -1598,17 +1598,19 @@ Perl_do_aexec5(pTHX_ SV *really, SV **mark, SV **sp,
 	*a = NULL;
 	if (really)
 	    tmps = SvPV_nolen_const(really);
-	if ((!really && *PL_Argv[0] != '/') ||
+	if ((!really && PL_Argv[0] && *PL_Argv[0] != '/') ||
 	    (really && *tmps != '/'))		/* will execvp use PATH? */
 	    TAINT_ENV();		/* testing IFS here is overkill, probably */
 	PERL_FPU_PRE_EXEC
 	if (really && *tmps) {
             PerlProc_execvp(tmps,EXEC_ARGV_CAST(PL_Argv));
-	} else {
+	} else if (PL_Argv[0]) {
             PerlProc_execvp(PL_Argv[0],EXEC_ARGV_CAST(PL_Argv));
-	}
+	} else {
+            SETERRNO(ENOENT,LIB_INVARG);
+	}
 	PERL_FPU_POST_EXEC
-	S_exec_failed(aTHX_ (really ? tmps : PL_Argv[0]), fd, do_report);
+	S_exec_failed(aTHX_ (really ? tmps : PL_Argv[0] ? PL_Argv[0] : ""), fd, do_report);
     }
     do_execfree();
 #endif

- ilmari
-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.               - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.                      - Calle Dybedahl

@p5pRT
Copy link
Author

p5pRT commented Jul 10, 2017

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

@p5pRT
Copy link
Author

p5pRT commented Jul 13, 2017

From @pali

On Monday 10 July 2017 06​:58​:10 Dagfinn Ilmari Mannsåker via RT wrote​:

(via RT) <perlbug-followup@​perl.org> writes​:

When exec is called with empty list, either via explicit () or implicit,
from empty array variable, then it always fail and set $! to empty
string. See examples​:

$ perl -e 'exec {"binary"} () or die "exec failed​: \"$!\"\n";'
exec failed​: ""

$ perl -e 'my @​args; exec {"binary"} @​args or die "exec failed​: \"$!\"\n";'
exec failed​: ""

Here is patch which fixes it​:

diff --git a/doio.c b/doio.c
index 6f4cd84f8c..32f6e3416f 100644
--- a/doio.c
+++ b/doio.c
@​@​ -1583,7 +1583,7 @​@​ Perl_do_aexec5(pTHX_ SV *really, SV **mark, SV **sp,
#if defined(__SYMBIAN32__) || defined(__LIBCATAMOUNT__)
Perl_croak(aTHX_ "exec? I'm not *that* kind of operating system");
#else
- if (sp > mark) {
+ if (sp >= mark) {
const char **a;
const char *tmps = NULL;
Newx(PL_Argv, sp - mark + 1, const char*);

With above patch it is working fine​:

perl -e 'exec {"true"} () or die "exec failed​: \"$!\"\n";'; echo $?
0

perl -e 'exec {"false"} () or die "exec failed​: \"$!\"\n";'; echo $?
1

This causes calling execvp() with an empty argv array, which is not
strictly POSIX-conforming (it "should point to a filename"),

I cannot currently find POSIX requirement that first arg cannot be NULL.

and might
break programs that expect to be able to access argv[0] without checking
argc, but I agree perl should still allow it.

But, in ISO C89, ISO C99 and also ISO C11, main's argc can be zero. In
all those ISO C standards is written​:

* The value of argc shall be nonnegative.
* argv[argc] shall be a null pointer.

However, with this patch "exec {''} ()" and "exec()" segfault a bit
further down, trying to dereference PL_Argv[0], which is the terminating
NULL pointer.

Ouh, I have not caught this.

exec() crashes on line 1601 while exec {''} () crashes on line 1608​:

    if \(really\)
         tmps = SvPV\_nolen\_const\(really\);

1601​: if ((!really && *PL_Argv[0] != '/') ||
[...]
if (really && *tmps) {
PerlProc_execvp(tmps,EXEC_ARGV_CAST(PL_Argv));
} else {
1608​: PerlProc_execvp(PL_Argv[0],EXEC_ARGV_CAST(PL_Argv));
}

It gets to that path insted of the "really" branch, because tmps is an
empty string so, *tmps is the terminating NUL byte (this behaviour of
ignoring the return value of the block if it's empty is not documented,
but has been there since perl 3.0).

I propose explicitly returning ENOENT (which is what execvp() returns if
the file name is an empty string) in both of the above failing cases,
i.e. something like this​:

Looks good, thanks for update!

diff --git a/doio.c b/doio.c
index 6f4cd84f8c..cbaf7e6a33 100644
--- a/doio.c
+++ b/doio.c
@​@​ -1583,7 +1583,7 @​@​ Perl_do_aexec5(pTHX_ SV *really, SV **mark, SV **sp,
#if defined(__SYMBIAN32__) || defined(__LIBCATAMOUNT__)
Perl_croak(aTHX_ "exec? I'm not *that* kind of operating system");
#else
- if (sp > mark) {
+ if (sp >= mark) {
const char **a;
const char *tmps = NULL;
Newx(PL_Argv, sp - mark + 1, const char*);
@​@​ -1598,17 +1598,19 @​@​ Perl_do_aexec5(pTHX_ SV *really, SV **mark, SV **sp,
*a = NULL;
if (really)
tmps = SvPV_nolen_const(really);
- if ((!really && *PL_Argv[0] != '/') ||
+ if ((!really && PL_Argv[0] && *PL_Argv[0] != '/') ||
(really && *tmps != '/')) /* will execvp use PATH? */
TAINT_ENV(); /* testing IFS here is overkill, probably */
PERL_FPU_PRE_EXEC
if (really && *tmps) {
PerlProc_execvp(tmps,EXEC_ARGV_CAST(PL_Argv));
- } else {
+ } else if (PL_Argv[0]) {
PerlProc_execvp(PL_Argv[0],EXEC_ARGV_CAST(PL_Argv));
- }
+ } else {
+ SETERRNO(ENOENT,LIB_INVARG);
+ }
PERL_FPU_POST_EXEC
- S_exec_failed(aTHX_ (really ? tmps : PL_Argv[0]), fd, do_report);
+ S_exec_failed(aTHX_ (really ? tmps : PL_Argv[0] ? PL_Argv[0] : ""), fd, do_report);
}
do_execfree();
#endif

- ilmari
--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl

@p5pRT
Copy link
Author

p5pRT commented Jul 13, 2017

From @ilmari

pali@​cpan.org writes​:

On Monday 10 July 2017 06​:58​:10 Dagfinn Ilmari Mannsåker via RT wrote​:

This causes calling execvp() with an empty argv array, which is not
strictly POSIX-conforming (it "should point to a filename"),

I cannot currently find POSIX requirement that first arg cannot be NULL.

http​://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html says​:

| The value in argv[0] should point to a filename string that is
| associated with the process being started by one of the exec
| functions.

It's only a "should", not a "must", so the requirement only applies to
Strictly Conforming POSIX Applications, which we can't require perl
scripts to be.

and might
break programs that expect to be able to access argv[0] without checking
argc, but I agree perl should still allow it.

But, in ISO C89, ISO C99 and also ISO C11, main's argc can be zero. In
all those ISO C standards is written​:

* The value of argc shall be nonnegative.
* argv[argc] shall be a null pointer.

The RATIONALE section of the POSIX exec() man page explains the history​:

| Early proposals required that the value of argc passed to main() be "one
| or greater". This was driven by the same requirement in drafts of the
| ISO C standard. In fact, historical implementations have passed a value
| of zero when no arguments are supplied to the caller of the exec
| functions. This requirement was removed from the ISO C standard and
| subsequently removed from this volume of POSIX.1-2008 as well. The
| wording, in particular the use of the word should, requires a Strictly
| Conforming POSIX Application to pass at least one argument to the exec
| function, thus guaranteeing that argc be one or greater when invoked by
| such an application. In fact, this is good practice, since many existing
| applications reference argv[0] without first checking the value of argc.

[…]

I propose explicitly returning ENOENT (which is what execvp() returns if
the file name is an empty string) in both of the above failing cases,
i.e. something like this​:

Looks good, thanks for update!

I'm going on holiday for a week today, but unless anyone objects (or
beats me to it) I'll write some tests and push this when I get back.

diff --git a/doio.c b/doio.c
index 6f4cd84f8c..cbaf7e6a33 100644
--- a/doio.c
+++ b/doio.c
@​@​ -1583,7 +1583,7 @​@​ Perl_do_aexec5(pTHX_ SV *really, SV **mark, SV **sp,
#if defined(__SYMBIAN32__) || defined(__LIBCATAMOUNT__)
Perl_croak(aTHX_ "exec? I'm not *that* kind of operating system");
#else
- if (sp > mark) {
+ if (sp >= mark) {
const char **a;
const char *tmps = NULL;
Newx(PL_Argv, sp - mark + 1, const char*);
@​@​ -1598,17 +1598,19 @​@​ Perl_do_aexec5(pTHX_ SV *really, SV **mark, SV **sp,
*a = NULL;
if (really)
tmps = SvPV_nolen_const(really);
- if ((!really && *PL_Argv[0] != '/') ||
+ if ((!really && PL_Argv[0] && *PL_Argv[0] != '/') ||
(really && *tmps != '/')) /* will execvp use PATH? */
TAINT_ENV(); /* testing IFS here is overkill, probably */
PERL_FPU_PRE_EXEC
if (really && *tmps) {
PerlProc_execvp(tmps,EXEC_ARGV_CAST(PL_Argv));
- } else {
+ } else if (PL_Argv[0]) {
PerlProc_execvp(PL_Argv[0],EXEC_ARGV_CAST(PL_Argv));
- }
+ } else {
+ SETERRNO(ENOENT,LIB_INVARG);
+ }
PERL_FPU_POST_EXEC
- S_exec_failed(aTHX_ (really ? tmps : PL_Argv[0]), fd, do_report);
+ S_exec_failed(aTHX_ (really ? tmps : PL_Argv[0] ? PL_Argv[0] : ""), fd, do_report);
}
do_execfree();
#endif

- ilmari
--
- Twitter seems more influential [than blogs] in the 'gets reported in
the mainstream press' sense at least. - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
to a mainstream media article. - Calle Dybedahl

--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law

@p5pRT
Copy link
Author

p5pRT commented Aug 9, 2017

From @pali

On Thursday 13 July 2017 10​:14​:41 Dagfinn Ilmari Mannsåker via RT wrote​:

I'm going on holiday for a week today, but unless anyone objects (or
beats me to it) I'll write some tests and push this when I get back.

Hi! If you are back, can you process this patch?

@p5pRT
Copy link
Author

p5pRT commented Oct 2, 2017

From @pali

Hi! I would like to remind patch in this ticket.

@p5pRT
Copy link
Author

p5pRT commented Oct 18, 2017

From @ilmari

pali@​cpan.org writes​:

On Thursday 13 July 2017 10​:14​:41 Dagfinn Ilmari Mannsåker via RT wrote​:

I'm going on holiday for a week today, but unless anyone objects (or
beats me to it) I'll write some tests and push this when I get back.

Hi! If you are back, can you process this patch?

Sorry for the delay. I've now merged the patch, and it'll be in the
upcoming 5.27.5 development release.

- ilmari
--
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
the consequences of." -- Skud's Meta-Law

@p5pRT
Copy link
Author

p5pRT commented Oct 21, 2017

From @jkeenan

Since we haven't observed any test failures since Ilmari committed the patch, I'm marking this ticket Resolved.
--
James E Keenan (jkeenan@​cpan.org)

@p5pRT p5pRT closed this as completed Oct 21, 2017
@p5pRT
Copy link
Author

p5pRT commented Oct 21, 2017

@jkeenan - Status changed from 'open' 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