From b08cb7352e8256d7b2f4ea282a13939e91948c58 Mon Sep 17 00:00:00 2001 From: Michael McConville Date: Sat, 5 Mar 2016 23:23:29 -0500 Subject: [PATCH 1/2] Misc. OpenBSD tuneup and improvement Including: o set *basenameEnd even in error cases (FreeBSD probably needs this) o use kvm_openfiles(3) rather than kvm_open(3) so that we can report errors (as with FreeBSD) o sanify the process argument list creation by using strlcat(3) o drop the pageSizeKb variable and use the PAGE_SIZE_KB macro directly, as the page size can't change anyway o clean up a few macros, add MINIMUM() and MAXIMUM() (should be mirrored to FreeBSD) o fix some syntax o add some useful comments --- openbsd/OpenBSDProcessList.c | 97 ++++++++++++++++++++---------------- openbsd/OpenBSDProcessList.h | 14 +++++- 2 files changed, 67 insertions(+), 44 deletions(-) diff --git a/openbsd/OpenBSDProcessList.c b/openbsd/OpenBSDProcessList.c index 97e40cb8..b1060367 100644 --- a/openbsd/OpenBSDProcessList.c +++ b/openbsd/OpenBSDProcessList.c @@ -12,14 +12,15 @@ in the source distribution for its full text. #include #include -#include -#include #include #include #include #include #include +#include +#include #include +#include #include #include @@ -42,22 +43,37 @@ typedef struct OpenBSDProcessList_ { }*/ -#ifndef CLAMP -#define CLAMP(x,low,high) (((x)>(high))?(high):(((x)<(low))?(low):(x))) +/* + * avoid relying on or conflicting with MIN() and MAX() in sys/param.h + */ +#ifndef MINIMUM +#define MINIMUM(x, y) ((x) > (y) ? (y) : (x)) +#endif + +#ifndef MAXIMUM +#define MAXIMUM(x, y) ((x) > (y) ? (x) : (y)) +#endif + +#ifndef CLAMP +#define CLAMP(x, low, high) (((x) > (high)) ? (high) : MAXIMUM(x, low)) #endif -static int pageSizeKb; static long fscale; ProcessList* ProcessList_new(UsersTable* usersTable, Hashtable* pidWhiteList, uid_t userId) { int mib[] = { CTL_HW, HW_NCPU }; int fmib[] = { CTL_KERN, KERN_FSCALE }; int i, e; - OpenBSDProcessList* opl = xCalloc(1, sizeof(OpenBSDProcessList)); - ProcessList* pl = (ProcessList*) opl; - size_t size = sizeof(pl->cpuCount); + OpenBSDProcessList* opl; + ProcessList* pl; + size_t size; + char errbuf[_POSIX2_LINE_MAX]; + opl = xCalloc(1, sizeof(OpenBSDProcessList)); + pl = (ProcessList*) opl; + size = sizeof(pl->cpuCount); ProcessList_init(pl, Class(OpenBSDProcess), usersTable, pidWhiteList, userId); + e = sysctl(mib, 2, &pl->cpuCount, &size, NULL, 0); if (e == -1 || pl->cpuCount < 1) { pl->cpuCount = 1; @@ -65,26 +81,29 @@ ProcessList* ProcessList_new(UsersTable* usersTable, Hashtable* pidWhiteList, ui opl->cpus = xRealloc(opl->cpus, pl->cpuCount * sizeof(CPUData)); size = sizeof(fscale); - if (sysctl(fmib, 2, &fscale, &size, NULL, 0) < 0) + if (sysctl(fmib, 2, &fscale, &size, NULL, 0) < 0) { err(1, "fscale sysctl call failed"); + } for (i = 0; i < pl->cpuCount; i++) { opl->cpus[i].totalTime = 1; opl->cpus[i].totalPeriod = 1; } - pageSizeKb = PAGE_SIZE_KB; - - // XXX: last arg should eventually be an errbuf - opl->kd = kvm_open(NULL, NULL, NULL, KVM_NO_FILES, NULL); - assert(opl->kd); + opl->kd = kvm_openfiles(NULL, NULL, NULL, KVM_NO_FILES, errbuf); + if (opl->kd == NULL) { + errx(1, "kvm_open: %s", errbuf); + } return pl; } void ProcessList_delete(ProcessList* this) { const OpenBSDProcessList* opl = (OpenBSDProcessList*) this; - if (opl->kd) kvm_close(opl->kd); + + if (opl->kd) { + kvm_close(opl->kd); + } free(opl->cpus); @@ -101,9 +120,8 @@ static inline void OpenBSDProcessList_scanMemoryInfo(ProcessList* pl) { err(1, "uvmexp sysctl call failed"); } - //kb_pagesize = uvmexp.pagesize / 1024; - pl->usedMem = uvmexp.active * pageSizeKb; - pl->totalMem = uvmexp.npages * pageSizeKb; + pl->usedMem = uvmexp.active * PAGE_SIZE_KB; + pl->totalMem = uvmexp.npages * PAGE_SIZE_KB; /* const OpenBSDProcessList* opl = (OpenBSDProcessList*) pl; @@ -112,10 +130,10 @@ static inline void OpenBSDProcessList_scanMemoryInfo(ProcessList* pl) { sysctl(MIB_hw_physmem, 2, &(pl->totalMem), &len, NULL, 0); pl->totalMem /= 1024; sysctl(MIB_vm_stats_vm_v_wire_count, 4, &(pl->usedMem), &len, NULL, 0); - pl->usedMem *= pageSizeKb; + pl->usedMem *= PAGE_SIZE_KB; pl->freeMem = pl->totalMem - pl->usedMem; sysctl(MIB_vm_stats_vm_v_cache_count, 4, &(pl->cachedMem), &len, NULL, 0); - pl->cachedMem *= pageSizeKb; + pl->cachedMem *= PAGE_SIZE_KB; struct kvm_swap swap[16]; int nswap = kvm_getswapinfo(opl->kd, swap, sizeof(swap)/sizeof(swap[0]), 0); @@ -125,8 +143,8 @@ static inline void OpenBSDProcessList_scanMemoryInfo(ProcessList* pl) { pl->totalSwap += swap[i].ksw_total; pl->usedSwap += swap[i].ksw_used; } - pl->totalSwap *= pageSizeKb; - pl->usedSwap *= pageSizeKb; + pl->totalSwap *= PAGE_SIZE_KB; + pl->usedSwap *= PAGE_SIZE_KB; pl->sharedMem = 0; // currently unused pl->buffersMem = 0; // not exposed to userspace @@ -134,40 +152,35 @@ static inline void OpenBSDProcessList_scanMemoryInfo(ProcessList* pl) { } char *OpenBSDProcessList_readProcessName(kvm_t* kd, struct kinfo_proc* kproc, int* basenameEnd) { - char *s, *buf, **arg; - size_t cpsz, len = 0, n; + char *s, **arg; + size_t len = 0, n; int i; /* - * We attempt to fall back to just the command name (argv[0]) if we - * fail to construct the full command at any point. + * Like OpenBSD's top(1), we try to fall back to the command name + * (argv[0]) if we fail to construct the full command. */ arg = kvm_getargv(kd, kproc, 500); if (arg == NULL) { - if ((s = xStrdup(kproc->p_comm)) == NULL) { - err(1, NULL); - } - return s; + *basenameEnd = strlen(kproc->p_comm); + return xStrdup(kproc->p_comm); } for (i = 0; arg[i] != NULL; i++) { - len += strlen(arg[i]) + 1; + len += strlen(arg[i]) + 1; /* room for arg and trailing space or NUL */ } - if ((buf = s = xMalloc(len)) == NULL) { - if ((s = xStrdup(kproc->p_comm)) == NULL) { - err(1, NULL); - } - return s; + /* don't use xMalloc here - we want to handle huge argv's gracefully */ + if ((s = xCalloc(len, 1)) == NULL) { + *basenameEnd = strlen(kproc->p_comm); + return xStrdup(kproc->p_comm); } for (i = 0; arg[i] != NULL; i++) { - n = strlcpy(buf, arg[i], (s + len) - buf); - buf += n; + n = strlcat(s, arg[i], len); if (i == 0) { - *basenameEnd = n; + /* TODO: rename all basenameEnd to basenameLen, make size_t */ + *basenameEnd = MINIMUM(n, len-1); } - *buf = ' '; - buf++; + strlcat(s, " ", len); } - *(buf - 1) = '\0'; return s; } diff --git a/openbsd/OpenBSDProcessList.h b/openbsd/OpenBSDProcessList.h index 192fb34f..ba9e6d14 100644 --- a/openbsd/OpenBSDProcessList.h +++ b/openbsd/OpenBSDProcessList.h @@ -27,10 +27,20 @@ typedef struct OpenBSDProcessList_ { } OpenBSDProcessList; -#ifndef CLAMP -#define CLAMP(x,low,high) (((x)>(high))?(high):(((x)<(low))?(low):(x))) +/* + * avoid relying on or conflicting with MIN() and MAX() in sys/param.h + */ +#ifndef MINIMUM +#define MINIMUM(x, y) ((x) > (y) ? (y) : (x)) #endif +#ifndef MAXIMUM +#define MAXIMUM(x, y) ((x) > (y) ? (x) : (y)) +#endif + +#ifndef CLAMP +#define CLAMP(x, low, high) (((x) > (high)) ? (high) : MAXIMUM(x, low)) +#endif ProcessList* ProcessList_new(UsersTable* usersTable, Hashtable* pidWhiteList, uid_t userId); From 4b780a3499edc795b8edddac2e3b91792d7721d1 Mon Sep 17 00:00:00 2001 From: Michael McConville Date: Sat, 5 Mar 2016 23:38:12 -0500 Subject: [PATCH 2/2] A few more OpenBSD fixes Namely: o use malloc where an xCalloc slipped in o safeguard against an empty arg list - I don't think it's possible, but it would be potentially exploitable o we need to initialize the arg string to an empty string because we no longer use strlcpy(3) o annotate a tricky use of strlcpy(3)'s truncation --- openbsd/OpenBSDProcessList.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/openbsd/OpenBSDProcessList.c b/openbsd/OpenBSDProcessList.c index b1060367..548a1eea 100644 --- a/openbsd/OpenBSDProcessList.c +++ b/openbsd/OpenBSDProcessList.c @@ -161,7 +161,7 @@ char *OpenBSDProcessList_readProcessName(kvm_t* kd, struct kinfo_proc* kproc, in * (argv[0]) if we fail to construct the full command. */ arg = kvm_getargv(kd, kproc, 500); - if (arg == NULL) { + if (arg == NULL || *arg == NULL) { *basenameEnd = strlen(kproc->p_comm); return xStrdup(kproc->p_comm); } @@ -169,18 +169,23 @@ char *OpenBSDProcessList_readProcessName(kvm_t* kd, struct kinfo_proc* kproc, in len += strlen(arg[i]) + 1; /* room for arg and trailing space or NUL */ } /* don't use xMalloc here - we want to handle huge argv's gracefully */ - if ((s = xCalloc(len, 1)) == NULL) { + if ((s = malloc(len)) == NULL) { *basenameEnd = strlen(kproc->p_comm); return xStrdup(kproc->p_comm); } + + *s = '\0'; + for (i = 0; arg[i] != NULL; i++) { n = strlcat(s, arg[i], len); if (i == 0) { /* TODO: rename all basenameEnd to basenameLen, make size_t */ *basenameEnd = MINIMUM(n, len-1); } + /* the trailing space should get truncated anyway */ strlcat(s, " ", len); } + return s; }