From e230ae7b68814a2ea560e5138188a58128e34417 Mon Sep 17 00:00:00 2001 From: Ruediger Meier Date: Tue, 27 Jun 2017 20:32:52 +0200 Subject: [PATCH 1/8] lib/path: fix crash, pathbuf overflow Before: $ lscpu -s "$(tr '\0' 'x' < /dev/zero | head -c 10000)" Segmentation fault (core dumped) After: $ lscpu -s "$(tr '\0' 'x' < /dev/zero | head -c 10000)" lscpu: invalid argument to --sysroot: File name too long Signed-off-by: Ruediger Meier --- include/path.h | 6 +++++- lib/path.c | 14 ++++++++++---- sys-utils/lscpu.c | 3 ++- sys-utils/lsmem.c | 3 ++- 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/include/path.h b/include/path.h index 45da692f8..11c336759 100644 --- a/include/path.h +++ b/include/path.h @@ -27,7 +27,11 @@ extern cpu_set_t *path_read_cpuset(int, const char *path, ...) __attribute__ ((__format__ (__printf__, 2, 3))); extern cpu_set_t *path_read_cpulist(int, const char *path, ...) __attribute__ ((__format__ (__printf__, 2, 3))); -extern void path_set_prefix(const char *); + +/* Returns: 0 on success, sets errno on error. */ +extern int path_set_prefix(const char *) + __attribute__((warn_unused_result)); + #endif /* HAVE_CPU_SET_T */ #endif /* UTIL_LINUX_PATH_H */ diff --git a/lib/path.c b/lib/path.c index 1a623bc6d..eaa6d881c 100644 --- a/lib/path.c +++ b/lib/path.c @@ -244,12 +244,18 @@ path_read_cpulist(int maxcpus, const char *path, ...) return set; } -void +int path_set_prefix(const char *prefix) { - prefixlen = strlen(prefix); - strncpy(pathbuf, prefix, sizeof(pathbuf)); - pathbuf[sizeof(pathbuf) - 1] = '\0'; + size_t len = strlen(prefix); + + if (len >= sizeof(pathbuf) - 1) { + errno = ENAMETOOLONG; + return -1; + } + prefixlen = len; + strcpy(pathbuf, prefix); + return 0; } #endif /* HAVE_CPU_SET_T */ diff --git a/sys-utils/lscpu.c b/sys-utils/lscpu.c index 424b9de0e..f6e47277e 100644 --- a/sys-utils/lscpu.c +++ b/sys-utils/lscpu.c @@ -2148,7 +2148,8 @@ int main(int argc, char *argv[]) mod->mode = c == 'p' ? OUTPUT_PARSABLE : OUTPUT_READABLE; break; case 's': - path_set_prefix(optarg); + if(path_set_prefix(optarg)) + err(EXIT_FAILURE, _("invalid argument to %s"), "--sysroot"); mod->system = SYSTEM_SNAPSHOT; break; case 'x': diff --git a/sys-utils/lsmem.c b/sys-utils/lsmem.c index 04e7d20be..e1ee5a5ce 100644 --- a/sys-utils/lsmem.c +++ b/sys-utils/lsmem.c @@ -470,7 +470,8 @@ int main(int argc, char **argv) lsmem->want_summary = 0; break; case 's': - path_set_prefix(optarg); + if(path_set_prefix(optarg)) + err(EXIT_FAILURE, _("invalid argument to %s"), "--sysroot"); break; case 'V': printf(UTIL_LINUX_VERSION); From f567220b716ee096cf6c094ab27dcdabaaa192f6 Mon Sep 17 00:00:00 2001 From: Ruediger Meier Date: Tue, 27 Jun 2017 20:33:08 +0200 Subject: [PATCH 2/8] lib/path: add error handling to path_vcreate() Do not operate on truncated/random paths. Note, path_strdup() can now really return NULL, to be handled in next commit. Signed-off-by: Ruediger Meier --- lib/path.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/lib/path.c b/lib/path.c index eaa6d881c..3c587e433 100644 --- a/lib/path.c +++ b/lib/path.c @@ -38,11 +38,15 @@ static char pathbuf[PATH_MAX]; static const char * path_vcreate(const char *path, va_list ap) { - if (prefixlen) - vsnprintf(pathbuf + prefixlen, - sizeof(pathbuf) - prefixlen, path, ap); - else - vsnprintf(pathbuf, sizeof(pathbuf), path, ap); + int rc = vsnprintf( + pathbuf + prefixlen, sizeof(pathbuf) - prefixlen, path, ap); + + if (rc < 0) + return NULL; + if ((size_t)rc >= sizeof(pathbuf)) { + errno = ENAMETOOLONG; + return NULL; + } return pathbuf; } @@ -64,11 +68,18 @@ path_vfopen(const char *mode, int exit_on_error, const char *path, va_list ap) { FILE *f; const char *p = path_vcreate(path, ap); + if (!p) + goto err; f = fopen(p, mode); - if (!f && exit_on_error) - err(EXIT_FAILURE, _("cannot open %s"), p); + if (!f) + goto err; + return f; +err: + if (exit_on_error) + err(EXIT_FAILURE, _("cannot open %s"), p ? p : "path"); + return NULL; } static int @@ -76,11 +87,16 @@ path_vopen(int flags, const char *path, va_list ap) { int fd; const char *p = path_vcreate(path, ap); + if (!p) + goto err; fd = open(p, flags); if (fd == -1) - err(EXIT_FAILURE, _("cannot open %s"), p); + goto err; + return fd; +err: + err(EXIT_FAILURE, _("cannot open %s"), p ? p : "path"); } FILE * @@ -181,7 +197,7 @@ path_exist(const char *path, ...) p = path_vcreate(path, ap); va_end(ap); - return access(p, F_OK) == 0; + return p && access(p, F_OK) == 0; } #ifdef HAVE_CPU_SET_T From 81435af3be9de47fd74dff0c5e0a6add7c66ae9b Mon Sep 17 00:00:00 2001 From: Ruediger Meier Date: Tue, 27 Jun 2017 20:33:18 +0200 Subject: [PATCH 3/8] lsmem: fix, using freed memory Simply avoiding strdup(). Error handling improved. This was the Clang Analyzer warning: Memory Error, Use-after-free sys-utils/lsmem.c:259:3: warning: Use of memory after it is freed err(EXIT_FAILURE, _("Failed to open %s"), path); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Signed-off-by: Ruediger Meier --- include/path.h | 5 ++++- lib/path.c | 6 +++--- sys-utils/lscpu.c | 6 +++--- sys-utils/lsmem.c | 19 ++++++++++--------- 4 files changed, 20 insertions(+), 16 deletions(-) diff --git a/include/path.h b/include/path.h index 11c336759..ae36d7f4c 100644 --- a/include/path.h +++ b/include/path.h @@ -4,8 +4,11 @@ #include #include -extern char *path_strdup(const char *path, ...) +/* Returns a pointer to a static buffer which may be destroyed by any later +path_* function call. NULL means error and errno will be set. */ +extern const char *path_get(const char *path, ...) __attribute__ ((__format__ (__printf__, 1, 2))); + extern FILE *path_fopen(const char *mode, int exit_on_err, const char *path, ...) __attribute__ ((__format__ (__printf__, 3, 4))); extern void path_read_str(char *result, size_t len, const char *path, ...) diff --git a/lib/path.c b/lib/path.c index 3c587e433..79c1e7a68 100644 --- a/lib/path.c +++ b/lib/path.c @@ -50,8 +50,8 @@ path_vcreate(const char *path, va_list ap) return pathbuf; } -char * -path_strdup(const char *path, ...) +const char * +path_get(const char *path, ...) { const char *p; va_list ap; @@ -60,7 +60,7 @@ path_strdup(const char *path, ...) p = path_vcreate(path, ap); va_end(ap); - return p ? strdup(p) : NULL; + return p; } static FILE * diff --git a/sys-utils/lscpu.c b/sys-utils/lscpu.c index f6e47277e..83f3a7d27 100644 --- a/sys-utils/lscpu.c +++ b/sys-utils/lscpu.c @@ -1430,12 +1430,12 @@ read_nodes(struct lscpu_desc *desc) int i = 0; DIR *dir; struct dirent *d; - char *path; + const char *path; /* number of NUMA node */ - path = path_strdup(_PATH_SYS_NODE); + if (!(path = path_get(_PATH_SYS_NODE))) + return; dir = opendir(path); - free(path); while (dir && (d = readdir(dir))) { if (is_node_dirent(d)) diff --git a/sys-utils/lsmem.c b/sys-utils/lsmem.c index e1ee5a5ce..4db678966 100644 --- a/sys-utils/lsmem.c +++ b/sys-utils/lsmem.c @@ -248,15 +248,14 @@ static void print_summary(struct lsmem *lsmem) static int memory_block_get_node(char *name) { struct dirent *de; - char *path; + const char *path; DIR *dir; int node; - path = path_strdup(_PATH_SYS_MEMORY"/%s", name); - dir = opendir(path); - free(path); - if (!dir) - err(EXIT_FAILURE, _("Failed to open %s"), path); + path = path_get(_PATH_SYS_MEMORY"/%s", name); + if (!path || !(dir= opendir(path))) + err(EXIT_FAILURE, _("Failed to open %s"), path ? path : name); + node = -1; while ((de = readdir(dir)) != NULL) { if (strncmp("node", de->d_name, 4)) @@ -348,14 +347,16 @@ static int memory_block_filter(const struct dirent *de) static void read_basic_info(struct lsmem *lsmem) { - char *dir; + const char *dir; if (!path_exist(_PATH_SYS_MEMORY_BLOCK_SIZE)) errx(EXIT_FAILURE, _("This system does not support memory blocks")); - dir = path_strdup(_PATH_SYS_MEMORY); + dir = path_get(_PATH_SYS_MEMORY); + if (!dir) + err(EXIT_FAILURE, _("Failed to read %s"), _PATH_SYS_MEMORY); + lsmem->ndirs = scandir(dir, &lsmem->dirs, memory_block_filter, versionsort); - free(dir); if (lsmem->ndirs <= 0) err(EXIT_FAILURE, _("Failed to read %s"), _PATH_SYS_MEMORY); From a25fb9e8ec534589bcc9f2fb45b4dfdc4d1084f7 Mon Sep 17 00:00:00 2001 From: Ruediger Meier Date: Tue, 27 Jun 2017 20:33:28 +0200 Subject: [PATCH 4/8] lscpu: make clang analyzer happy Let read_nodes() work on uninitialized structs to silence these two warnings: CC sys-utils/lscpu-lscpu.o warning: Path diagnostic report is not generated. Current output format does not support diagnostics that cross file boundaries. Refer to --analyzer-output for valid output formats In file included from sys-utils/lscpu.c:63: ./include/xalloc.h:32:21: warning: Call to 'malloc' has an allocation size of 0 bytes void *ret = malloc(size); ^~~~~~~~~~~~ sys-utils/lscpu.c:1468:23: warning: Function call argument is an uninitialized value desc->nodemaps[i] = path_read_cpuset(maxcpus, ^~~~~~~~~~~~~~~~~~~~~~~~~ 2 warnings generated. Signed-off-by: Ruediger Meier --- sys-utils/lscpu.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/sys-utils/lscpu.c b/sys-utils/lscpu.c index 83f3a7d27..852711e74 100644 --- a/sys-utils/lscpu.c +++ b/sys-utils/lscpu.c @@ -1432,35 +1432,34 @@ read_nodes(struct lscpu_desc *desc) struct dirent *d; const char *path; + desc->nnodes = 0; + /* number of NUMA node */ if (!(path = path_get(_PATH_SYS_NODE))) return; - dir = opendir(path); - - while (dir && (d = readdir(dir))) { + if (!(dir = opendir(path))) + return; + while ((d = readdir(dir))) { if (is_node_dirent(d)) desc->nnodes++; } if (!desc->nnodes) { - if (dir) - closedir(dir); + closedir(dir); return; } desc->nodemaps = xcalloc(desc->nnodes, sizeof(cpu_set_t *)); desc->idx2nodenum = xmalloc(desc->nnodes * sizeof(int)); - if (dir) { - rewinddir(dir); - while ((d = readdir(dir)) && i < desc->nnodes) { - if (is_node_dirent(d)) - desc->idx2nodenum[i++] = strtol_or_err(((d->d_name) + 4), - _("Failed to extract the node number")); - } - closedir(dir); - qsort(desc->idx2nodenum, desc->nnodes, sizeof(int), nodecmp); + rewinddir(dir); + while ((d = readdir(dir)) && i < desc->nnodes) { + if (is_node_dirent(d)) + desc->idx2nodenum[i++] = strtol_or_err(((d->d_name) + 4), + _("Failed to extract the node number")); } + closedir(dir); + qsort(desc->idx2nodenum, desc->nnodes, sizeof(int), nodecmp); /* information about how nodes share different CPUs */ for (i = 0; i < desc->nnodes; i++) From c3ae7854337be9f49540f4b0fb465ee4bd77a5ee Mon Sep 17 00:00:00 2001 From: Ruediger Meier Date: Sat, 17 Jun 2017 22:40:41 +0200 Subject: [PATCH 5/8] misc: avoid some dead initialization warnings Clang analyzer warnings: Dead store, Dead initialization: lib/mbsedit.c:154:8: warning: Value stored to 'in' during its initialization is never read char *in = (char *) &c; ^~ ~~~~~~~~~~~ misc-utils/findmnt-verify.c:129:14: warning: Value stored to 'cn' during its initialization is never read const char *cn = tgt; ^~ ~~~ Dead store, Dead increment: sys-utils/hwclock.c:1461:2: warning: Value stored to 'argv' is never read argv += optind; ^ ~~~~~~ Signed-off-by: Ruediger Meier --- lib/mbsedit.c | 3 ++- misc-utils/findmnt-verify.c | 3 +-- sys-utils/hwclock.c | 5 +---- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/mbsedit.c b/lib/mbsedit.c index d464358fc..e028c496d 100644 --- a/lib/mbsedit.c +++ b/lib/mbsedit.c @@ -151,7 +151,7 @@ static size_t mbs_insert(char *str, wint_t c, size_t *ncells) { /* all in bytes! */ size_t n = 1, bytes; - char *in = (char *) &c; + char *in; #ifdef HAVE_WIDECHAR wchar_t wc = (wchar_t) c; @@ -162,6 +162,7 @@ static size_t mbs_insert(char *str, wint_t c, size_t *ncells) in = in_buf; #else *ncells = 1; + in = (char *) &c; #endif bytes = strlen(str); diff --git a/misc-utils/findmnt-verify.c b/misc-utils/findmnt-verify.c index b32901d66..1cc62def9 100644 --- a/misc-utils/findmnt-verify.c +++ b/misc-utils/findmnt-verify.c @@ -126,14 +126,13 @@ done: static int verify_target(struct verify_context *vfy) { const char *tgt = mnt_fs_get_target(vfy->fs); - const char *cn = tgt; struct stat sb; if (!tgt) return verify_err(vfy, _("undefined target (fs_file)")); if (!(flags & FL_NOCACHE)) { - cn = mnt_resolve_target(tgt, cache); + const char *cn = mnt_resolve_target(tgt, cache); if (!cn) return -ENOMEM; if (strcmp(cn, tgt) != 0) diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c index 98ee5bef5..2f2c03a6f 100644 --- a/sys-utils/hwclock.c +++ b/sys-utils/hwclock.c @@ -1457,10 +1457,7 @@ int main(int argc, char **argv) } } - argc -= optind; - argv += optind; - - if (argc > 0) { + if (argc > optind) { warnx(_("%d too many arguments given"), argc); errtryhelp(EXIT_FAILURE); } From 66b7469960bc97c05a22498d11117ef2d60b54cd Mon Sep 17 00:00:00 2001 From: Ruediger Meier Date: Tue, 27 Jun 2017 20:34:29 +0200 Subject: [PATCH 6/8] tools: add segfault detection for checkusage.sh Signed-off-by: Ruediger Meier --- tools/checkusage.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/checkusage.sh b/tools/checkusage.sh index 6cde8fbf6..69d69fde1 100755 --- a/tools/checkusage.sh +++ b/tools/checkusage.sh @@ -61,8 +61,8 @@ function exec_option { # hardcoded ... nologin should always return false if test "$cmdb" = "nologin" && test "$opt" = "--help" -o "$opt" = "--version"; then - if test "$ret" = "0"; then - echo "$cmdb, $opt, should return false" + if test "$ret" -eq 0 -o "$ret" -ge 128; then + echo "$cmdb, $opt, should return false: $ret" fi ret=0 fi @@ -123,6 +123,8 @@ function check_unknownopt { if test $ret = 0; then echo "$cb: $opt, returns no error" + elif test $ret -ge 128; then + echo "$cb: $opt, abnormal exit: $ret" fi if test -n "$out"; then echo "$cb: $opt, non-empty stdout" From a4dc44337dcf81357c14ca148f947a6de7cc7894 Mon Sep 17 00:00:00 2001 From: Ruediger Meier Date: Tue, 27 Jun 2017 20:36:11 +0200 Subject: [PATCH 7/8] setpriv: align --help This was forgotton during my last cleanup because the build was auto-disabled on my system. Signed-off-by: Ruediger Meier --- sys-utils/setpriv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sys-utils/setpriv.c b/sys-utils/setpriv.c index 41a865ff1..1e5c0b499 100644 --- a/sys-utils/setpriv.c +++ b/sys-utils/setpriv.c @@ -139,7 +139,7 @@ static void __attribute__((__noreturn__)) usage(void) fputs(_(" --apparmor-profile set AppArmor profile\n"), out); fputs(USAGE_SEPARATOR, out); - print_usage_help_options(16); + print_usage_help_options(29); fputs(USAGE_SEPARATOR, out); fputs(_(" This tool can be dangerous. Read the manpage, and be careful.\n"), out); fprintf(out, USAGE_MAN_TAIL("setpriv(1)")); From 3eeaef995bced0b44c23ae1dcd6c808667667986 Mon Sep 17 00:00:00 2001 From: Ruediger Meier Date: Wed, 28 Jun 2017 18:37:44 +0200 Subject: [PATCH 8/8] hwclock: don't ifdef printf arguments This may fails if printf() is macro, introduced in cc7cb070. clang compiler warnings: CC sys-utils/hwclock.o ../sys-utils/hwclock.c:1228:2: warning: embedding a directive within macro arguments has undefined behavior [-Wembedded-directive] #ifdef __linux__ ^ ../sys-utils/hwclock.c:1230:2: warning: embedding a directive within macro arguments has undefined behavior [-Wembedded-directive] #endif ^ 2 warnings generated. CC: J William Piggott Signed-off-by: Ruediger Meier --- sys-utils/hwclock.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c index 2f2c03a6f..a0a48dd13 100644 --- a/sys-utils/hwclock.c +++ b/sys-utils/hwclock.c @@ -1225,10 +1225,11 @@ usage(const struct hwclock_control *ctl) fputs(USAGE_OPTIONS, out); fputs(_(" -u, --utc inform hwclock the RTC timescale is UTC\n"), out); fputs(_(" -l, --localtime inform hwclock the RTC timescale is Local\n"), out); - fprintf(out, _( #ifdef __linux__ - " -f, --rtc use an alternate file to %1$s\n" + printf(_( + " -f, --rtc use an alternate file to %1$s\n"), _PATH_RTC_DEV); #endif + printf(_( " --directisa use the ISA bus instead of %1$s access\n"), _PATH_RTC_DEV); fputs(_(" --date