null ptr deref, segfault in Perl_do_aexec5 (doio.c:1595)

[p5pRT]

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

Searchable as RT129888$

[p5pRT]

From @geeknik

Triggered with AFL+ASAN in Perl v5.25.6 (v5.25.5-104-gaff2be5).

od -tx1 test274
0000000 7b 24 30 3d 24 53 49 47 7b 5f 5f 57 41 52 4e 5f
0000020 5f 7d 3d 73 75 62 7b 65 78 65 63 7d 3b 24 5e 57
0000040 3d 32 2c 65 78 65 63 24 70 00 30 7d
0000054

Number found where operator expected at test274 line 1, near "$p0"
  (Missing operator before 0?)
ASAN​:SIGSEGV

==6592==ERROR​: AddressSanitizer​: SEGV on unknown address 0x000000000000 (pc
0x000000b30ea7 bp 0x7fffff249420 sp 0x7fffff249280 T0)
  #0 0xb30ea6 in Perl_do_aexec5 /root/perl/doio.c​:1595​:13
  #1 0xaf9bbc in Perl_pp_exec /root/perl/pp_sys.c​:4515​:15
  #2 0x7f44b3 in Perl_runops_debug /root/perl/dump.c​:2246​:23
  #3 0x5a12b6 in S_run_body /root/perl/perl.c​:2526​:2
  #4 0x5a12b6 in perl_run /root/perl/perl.c​:2449
  #5 0x4de60d in main /root/perl/perlmain.c​:123​:9
  #6 0x7f7211793b44 in __libc_start_main
/build/glibc-daoqzt/glibc-2.19/csu/libc-start.c​:287
  #7 0x4de27c in _start (/root/perl/perl+0x4de27c)

AddressSanitizer can not provide additional info.
SUMMARY​: AddressSanitizer​: SEGV /root/perl/doio.c​:1595 Perl_do_aexec5
==6592==ABORTING

Number found where operator expected at test274 line 1, near "$p0"
  (Missing operator before 0?)
==6567== Invalid read of size 8
==6567== at 0x5A7B17​: Perl_do_aexec5 (doio.c​:1595)
==6567== by 0x598CD4​: Perl_pp_exec (pp_sys.c​:4515)
==6567== by 0x4D71A1​: Perl_runops_debug (dump.c​:2246)
==6567== by 0x453156​: S_run_body (perl.c​:2526)
==6567== by 0x453156​: perl_run (perl.c​:2449)
==6567== by 0x421984​: main (perlmain.c​:123)
==6567== Address 0x0 is not stack'd, malloc'd or (recently) free'd
==6567==
==6567==
==6567== Process terminating with default action of signal 11 (SIGSEGV)
==6567== Access not within mapped region at address 0x0
==6567== at 0x5A7B17​: Perl_do_aexec5 (doio.c​:1595)
==6567== by 0x598CD4​: Perl_pp_exec (pp_sys.c​:4515)
==6567== by 0x4D71A1​: Perl_runops_debug (dump.c​:2246)
==6567== by 0x453156​: S_run_body (perl.c​:2526)
==6567== by 0x453156​: perl_run (perl.c​:2449)
==6567== by 0x421984​: main (perlmain.c​:123)
==6567== If you believe this happened as a result of a stack
==6567== overflow in your program's main thread (unlikely but
==6567== possible), you can try to increase the size of the
==6567== main thread stack using the --main-stacksize= flag.
==6567== The main thread stack size used in this run was 8388608.
Segmentation fault

Number found where operator expected at test274 line 1, near "$p0"
  (Missing operator before 0?)

