From 01c01fe8f604b951a2010e4b053c340e208615a1 Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Thu, 22 Apr 2021 10:13:12 -0700 Subject: [PATCH] Fix buffer overflows and memory leaks caught by ASAN (#282) The changes in this commit allow ksh to be built and run with ASan[*], although for now it only works under vmalloc. Example command to build ksh with ASan: $ bin/package make CCFLAGS='-O0 -g -fsanitize=address' [*] https://en.wikipedia.org/wiki/AddressSanitizer src/cmd/INIT/mamake.c: - Fix a few memory leaks in mamake. This doesn't fix all of the memory leaks ASan complains about (there is one remaining in the view() function), but it's enough to get ksh to build under ASan. src/lib/libast/features/map.c, src/lib/libast/misc/glob.c: - Rename the ast globbing functions to _ast_glob() and _ast_globfree(). Without this change the globbing tests fail under ASan. See: https://github.com/att/ast/commit/2c49eb6e src/cmd/ksh93/sh/{init,io,nvtree,subshell}.c: - Fix buffer overflows by using strncmp(3) instead of memcmp(3). src/cmd/ksh93/sh/name.c: - Fix another invalid usage of memcmp by using strncmp instead. This change is also in one of Red Hat's patches: https://git.centos.org/rpms/ksh/blob/c8s/f/SOURCES/ksh-20120801-nv_open-memcmp.patch Resolves: https://github.com/ksh93/ksh/issues/230 --- src/cmd/INIT/mamake.c | 26 ++++++++++++++++++++++++-- src/cmd/ksh93/sh/init.c | 2 +- src/cmd/ksh93/sh/io.c | 2 +- src/cmd/ksh93/sh/name.c | 2 +- src/cmd/ksh93/sh/nvtree.c | 4 ++-- src/cmd/ksh93/sh/subshell.c | 2 +- src/lib/libast/features/map.c | 3 +++ src/lib/libast/misc/glob.c | 4 ++-- 8 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/cmd/INIT/mamake.c b/src/cmd/INIT/mamake.c index 61e6e4284..47958e7e0 100644 --- a/src/cmd/INIT/mamake.c +++ b/src/cmd/INIT/mamake.c @@ -1079,13 +1079,23 @@ push(char* file, Stdio_t* fp, int flags) else if (++state.sp >= &state.streams[elementsof(state.streams)]) report(3, "input stream stack overflow", NiL, (unsigned long)0); if (state.sp->fp = fp) - state.sp->file = "pipeline"; + { + if(state.sp->file) + free(state.sp->file); + state.sp->file = strdup("pipeline"); + if(!state.sp->file) + report(3, "out of memory [push]", NiL, (unsigned long)0); + } else if (flags & STREAM_PIPE) report(3, "pipe error", file, (unsigned long)0); else if (!file || !strcmp(file, "-") || !strcmp(file, "/dev/stdin")) { flags |= STREAM_KEEP; - state.sp->file = "/dev/stdin"; + if(state.sp->file) + free(state.sp->file); + state.sp->file = strdup("/dev/stdin"); + if(!state.sp->file) + report(3, "out of memory [push]", NiL, (unsigned long)0); state.sp->fp = stdin; } else @@ -1095,6 +1105,8 @@ push(char* file, Stdio_t* fp, int flags) { if (!(state.sp->fp = fopen(path, "r"))) report(3, "cannot read", path, (unsigned long)0); + if(state.sp->file) + free(state.sp->file); state.sp->file = duplicate(path); drop(buf); } @@ -1446,6 +1458,7 @@ require(char* lib, int dontcare) Buf_t* tmp; struct stat st; + int tofree = 0; static int dynamic = -1; if (dynamic < 0) @@ -1490,7 +1503,10 @@ require(char* lib, int dontcare) } } if (r != lib) + { + tofree = 1; r = duplicate(r); + } search(state.vars, lib, r); append(tmp, lib + 2); append(tmp, ".req"); @@ -1519,6 +1535,8 @@ require(char* lib, int dontcare) } } fclose(f); + if(tofree) + free(r); r = use(buf); } else if (dontcare) @@ -1533,7 +1551,11 @@ require(char* lib, int dontcare) append(tmp, "rm -f x.${!-$$}.[cox]\n"); append(tmp, "exit $c\n"); if (execute(expand(buf, use(tmp)))) + { + if(tofree) + free(r); r = ""; + } } r = duplicate(r); search(state.vars, lib, r); diff --git a/src/cmd/ksh93/sh/init.c b/src/cmd/ksh93/sh/init.c index 852a14394..c92f119b9 100644 --- a/src/cmd/ksh93/sh/init.c +++ b/src/cmd/ksh93/sh/init.c @@ -1636,7 +1636,7 @@ static Namval_t *create_stat(Namval_t *np,const char *name,int flag,Namfun_t *fp for(i=0; i < sp->numnodes; i++) { nq = nv_namptr(sp->nodes,i); - if((n==0||memcmp(name,nq->nvname,n)==0) && nq->nvname[n]==0) + if((n==0||strncmp(name,nq->nvname,n)==0) && nq->nvname[n]==0) goto found; } nq = 0; diff --git a/src/cmd/ksh93/sh/io.c b/src/cmd/ksh93/sh/io.c index 249c3f51d..01284cecb 100644 --- a/src/cmd/ksh93/sh/io.c +++ b/src/cmd/ksh93/sh/io.c @@ -2703,7 +2703,7 @@ Sfio_t *sh_pathopen(const char *cp) int sh_isdevfd(register const char *fd) { - if(!fd || memcmp(fd,"/dev/fd/",8) || fd[8]==0) + if(!fd || strncmp(fd,"/dev/fd/",8) || fd[8]==0) return(0); for ( fd=&fd[8] ; *fd != '\0' ; fd++ ) { diff --git a/src/cmd/ksh93/sh/name.c b/src/cmd/ksh93/sh/name.c index 9b571bd8a..475f975ac 100644 --- a/src/cmd/ksh93/sh/name.c +++ b/src/cmd/ksh93/sh/name.c @@ -1413,7 +1413,7 @@ Namval_t *nv_open(const char *name, Dt_t *root, int flags) { if(xp->root!=root) continue; - if(*name==*xp->name && xp->namespace==shp->namespace && (flags&(NV_ARRAY|NV_NOSCOPE))==xp->flags && memcmp(xp->name,name,xp->len)==0 && (name[xp->len]==0 || name[xp->len]=='=' || name[xp->len]=='+')) + if(*name==*xp->name && xp->namespace==shp->namespace && (flags&(NV_ARRAY|NV_NOSCOPE))==xp->flags && strncmp(xp->name,name,xp->len)==0 && (name[xp->len]==0 || name[xp->len]=='=' || name[xp->len]=='+')) { sh_stats(STAT_NVHITS); np = xp->np; diff --git a/src/cmd/ksh93/sh/nvtree.c b/src/cmd/ksh93/sh/nvtree.c index 9fb371eb9..0bd0eb019 100644 --- a/src/cmd/ksh93/sh/nvtree.c +++ b/src/cmd/ksh93/sh/nvtree.c @@ -257,7 +257,7 @@ static Namval_t *nextnode(struct nvdir *dp) { if(dp->nextnode) return((*dp->nextnode)(dp->hp,dp->root,dp->fun)); - if(dp->len && memcmp(dp->data, dp->hp->nvname, dp->len)) + if(dp->len && strncmp(dp->data, dp->hp->nvname, dp->len)) return(0); return((Namval_t*)dtnext(dp->root,dp->hp)); } @@ -288,7 +288,7 @@ char *nv_dirnext(void *dir) dp->hp = (*dp->nextnode)(np,(Dt_t*)0,dp->fun); } sh.last_table = last_table; - if(!dp->len || memcmp(cp,dp->data,dp->len)==0) + if(!dp->len || strncmp(cp,dp->data,dp->len)==0) { if((nfp=nextdisc(np)) && (nfp->disc->getval||nfp->disc->getnum) && nv_isvtree(np) && strcmp(cp,dp->data)) nfp = 0; diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c index 3039b9795..2d555316c 100644 --- a/src/cmd/ksh93/sh/subshell.c +++ b/src/cmd/ksh93/sh/subshell.c @@ -327,7 +327,7 @@ Namval_t *sh_assignok(register Namval_t *np,int add) { walk = root->walk?root->walk:root; mpnext = dtnext(root,mp); - if(memcmp(name,mp->nvname,len) || mp->nvname[len]!='.') + if(strncmp(name,mp->nvname,len) || mp->nvname[len]!='.') break; nv_delete(mp,walk,NV_NOFREE); *((Namval_t**)mp) = lp->child; diff --git a/src/lib/libast/features/map.c b/src/lib/libast/features/map.c index 660d4d5cb..a9743ead6 100644 --- a/src/lib/libast/features/map.c +++ b/src/lib/libast/features/map.c @@ -137,10 +137,13 @@ main() printf("#undef getwd\n"); printf("#define getwd _ast_getwd\n"); printf("extern char* getwd(char*);\n"); +#endif + /* use the libast glob functions rather than the native versions */ printf("#undef glob\n"); printf("#define glob _ast_glob\n"); printf("#undef globfree\n"); printf("#define globfree _ast_globfree\n"); +#if _map_libc printf("#undef memdup\n"); printf("#define memdup _ast_memdup\n"); printf("#undef memfatal\n"); diff --git a/src/lib/libast/misc/glob.c b/src/lib/libast/misc/glob.c index f6ae924f0..87e0b8367 100644 --- a/src/lib/libast/misc/glob.c +++ b/src/lib/libast/misc/glob.c @@ -587,7 +587,7 @@ skip: } int -glob(const char* pattern, int flags, int (*errfn)(const char*, int), register glob_t* gp) +_ast_glob(const char* pattern, int flags, int (*errfn)(const char*, int), register glob_t* gp) { register globlist_t* ap; register char* pat; @@ -831,7 +831,7 @@ glob(const char* pattern, int flags, int (*errfn)(const char*, int), register gl } void -globfree(glob_t* gp) +_ast_globfree(glob_t* gp) { if ((gp->gl_flags & GLOB_MAGIC) == GLOB_MAGIC) {