From b958b2dc5a698fcc7dedae42b4cd2d0ce94a6f2b Mon Sep 17 00:00:00 2001 From: kre Date: Tue, 14 Jun 2022 08:06:13 +0000 Subject: [PATCH] Fix some config file parsing. First, and what got me started on this set of cleanups, the queue length in the "queue" section (START queue) is limited to what will fit in a char without losing accuracy (I tried setting it to 200, rather than the more common (universal?) 100 and found that the value configured into the array was -56 instead. Why the value needs to be passed through a char variable I have no idea (it is an int in the filesystem raidframe headers) - but that's the way it is done, and changing it would be an ABI change I believe (and so need versioning to alter) and that isn't worth it for this (or not now, IMO). Instead check that the value in the char is the same value as was read from the config file, and complain if not. Those of you with unsigned chars will be able to have queue lengths up to 255, the rest of us are limited to 127. While looking at that, I noticed some code that obviously fails to understand that scanf("%s") will never return a string containing spaces, and proceeded to attempt to remove trailing spaces from the result ... amusingly, after having used the result for its intended purpose (non existent trailing spaces unremoved), after which that buffer was never used again. That code is now gone (but for now, just #if 0'd rather than actually deleted - it should be cleaned up sometime). Then I saw some other issues with how the config was parsed - a simple (unbounded) scanf("%s") into a buffer, which hypothetically might not be large enough (not a security issue really, raidctl has no special privs, and it isn't likely that root could easily be tricked into running it on a bogus config file - or not without looking first anyway, and a huge long string would rather stand out). Bound the string length to something reasonable, and assert() that the buffer is big enough to contain it. Lastly, in the event of one particular detected error in the config file, the code would write a warning, but then just go ahead and use the bad data (or nothing perhaps) anyway - a failure of logic flow (unlikely to have ever happened, everyone seems to simply copy the sample config from the man page, and make minor adjustments as needed). If any of these changes make any difference to anyone (except me with my attempt to make longer queues - for no particularly well thought out reason), I'd be very surprised. --- sbin/raidctl/rf_configure.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/sbin/raidctl/rf_configure.c b/sbin/raidctl/rf_configure.c index 11dd6fefae5a..dc7722bc05d6 100644 --- a/sbin/raidctl/rf_configure.c +++ b/sbin/raidctl/rf_configure.c @@ -1,4 +1,4 @@ -/* $NetBSD: rf_configure.c,v 1.35 2022/06/14 08:06:07 kre Exp $ */ +/* $NetBSD: rf_configure.c,v 1.36 2022/06/14 08:06:13 kre Exp $ */ /* * Copyright (c) 1995 Carnegie-Mellon University. @@ -49,7 +49,7 @@ #include #ifndef lint -__RCSID("$NetBSD: rf_configure.c,v 1.35 2022/06/14 08:06:07 kre Exp $"); +__RCSID("$NetBSD: rf_configure.c,v 1.36 2022/06/14 08:06:13 kre Exp $"); #endif @@ -59,6 +59,7 @@ __RCSID("$NetBSD: rf_configure.c,v 1.35 2022/06/14 08:06:07 kre Exp $"); #include #include #include +#include #include #include @@ -212,29 +213,45 @@ rf_MakeConfig(char *configname, RF_Config_t *cfgPtr) warnx("[No disk queue discipline specified in config file %s. " "Using %s.]", configname, cfgPtr->diskQueueType); } + else { /* * the queue specifier line contains two entries: 1st char of first * word specifies queue to be used 2nd word specifies max num reqs * that can be outstanding on the disk itself (typically 1) */ - if (sscanf(buf, "%s %d", buf1, &val) != 2) { + assert(64 < sizeof buf1); + if (sscanf(buf, "%64s %d", buf1, &val) != 2 || strlen(buf1) >= 60) { warnx("Can't determine queue type and/or max outstanding " - "reqs from line: %*s", (int)(sizeof(buf) - 1), buf); + "reqs from line: %.*s", (int)(sizeof(buf) - 1), buf); warnx("Using %s-%d", cfgPtr->diskQueueType, cfgPtr->maxOutstandingDiskReqs); } else { +#if 0 char *ch; +#endif memcpy(cfgPtr->diskQueueType, buf1, RF_MIN(sizeof(cfgPtr->diskQueueType), strlen(buf1) + 1)); + cfgPtr->diskQueueType[sizeof cfgPtr->diskQueueType - 1] = '\0'; + +#if 0 /* this indicates a lack of understanding of how scanf() works */ + /* it was also pointless as buf1 data was (b4) never used again */ for (ch = buf1; *ch; ch++) { if (*ch == ' ') { *ch = '\0'; break; } } +#endif cfgPtr->maxOutstandingDiskReqs = val; + if (cfgPtr->maxOutstandingDiskReqs != val) { + warnx("Queue length for %s out of range" + " [%d interp as %d], assuming 1", + buf1, val, cfgPtr->maxOutstandingDiskReqs); + cfgPtr->maxOutstandingDiskReqs = 1; + } } + } rewind(fp);