Program received signal SIGSEGV, Segmentation fault.
0x00000000005a7b17 in Perl_do_aexec5 (really=0x939998, mark=0x919b70,
mark@​entry=0x919b60, sp=sp@​entry=0x919b68, fd=fd@​entry=0,
do_report=do_report@​entry=0) at doio.c​:1595
1595 PerlProc_execvp(PL_Argv[0],EXEC_ARGV_CAST(PL_Argv));
(gdb) bt
#0 0x00000000005a7b17 in Perl_do_aexec5 (really=0x939998, mark=0x919b70,
mark@​entry=0x919b60, sp=sp@​entry=0x919b68, fd=fd@​entry=0,
do_report=do_report@​entry=0) at doio.c​:1595
#1 0x0000000000598cd5 in Perl_pp_exec () at pp_sys.c​:4515
#2 0x00000000004d71a2 in Perl_runops_debug () at dump.c​:2246
#3 0x0000000000453157 in S_run_body (oldscope=1) at perl.c​:2526
#4 perl_run (my_perl=<optimized out>) at perl.c​:2449
#5 0x0000000000421985 in main (argc=2, argv=0x7fffffffe6b8,
env=0x7fffffffe6d0) at perlmain.c​:123
(gdb) list
1590 TAINT_ENV(); /* testing IFS here is
overkill, probably */
1591 PERL_FPU_PRE_EXEC
1592 if (really && *tmps) {
1593 PerlProc_execvp(tmps,EXEC_ARGV_CAST(PL_Argv));
1594 } else {
1595 PerlProc_execvp(PL_Argv[0],EXEC_ARGV_CAST(PL_Argv));
1596 }
1597 PERL_FPU_POST_EXEC
1598 S_exec_failed(aTHX_ (really ? tmps : PL_Argv[0]), fd,
do_report);
1599 }
(gdb) i r
rax 0x0 0
rbx 0x6193c4 6394820
rcx 0x0 0
rdx 0x7ffff6cf20e0 140737334157536
rsi 0x7fffffffe140 140737488347456
rdi 0x8 8
rbp 0x7fffffffe360 0x7fffffffe360
rsp 0x7fffffffe350 0x7fffffffe350
r8 0x7fffffffe360 140737488348000
r9 0x7fffffffe1e0 140737488347616
r10 0x8 8
r11 0x206 518
r12 0x932d10 9645328
r13 0x939998 9673112
r14 0x932d20 9645344
r15 0x919b70 9542512
rip 0x5a7b17 0x5a7b17 <Perl_do_aexec5+359>
eflags 0x10246 [ PF ZF IF RF ]
cs 0x33 51
ss 0x2b 43
ds 0x0 0
es 0x0 0
fs 0x0 0
gs 0x0 0

[p5pRT]

From @geeknik

test274.gz

[p5pRT]

From @hvds

In short​:

% ./miniperl -we '$SIG{__WARN__} = sub { exec }; exec $p 0;'
Name "main​::p" used only once​: possible typo at -e line 1.
Segmentation fault (core dumped)
%

Fetching the exec arguments in do_aexec5() can trigger arbitrary perl code, which can cause the PL_Argv array we're building up to get removed before we're done.

I'm not sure what's the best approach here; here's a couple of possibilities​:

