From 39d4d147d7ab8c67697974ffba249bab793a7104 Mon Sep 17 00:00:00 2001 From: Craig Small Date: Sat, 8 Jan 2022 07:49:13 +1100 Subject: [PATCH] library: Better PID file checks MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit This started off with fixing the compilier warning: proc/readproc.c: In function ‘simple_nextpid’: proc/readproc.c:1373:38: warning: ‘%s’ directive output may be truncated writing up to 255 bytes into a region of size 58 [-Wformat-truncation=] 1373 | snprintf(path, PROCPATHLEN, "/proc/%s", ent->d_name); | ^~ proc/readproc.c:1373:3: note: ‘snprintf’ output between 7 and 262 bytes into a destination of size 64 1373 | snprintf(path, PROCPATHLEN, "/proc/%s", ent->d_name); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ We know that ent->d_name will fit under 64 bytes because we check the name starts with a digit. The first change was simple and changed the printf to use tgid like the task function below it. Is everything under /proc that starts with a digit a directory with a PID only? Today, it is but there are no guarantees. The entire function works ok if every non-pid directory doesn't start with a number. We don't check for strtoul() having an issue nor if the for loop just falls off the end. The moment the kernel guys (or some module writer) think "/proc/12mykernelval" is a neat idea this function is in trouble. We won't get buffer overflow as we are using snprintf at least. This change now: We check if strtoul() actually came across a number Process the pid directory as a conditional branch Treat falling off the for loop as a not-found Signed-off-by: Craig Small --- proc/readproc.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/proc/readproc.c b/proc/readproc.c index 44203640..cc55cdfc 100644 --- a/proc/readproc.c +++ b/proc/readproc.c @@ -1361,17 +1361,22 @@ next_task: // This finds processes in /proc in the traditional way. // Return non-zero on success. static int simple_nextpid(PROCTAB *restrict const PT, proc_t *restrict const p) { - static __thread struct dirent *ent; /* dirent handle */ - char *restrict const path = PT->path; - for (;;) { - ent = readdir(PT->procfs); - if(!ent || !ent->d_name[0]) return 0; - if(*ent->d_name > '0' && *ent->d_name <= '9') break; - } - p->tgid = strtoul(ent->d_name, NULL, 10); - p->tid = p->tgid; - snprintf(path, PROCPATHLEN, "/proc/%s", ent->d_name); - return 1; + static __thread struct dirent *ent; /* dirent handle */ + char *restrict const path = PT->path; + for (;;) { + ent = readdir(PT->procfs); + if (!ent || !ent->d_name[0]) return 0; + if (*ent->d_name > '0' && *ent->d_name <= '9') { + errno = 0; + p->tgid = strtoul(ent->d_name, NULL, 10); + if (errno == 0) { + p->tid = p->tgid; + snprintf(path, PROCPATHLEN, "/proc/%d", p->tgid); + return 1; + } + } + } + return 0; } -- 2.40.0