Skip to content

Commit a9283be

Browse files
JohnoKingMcDutchie
andcommitted
Fix annoying silent crash in builtins.sh under ASan (#810)
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 #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: - Remove realloc (newof) shenanigans such that memcpy can always be used. - Produce an obvious panic if memory allocation fails. - Use conferror mechanism to post errors when memory cannot be allocated (also applied to code that ought have already had such error handling). src/cmd/ksh93/sh/init.c: - For correctness, prevent memory leaks by freeing memory in sh_realloc upon failure. Co-authored-by: Martijn Dekker <martijn@inlv.org>
1 parent f98ed55 commit a9283be

File tree

3 files changed

+45
-21
lines changed

3 files changed

+45
-21
lines changed

NEWS

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@ Uppercase BUG_* IDs are shell bug IDs as used by the Modernish shell library.
44

55
2025-05-27:
66

7+
- Fixed a crash that could occur in some scenarios when /opt/ast/bin
8+
builtins handled invalid options in virtual subshells.
9+
710
- Fixed a bug in printf where \u0025 was taken as a % operator in a
811
formatter argument. For example, printf '\u0025\n' now correctly prints a
912
% followed by a newline instead of producing an error.

src/cmd/ksh93/sh/init.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,11 @@ void *sh_realloc(void *ptr, size_t size)
242242
void *cp;
243243
cp = realloc(ptr, size);
244244
if(!cp)
245+
{
246+
if(ptr)
247+
free(ptr);
245248
nomemory(size);
249+
}
246250
return cp;
247251
}
248252

src/lib/libast/port/astconf.c

Lines changed: 38 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@
7070
#define CONF_GLOBAL (CONF_USER<<3)
7171

7272
#define DEFAULT(o) ((state.std||!dynamic[o].ast)?dynamic[o].std:dynamic[o].ast)
73-
#define INITIALIZE() do{if(!state.data)synthesize(NULL,NULL,NULL);}while(0)
73+
#define INITIALIZE() do{if(!state.data)synthesize(NULL,NULL,NULL,NULL);}while(0)
7474
#define STANDARD(v) (streq(v,"standard")||streq(v,"strict")||streq(v,"posix")||streq(v,"xopen"))
7575

7676
#define MAXVAL 256
@@ -300,12 +300,13 @@ buffer(char* s)
300300
*/
301301

302302
static char*
303-
synthesize(Feature_t* fp, const char* path, const char* value)
303+
synthesize(Feature_t* fp, const char* path, const char* value, Error_f conferror)
304304
{
305305
char* s;
306306
char* d;
307307
char* v;
308308
char* p;
309+
char* newvalue;
309310
int n;
310311

311312
#if DEBUG_astconf
@@ -326,7 +327,11 @@ synthesize(Feature_t* fp, const char* path, const char* value)
326327
n += strlen(s) + 1;
327328
n = roundof(n, 32);
328329
if (!(state.data = newof(0, char, n, 0)))
330+
{
331+
if (conferror)
332+
(*conferror)(&state, &state, 2, "synthesize(): out of memory");
329333
return NULL;
334+
}
330335
state.last = state.data + n - 1;
331336
strcpy(state.data, state.name);
332337
state.data += state.prefix - 1;
@@ -434,7 +439,11 @@ synthesize(Feature_t* fp, const char* path, const char* value)
434439
c = n + state.last - state.data + 3 * MAXVAL;
435440
c = roundof(c, 32);
436441
if (!(state.data = newof(state.data, char, c, 0)))
442+
{
443+
if (conferror)
444+
(*conferror)(&state, &state, 2, "synthesize(): out of memory");
437445
return NULL;
446+
}
438447
state.last = state.data + c - 1;
439448
state.data += state.prefix;
440449
d = state.data + i;
@@ -458,15 +467,22 @@ synthesize(Feature_t* fp, const char* path, const char* value)
458467
fp->value = 0;
459468
if (n == 1 && (*value == '0' || *value == '-'))
460469
n = 0;
461-
if (!(fp->value = newof(fp->value, char, n, 1)))
462-
fp->value = null;
463-
else
470+
if(!(newvalue = malloc(n + 1)))
464471
{
465-
fp->flags |= CONF_ALLOC;
466-
memcpy(fp->value, value, n);
467-
fp->value[n] = 0;
472+
if(fp->value && fp->value != null)
473+
free(fp->value);
474+
fp->value = null;
475+
if (conferror)
476+
(*conferror)(&state, &state, 2, "synthesize(): out of memory");
477+
return NULL;
468478
}
469-
return fp->value;
479+
/* memcpy comes before free because fp->value and value might share memory */
480+
memcpy(newvalue, value, n);
481+
newvalue[n] = 0;
482+
if(fp->value && fp->value != null)
483+
free(fp->value);
484+
fp->flags |= CONF_ALLOC;
485+
return fp->value = newvalue;
470486
}
471487

