Commit a1325b902d ("try" support) can read uninitialized memory

[hvds]

As initially reported by George Greer to p5p, building with sanitize=address shows that this commit causes reads of uninitialized memory, for example when a sort block exits with an explicit return:

==3168==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61c000000018 at pc 0x560bb1fe782a bp 0x7ffcd34f9a10 sp 0x7ffcd34f9a00
READ of size 1 at 0x61c000000018 thread T0
    #0 0x560bb1fe7829 in Perl_pp_return /src/perl/git/pp_ctl.c:2575
    #1 0x560bb1ee2642 in Perl_runops_standard /src/perl/git/run.c:41
    #2 0x560bb211eb85 in S_sortcv /src/perl/git/pp_sort.c:1066
    #3 0x560bb2145825 in dynprep /src/perl/git/pp_sort.c:188
    #4 0x560bb2145825 in S_sortsv_flags_impl /src/perl/git/pp_sort.c:358
    #5 0x560bb2145825 in Perl_sortsv_flags /src/perl/git/pp_sort.c:556
    #6 0x560bb2147f6b in Perl_pp_sort /src/perl/git/pp_sort.c:909
    #7 0x560bb1ee2642 in Perl_runops_standard /src/perl/git/run.c:41
    #8 0x560bb1cdd700 in S_run_body /src/perl/git/perl.c:2738
    #9 0x560bb1cdd700 in perl_run /src/perl/git/perl.c:2666
    #10 0x560bb1c57636 in main /src/perl/git/miniperlmain.c:116
    #11 0x7f6ef677abf6 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21bf6)
    #12 0x560bb1c576d9 in _start (/src/perl/git/miniperl+0xe06d9)

0x61c000000018 is located 104 bytes to the left of 1872-byte region [0x61c000000080,0x61c0000007d0)
allocated by thread T0 here:
    #0 0x7f6ef741db40 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb40)
    #1 0x560bb1e66c82 in Perl_safesysmalloc /src/perl/git/util.c:161

SUMMARY: AddressSanitizer: heap-buffer-overflow /src/perl/git/pp_ctl.c:2575 in Perl_pp_return
Shadow bytes around the buggy address:
  0x0c387fff7fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c387fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c387fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c387fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c387fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c387fff8000: fa fa fa[fa]fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c387fff8010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c387fff8020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c387fff8030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c387fff8040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c387fff8050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==3168==ABORTING

This occurs at least when the cxix found at the start of pp_return() is negative, and during a normal build causes lib/unicore/mktables to fail.

The diff below is sufficient to get make minitest to pass, but it isn't clear if it fully solves the problem: for example, other code in pp_return() also checks for cxix < cxstack_ix. It seems likely to me that the whole CxTRY test should probably be moved later in the function.

@leonerd suggests we apply this diff; @tonycoz or @iabyn could you check it over?

Hugo

diff --git a/pp_ctl.c b/pp_ctl.c
index 9e440dcd9b..6ef2ba17bb 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -2487,11 +2487,13 @@ PP(pp_return)
     I32 cxix = dopopto_cursub();
 
 again:
-    cx = &cxstack[cxix];
-    if(CxTRY(cx)) {
-        /* This was a try {}. keep going */
-        cxix = dopoptosub_at(cxstack, cxix - 1);
-        goto again;
+    if (cxix >= 0) {
+        cx = &cxstack[cxix];
+        if (CxTRY(cx)) {
+            /* This was a try {}. keep going */
+            cxix = dopoptosub_at(cxstack, cxix - 1);
+            goto again;
+        }
     }
 
     assert(cxstack_ix >= 0);

[jkeenan]

diff --git a/pp_ctl.c b/pp_ctl.c
index 9e440dcd9b..6ef2ba17bb 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -2487,11 +2487,13 @@ PP(pp_return)
     I32 cxix = dopopto_cursub();
 
 again:
-    cx = &cxstack[cxix];
-    if(CxTRY(cx)) {
-        /* This was a try {}. keep going */
-        cxix = dopoptosub_at(cxstack, cxix - 1);
-        goto again;
+    if (cxix >= 0) {
+        cx = &cxstack[cxix];
+        if (CxTRY(cx)) {
+            /* This was a try {}. keep going */
+            cxix = dopoptosub_at(cxstack, cxix - 1);
+            goto again;
+        }
     }
 
     assert(cxstack_ix >= 0);

@hvds, this is the same as what we've placed in the smoke-me/jkeenan/hv/pp_ctl-20210207 branch -- correct?

jimk

[hvds]

@hvds, this is the same as what we've placed in the smoke-me/jkeenan/hv/pp_ctl-20210207 branch -- correct?

Yes, this was intended as a synopsis of the p5p thread for ease of tracking.

[tonycoz]

The fix looks reasonable to me.

[hvds]

The fix looks reasonable to me.

Thanks, I've committed this as 5fa8d5d.

I was hoping for a more specific response to my concerns above:

it isn't clear if it fully solves the problem: for example, other code in pp_return() also checks for cxix < cxstack_ix. It seems likely to me that the whole CxTRY test should probably be moved later in the function.

.. but perhaps they could better be addressed by adding some comments to pp_return() clarifying why things are done in that precise order.

[tonycoz]

.. but perhaps they could better be addressed by adding some comments to pp_return() clarifying why things are done in that precise order.

I don't think there's a problem with the order the code works in, there's two parts of pp_return, first found the sub context to return from, then pop contexts to that point and do the return, and the added try handling code is part of the first.

I do think that the additional code that's doing what dopopto_cursub() should be doing is a bit suspicious.

I looked over what else calls dopopto_cursub(), and ended up with:

$ gdb --args ./perl -Ilib -Mfeature=try -le 'my $x = 0; sub foo :lvalue { try { print "Hello\n"; return %x } catch ($e) { } } foo() = 1; print $x'
...
(gdb) r
Starting program: /home/tony/dev/perl/git/perl4/perl -Ilib -Mfeature=try -le my\ \$x\ =\ 0\;\ sub\ foo\ :lvalue\ \{\ try\ \{\ print\ \"Hello\\n\"\;\ return\ %x\ \}\ catch\ \(\$e\)\ \{\ \}\ \}\ foo\(\)\ =\ 1\;\ print\ \$x
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
try/catch is experimental at -e line 1.
try/catch is experimental at -e line 1.
Hello


Program received signal SIGSEGV, Segmentation fault.
0x00005555558440e9 in Perl_is_lvalue_sub () at pp_ctl.c:1416
1416        if (CxLVAL(cxstack + cxix) && CvLVALUE(cxstack[cxix].blk_sub.cv))
(gdb) bt
#0  0x00005555558440e9 in Perl_is_lvalue_sub () at pp_ctl.c:1416
#1  0x000055555577c005 in Perl_pp_rv2av () at pp_hot.c:1980
#2  0x0000555555721e8d in Perl_runops_debug () at dump.c:2572
#3  0x00005555555f1102 in S_run_body (oldscope=1) at perl.c:2743
#4  0x00005555555f0680 in perl_run (my_perl=0x555555c17260) at perl.c:2666
#5  0x00005555555a2122 in main (argc=5, argv=0x7fffffffe778, 
    env=0x7fffffffe7a8) at perlmain.c:110

I haven't worked out a fix yet.

[tonycoz]

@leonerd ^^

[hvds]

I note that after this commit, George's smoker that originally complained with read access violations is now getting further, but still has some test failures on threaded builds - see here.

I tried to make a similar build locally, with:

./Configure -des -Dcc=clang -Dprefix=/opt/scratch \
  -Accflags='-DPERL_POISON -Werror=declaration-after-statement -fsanitize=address' \
  -Aldflags='-fsanitize=address' -Doptimize='-g -O3' -Dusethread -Duseithreads \
  -DDEBUGGING -Dusedevel -Uversiononly

.. but it complains of all sorts of memory leaks, as below.

These complaints predate the "try" work (and don't make much sense to me), could someone else see if they can reproduce the test failures in io/errnosig.t and op/magic.t?

% ./miniperl -w -Ilib -Idist/Exporter/lib -MExporter -e '<?>'

=================================================================
==22020==ERROR: LeakSanitizer: detected memory leaks

Indirect leak of 60672 byte(s) in 32 object(s) allocated from:
    #0 0x4dcb50 in malloc (/src/perl/git/miniperl+0x4dcb50)
    #1 0x515fcb in S_new_slab /src/perl/git/op.c:253:22

Indirect leak of 2484 byte(s) in 63 object(s) allocated from:
    #0 0x4dcb50 in malloc (/src/perl/git/miniperl+0x4dcb50)
    #1 0x794463 in Perl_savesharedpv /src/perl/git/util.c:1413:22

Indirect leak of 560 byte(s) in 10 object(s) allocated from:
    #0 0x4dcd78 in calloc (/src/perl/git/miniperl+0x4dcd78)
    #1 0x5150d9 in Perl_Slab_Alloc /src/perl/git/op.c:333:11

Indirect leak of 520 byte(s) in 9 object(s) allocated from:
    #0 0x4dcb50 in malloc (/src/perl/git/miniperl+0x4dcb50)
    #1 0x591de2 in S_maybe_multiconcat /src/perl/git/op.c:3333:27
    #2 0x5226e2 in S_optimize_op /src/perl/git/op.c:3741:13
    #3 0x67a8e5 in Perl_yyparse /src/perl/git/perly.y:324:12
    #4 0x98af35 in S_doeval_compile /src/perl/git/pp_ctl.c:3567:77

Indirect leak of 312 byte(s) in 10 object(s) allocated from:
    #0 0x4dcb50 in malloc (/src/perl/git/miniperl+0x4dcb50)
    #1 0x58c969 in S_maybe_multideref /src/perl/git/op.c:16548:39

Indirect leak of 192 byte(s) in 9 object(s) allocated from:
    #0 0x4dcd78 in calloc (/src/perl/git/miniperl+0x4dcd78)
    #1 0x5164a4 in S_link_freed_op /src/perl/git/op.c:283:36

Indirect leak of 96 byte(s) in 3 object(s) allocated from:
    #0 0x4dcfd0 in realloc (/src/perl/git/miniperl+0x4dcfd0)
    #1 0x5163e8 in S_link_freed_op /src/perl/git/op.c:294:25

Indirect leak of 95 byte(s) in 9 object(s) allocated from:
    #0 0x4dcb50 in malloc (/src/perl/git/miniperl+0x4dcb50)
    #1 0x591dfd in S_maybe_multiconcat /src/perl/git/op.c:3340:25
    #2 0x5226e2 in S_optimize_op /src/perl/git/op.c:3741:13
    #3 0x67a8e5 in Perl_yyparse /src/perl/git/perly.y:324:12
    #4 0x98af35 in S_doeval_compile /src/perl/git/pp_ctl.c:3567:77

SUMMARY: AddressSanitizer: 64931 byte(s) leaked in 145 allocation(s).
Failed to build miniperl. Please run make minitest
makefile:363: recipe for target 'lib/buildcustomize.pl' failed
make: *** [lib/buildcustomize.pl] Error 1

[tonycoz]

.. but it complains of all sorts of memory leaks, as below.

These complaints predate the "try" work (and don't make much sense to me), could someone else see if they can reproduce the test failures in io/errnosig.t and op/magic.t?

% ./miniperl -w -Ilib -Idist/Exporter/lib -MExporter -e '<?>'

Is this with PERL_DESTRUCT_LEVEL=2 ?

[hvds]

Is this with PERL_DESTRUCT_LEVEL=2 ?

Ah no, thanks, I'd completely forgotten about that one; with that in place perl builds and passes those tests for me without a problem. And looking back a bit further, I see that the smoker was already failing those tests before the "try" work was introduced, so they aren't relevant to this ticket - I've created #18547 to track that instead.

So as far as the original issue is concerned this ticket could be closed now. @tonycoz it may be worth putting your additional problem case in its own ticket.

[jkeenan]

Closing per OP @hvds's recommendation.

[hvds]

Closing per OP @hvds's recommendation.

@jkeenan Sorry if I was unclear: what I wrote was intended to imply "this ticket could be closed now if it were not for the additional issue raised by Tony. It should not be closed until that is either resolved or moved to a new ticket.

It isn't clear to me how closely it relates to the rest of this ticket, so I'd rather leave it to the judgement of @tonycoz and/or @leonerd whether to continue it here or move it.

[tonycoz]

Moved my unrelated crash to #18553