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

Fix annoying silent crash in builtins.sh under ASan #810

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

JohnoKing
Copy link

Under ASan the builtins.sh script silently exits with exit status 1 and no backtrace. After isolating the bug into a simplified script, I managed to reproduce it with an actual stacktrace. Reproducer:

#!/bin/ksh
set -o xtrace  # Shows how far the script goes before crashing
               # (which is inconsistent and system-dependent
               # AFAICT; the ALL_LIBCMD shopt can influence it).
bltin='/opt/ast/bin/basename
/opt/ast/bin/cat
/opt/ast/bin/cp
/opt/ast/bin/cut
/opt/ast/bin/dirname
/opt/ast/bin/getconf
/opt/ast/bin/ln
/opt/ast/bin/mktemp
/opt/ast/bin/mv'  # Feel free to expand this with other commands
                  # from libcmd if you with.
for i in ${bltin}
do ({ PATH=/opt/ast/bin; "${bltin##*/}" --this-option-does-not-exist; } 2>&1)
done

Stacktrace obtained after much effort:

=================================================================
==116622==ERROR: AddressSanitizer: heap-use-after-free on address 0x502000000e50 at pc 0x72ad1bb52b7f bp 0x7ffc8b5a0cd0 sp 0x7ffc8b5a0478 READ of size 3 at 0x502000000e50 thread T0
    #0 0x72ad1bb52b7e in memcpy /usr/src/debug/gcc/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors_memintrinsics.inc:115
    #1 0x5bb0a922f3de in synthesize /home/johno/GitRepos/KornShell/ksh/src/lib/libast/port/astconf.c:466
    #2 0x5bb0a92305cd in initialize /home/johno/GitRepos/KornShell/ksh/src/lib/libast/port/astconf.c:591
    #3 0x5bb0a9230fac in format /home/johno/GitRepos/KornShell/ksh/src/lib/libast/port/astconf.c:632
    #4 0x5bb0a923ba4b in astgetconf /home/johno/GitRepos/KornShell/ksh/src/lib/libast/port/astconf.c:1382
    #5 0x5bb0a923d5c5 in astconf /home/johno/GitRepos/KornShell/ksh/src/lib/libast/port/astconf.c:1472
    #6 0x5bb0a924e07b in initconformance /home/johno/GitRepos/KornShell/ksh/src/lib/libast/misc/conformance.c:50
    #7 0x5bb0a924eff2 in conformance /home/johno/GitRepos/KornShell/ksh/src/lib/libast/misc/conformance.c:122
    #8 0x5bb0a9288b8e in b_cp /home/johno/GitRepos/KornShell/ksh/src/lib/libcmd/cp.c:706
    <CUT>

src/lib/libast/port/astconf.c:

  • If fp->value and value pointed to the same allocated memory before realloc, avoid memcpy as value now points to freed memory.
  • Produce an obvious panic if memory allocation fails.

src/cmd/ksh93/sh/init.c:

  • For correctness, prevent memory leaks by freeing memory in sh_realloc upon failure.

Under ASan the builtins.sh script silently exits with exit status 1
and no backtrace. After isolating the bug into a simplified script,
I managed to reproduce it with an actual stacktrace. Reproducer:
   set -o xtrace  # Shows how far the script goes before crashing
                  # (which is inconsistent and system-dependent
                  # AFAICT; ALL_LIBCMD can affect it).
   bltin='/opt/ast/bin/basename
   /opt/ast/bin/cat
   /opt/ast/bin/cp
   /opt/ast/bin/cut
   /opt/ast/bin/dirname
   /opt/ast/bin/getconf
   /opt/ast/bin/ln
   /opt/ast/bin/mktemp
   /opt/ast/bin/mv'  # Feel free to expand this with other commands
                     # from libcmd if you with.
   for i in ${bltin}
   do ({ PATH=/opt/ast/bin; "${bltin##*/}" --this-option-does-not-exist; } 2>&1)
   done
Stacktrace obtained after much effort:
=================================================================
==116622==ERROR: AddressSanitizer: heap-use-after-free on address 0x502000000e50 at pc 0x72ad1bb52b7f bp 0x7ffc8b5a0cd0 sp 0x7ffc8b5a0478
READ of size 3 at 0x502000000e50 thread T0
    #0 0x72ad1bb52b7e in memcpy /usr/src/debug/gcc/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors_memintrinsics.inc:115
    ksh93#1 0x5bb0a922f3de in synthesize /home/johno/GitRepos/KornShell/ksh/src/lib/libast/port/astconf.c:466
    ksh93#2 0x5bb0a92305cd in initialize /home/johno/GitRepos/KornShell/ksh/src/lib/libast/port/astconf.c:591
    ksh93#3 0x5bb0a9230fac in format /home/johno/GitRepos/KornShell/ksh/src/lib/libast/port/astconf.c:632
    ksh93#4 0x5bb0a923ba4b in astgetconf /home/johno/GitRepos/KornShell/ksh/src/lib/libast/port/astconf.c:1382
    ksh93#5 0x5bb0a923d5c5 in astconf /home/johno/GitRepos/KornShell/ksh/src/lib/libast/port/astconf.c:1472
    ksh93#6 0x5bb0a924e07b in initconformance /home/johno/GitRepos/KornShell/ksh/src/lib/libast/misc/conformance.c:50
    ksh93#7 0x5bb0a924eff2 in conformance /home/johno/GitRepos/KornShell/ksh/src/lib/libast/misc/conformance.c:122
    ksh93#8 0x5bb0a9288b8e in b_cp /home/johno/GitRepos/KornShell/ksh/src/lib/libcmd/cp.c:706
    <CUT>

src/lib/libast/port/astconf.c:
- If fp->value and value pointed to the same allocated memory
  before realloc, avoid memcpy as value now points to freed memory.
- Produce an obvious panic if memory allocation fails.

src/cmd/ksh93/sh/init.c:
- For correctness, prevent memory leaks by freeing memory in sh_realloc
  upon failure.
@McDutchie
Copy link

McDutchie commented Jan 5, 2025

Odd. The builtins.sh script passes without error (silent or otherwise) under ASan on my system.

The PR looks good to me.

@McDutchie
Copy link

Actually, I'm not sure if that code should be throwing out of memory errors directly. Elsewhere in astconf.c, error messages (including out of memory) are passed on via a function given as a conferror pointer argument.

fp->flags |= CONF_ALLOC;
if(docpy)
memcpy(fp->value, value, n);
fp->value[n] = 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the terminating 0 not be subject to docpy as well?

@McDutchie
Copy link

I no longer think this change is correct. I think the memcpy always needs to happen, otherwise the memory added to the block with realloc is simply going to be left uninitialised if docpy is false.

I'm too tired to reason this out further right now so I'll have another look another day.

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

Successfully merging this pull request may close these issues.

2 participants