472488
/*
@@ -477,7 +493,7 @@ synthesize(Feature_t* fp, const char* path, const char* value)
477493
*/
478494

479495
static void
480-
initialize(Feature_t* fp, const char* path, const char* command, const char* succeed, const char* fail)
496+
initialize(Feature_t* fp, const char* path, const char* command, const char* succeed, const char* fail, Error_f conferror)
481497
{
482498
char* p;
483499
int ok = 1;
@@ -588,7 +604,7 @@ initialize(Feature_t* fp, const char* path, const char* command, const char* suc
588604
#if DEBUG_astconf
589605
error(-6, "state.std=%d %s [%s] std=%s ast=%s value=%s ok=%d", state.std, fp->name, ok ? succeed : fail, fp->std, fp->ast, fp->value, ok);
590606
#endif
591-
synthesize(fp, path, ok ? succeed : fail);
607+
synthesize(fp, path, ok ? succeed : fail, conferror);
592608
}
593609

594610
/*
@@ -602,9 +618,6 @@ format(Feature_t* fp, const char* path, const char* value, unsigned int flags, E
602618
int n;
603619
static struct utsname uts;
604620

605-
#ifndef UNIV_MAX
606-
NOT_USED(conferror);
607-
#endif
608621
#if DEBUG_astconf
609622
error(-6, "astconf format name=%s path=%s value=%s flags=%04x fp=%p%s", fp->name, path, value, flags, fp, state.synthesizing ? " SYNTHESIZING" : "");
610623
#else
@@ -633,8 +646,8 @@ format(Feature_t* fp, const char* path, const char* value, unsigned int flags, E
633646
#endif
634647
if (state.synthesizing && value == (char*)fp->std)
635648
fp->value = (char*)value;
636-
else if (!synthesize(fp, path, value))
637-
initialize(fp, path, NULL, fp->std, fp->value);
649+
else if (!synthesize(fp, path, value, conferror))
650+
initialize(fp, path, NULL, fp->std, fp->value, conferror);
638651
#if DEBUG_astconf
639652
error(-6, "state.std=%d %s [%s] std=%s ast=%s value=%s", state.std, fp->name, value, fp->std, fp->ast, fp->value);
640653
#endif
@@ -660,8 +673,8 @@ format(Feature_t* fp, const char* path, const char* value, unsigned int flags, E
660673
case OP_path_resolve:
661674
if (state.synthesizing && value == (char*)fp->std)
662675
fp->value = (char*)value;
663-
else if (!synthesize(fp, path, value))
664-
initialize(fp, path, NULL, "logical", DEFAULT(OP_path_resolve));
676+
else if (!synthesize(fp, path, value, conferror))
677+
initialize(fp, path, NULL, "logical", DEFAULT(OP_path_resolve), conferror);
665678
break;
666679

667680
case OP_universe:
@@ -703,7 +716,11 @@ format(Feature_t* fp, const char* path, const char* value, unsigned int flags, E
703716
fp->value = 0;
704717
n = strlen(value);
705718
if (!(fp->value = newof(fp->value, char, n, 1)))
719+
{
720+
if (conferror)
721+
(*conferror)(&state, &state, 2, "%s: out of memory", value);
706722
fp->value = null;
723+
}
707724
else
708725
{
709726
fp->flags |= CONF_ALLOC;
@@ -712,10 +729,10 @@ format(Feature_t* fp, const char* path, const char* value, unsigned int flags, E
712729
}
713730
}
714731
else
715-
synthesize(fp, path, value);
732+
synthesize(fp, path, value, conferror);
716733
}
717734
else
718-
initialize(fp, path, "echo", DEFAULT(OP_universe), "ucb");
735+
initialize(fp, path, "echo", DEFAULT(OP_universe), "ucb", conferror);
719736
#endif
720737
#endif
721738
break;
@@ -724,7 +741,7 @@ format(Feature_t* fp, const char* path, const char* value, unsigned int flags, E
724741
if (state.synthesizing && value == (char*)fp->std)
725742
fp->value = (char*)value;
726743
else
727-
synthesize(fp, path, value);
744+
synthesize(fp, path, value, conferror);
728745
break;
729746

730747
}

0 commit comments

Comments
 (0)