# disallow reentrant calls​: UNLIKELY(breaks some working code)
--- a/doio.c
+++ b/doio.c
@​@​ -1574,6 +1574,8 @​@​ 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 (PL_Argv)
+ Perl_croak(aTHX_ "panic​: do_aexec5() called reentrantly");
  if (sp > mark) {
  const char **a;
  const char *tmps = NULL;

# set PL_Argv as late as possible​: UNLIKELY(leaks some memory)
--- a/doio.c
+++ b/doio.c
@​@​ -1575,10 +1575,11 @​@​ Perl_do_aexec5(pTHX_ SV *really, SV **mark, SV **sp,
  Perl_croak(aTHX_ "exec? I'm not *that* kind of operating system");
#else
  if (sp > mark) {
+ const char **argv;
  const char **a;
  const char *tmps = NULL;
- Newx(PL_Argv, sp - mark + 1, const char*);
- a = PL_Argv;
+ Newx(argv, sp - mark + 1, const char*);
+ a = argv;

  while (++mark <= sp) {
  if (*mark)
@​@​ -1589,10 +1590,11 @​@​ 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 && *argv[0] != '/') ||
  (really && *tmps != '/')) /* will execvp use PATH? */
  TAINT_ENV(); /* testing IFS here is overkill, probably */
  PERL_FPU_PRE_EXEC
+ PL_Argv = argv;
  if (really && *tmps) {
  PerlProc_execvp(tmps,EXEC_ARGV_CAST(PL_Argv));
  } else {

Hugo

[p5pRT]

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

[p5pRT]

From zefram@fysh.org

The globals that are causing this problem don't need to be global at all.
They're only managing the freeing of that memory, and I reckon we can do
better by using the scope stack to arrange freeing. I've implemented this
on the branch zefram/smoke-me/exec_args. It fixes the original test case
on Unix. However, there's a bunch of platform-specific code in this area,
so this needs broader testing. It also needs broad testing to make sure
the scope stack arrangement actually achieves correct memory management,
for example around vfork and Windows's simulated fork.

-zefram

[p5pRT]

From @jkeenan

On 12/06/2017 04​:37 PM, Zefram wrote​:

The globals that are causing this problem don't need to be global at all.
They're only managing the freeing of that memory, and I reckon we can do
better by using the scope stack to arrange freeing. I've implemented this
on the branch zefram/smoke-me/exec_args.

For whatever benefit there might be of creating a branch for
smoke-testing, you probably should rename it​:

smoke-me/zefram/exec_args

That way (a) in the rare case of someone (i.e., TonyC) who has set up
automated smoke-testing set up for non-blead branches, it gets
recognized and run; and (b) its results get tabulated at
perl.develop-help.com (e.g.,
http​://perl.develop-help.com/?b=smoke-me%2Fkhw-locale).

Thank you very much.
Jim Keenan

[p5pRT]

From zefram@fysh.org

James E Keenan wrote​:

For whatever benefit there might be of creating a branch for smoke-testing,
you probably should rename it​:

Really? I had got the impression that the "smoke-me" segment could be
anywhere in the name. I see that perlgit(1) doesn't actually say what the
rules are. Could someone authoritative please give us a regexp defining
what constitutes a smoke-me branch name? And put it in perlgit(1).

-zefram

[p5pRT]

From @jkeenan

On 12/07/2017 02​:52 PM, Zefram wrote​:

James E Keenan wrote​:

For whatever benefit there might be of creating a branch for smoke-testing,
you probably should rename it​:

Really? I had got the impression that the "smoke-me" segment could be
anywhere in the name. I see that perlgit(1) doesn't actually say what the
rules are. Could someone authoritative please give us a regexp defining
what constitutes a smoke-me branch name? And put it in perlgit(1).

Zefram​:

I've had a commit bit since 2012. How many more years do I have to
serve before you consider me "authoritative"?

For what it's worth, all three of the examples of a smoke-me branch
presented in perlgit follow the "smoke-me/<committer>/<branch_name>"
convention.

In addition, if you go to http​://perl.develop-help.com/, click on the
drop-down that says 'blead', then click on "I want an older branch" you
will see that all the branches with Tony tracks on that site begin with
'blead', 'maint' or 'smoke-me'. The <committer> part appears to be
optional, but 'smoke-me' always appears first.

Thank you very much.
Jim Keenan

[p5pRT]

From zefram@fysh.org

James E Keenan wrote​:

I've had a commit bit since 2012. How many more years do I have to serve
before you consider me "authoritative"?

By "authoritative" I meant someone who maintains the smoke-me automation,
and so can look at the code and tell us first-hand what the said
automation actually does. It's about the role one plays, not about
expertise, seniority, or other qualities.

-zefram

[p5pRT]

From carlos@carlosguevara.com

On Thu, Dec 7, 2017 at 11​:31 PM, Zefram <zefram@​fysh.org> wrote​:

James E Keenan wrote​:

I've had a commit bit since 2012. How many more years do I have to serve
before you consider me "authoritative"?

By "authoritative" I meant someone who maintains the smoke-me automation,
and so can look at the code and tell us first-hand what the said
automation actually does. It's about the role one plays, not about
expertise, seniority, or other qualities.

-zefram

I use this command on my systems​:
git for-each-ref --count=3 --sort=-committerdate refs/heads/smoke-me
--format='%(refname​:short)'

[p5pRT]

From @xsawyerx

On 12/08/2017 05​:44 AM, James E Keenan wrote​:

On 12/07/2017 02​:52 PM, Zefram wrote​:

James E Keenan wrote​:

For whatever benefit there might be of creating a branch for
smoke-testing,
you probably should rename it​:

Really?  I had got the impression that the "smoke-me" segment could be
anywhere in the name.  I see that perlgit(1) doesn't actually say
what the
rules are.  Could someone authoritative please give us a regexp defining
what constitutes a smoke-me branch name?  And put it in perlgit(1).

Zefram​:

I've had a commit bit since 2012.  How many more years do I have to
serve before you consider me "authoritative"?

I think the intention here is for "definitive," as in "What does the
darn regexp looks like, so we can know for sure."

Funny story​: I asked my brother for his car's code. His partner gave me
a different code. He jumped up and said "No, that's the wrong code. You
swapped two digits!" She replied with "This is the number I use every
day." And it so happens that both of the numbers indeed work.[1]

The regexp could be /^smoke-me/ or /smoke-me/. Either case, it would be
good to know which it is. Still, I support your concern for having
uniformity, regardless of where you put the "smoke-me".

[p5pRT]

From @craigberry

On Mon, Dec 11, 2017 at 6​:59 AM, Sawyer X <xsawyerx@​gmail.com> wrote​:

On 12/08/2017 05​:44 AM, James E Keenan wrote​:

On 12/07/2017 02​:52 PM, Zefram wrote​:

I had got the impression that the "smoke-me" segment could be
anywhere in the name. I see that perlgit(1) doesn't actually say
what the
rules are. Could someone authoritative please give us a regexp defining
what constitutes a smoke-me branch name? And put it in perlgit(1).

The regexp could be /^smoke-me/ or /smoke-me/. Either case, it would be
good to know which it is. Still, I support your concern for having
uniformity, regardless of where you put the "smoke-me".

As far as I know the automatic selection of what branch or branches to
smoke is external to Test​::Smoke and the use of "smoke-me" in the name
is just a convention to be used in whatever fashion the implementor of
a smoke set-up sees fit. It's not even necessarily implemented
exactly the same way in every smoke-me set-up (of which there have
never been more than a couple, I think). And of course there are a
number of smokers that only do blead or only blead and maint and don't
do the smoke-me branches. I would be happy to discover that there is
more consistency and ready-to-use code for automatic branch selection
than I am aware of, but this is my impression of where we are now.

That said, I've always assumed the convention was that the branch
should start with "smoke-me" and the one example in perlgit does that.
Eyeballing the lengthy list of existing branches at
<https://perl5.git.perl.org/perl.git/heads>  does not turn up any
examples prior to Zefram's that puts the "smoke-me" anywhere but at
the beginning. In the absence of better information, I'd say we
already have a convention and this is it​: "smoke-me" at the beginning.

[p5pRT]

From zefram@fysh.org

Carlos Guevara wrote​:

I use this command on my systems​:
git for-each-ref --count=3 --sort=-committerdate refs/heads/smoke-me

Thanks, that makes it clear. I've renamed my branch to
"smoke-me/zefram/exec_args", and pushed a patch for perlgit.pod.

-zefram

[p5pRT]

From @xsawyerx

On 12/11/2017 08​:29 PM, Craig A. Berry wrote​:

On Mon, Dec 11, 2017 at 6​:59 AM, Sawyer X <xsawyerx@​gmail.com> wrote​:

On 12/08/2017 05​:44 AM, James E Keenan wrote​:

On 12/07/2017 02​:52 PM, Zefram wrote​:

I had got the impression that the "smoke-me" segment could be
anywhere in the name. I see that perlgit(1) doesn't actually say
what the
rules are. Could someone authoritative please give us a regexp defining
what constitutes a smoke-me branch name? And put it in perlgit(1).
The regexp could be /^smoke-me/ or /smoke-me/. Either case, it would be
good to know which it is. Still, I support your concern for having
uniformity, regardless of where you put the "smoke-me".
[...]

That said, I've always assumed the convention was that the branch
should start with "smoke-me" and the one example in perlgit does that.
Eyeballing the lengthy list of existing branches at
<https://perl5.git.perl.org/perl.git/heads>  does not turn up any
examples prior to Zefram's that puts the "smoke-me" anywhere but at
the beginning. In the absence of better information, I'd say we
already have a convention and this is it​: "smoke-me" at the beginning.

Agreed.

[p5pRT]

From zefram@fysh.org

smoke-me testing didn't cover as many platforms as I'd hoped, but at
least didn't show any relevant failures. Patch applied to blead as
commit 282fc0b.

-zefram