[src/trunk]: src/sbin/raidctl Fix some config file parsing.

[kre <kre%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/1a75b06e15d8
branches:  trunk
changeset: 366779:1a75b06e15d8
user:      kre <kre%NetBSD.org@localhost>
date:      Tue Jun 14 08:06:13 2022 +0000

description:
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.

diffstat:

 sbin/raidctl/rf_configure.c |  25 +++++++++++++++++++++----
 1 files changed, 21 insertions(+), 4 deletions(-)

diffs (74 lines):

diff -r 7c310cf5ccf2 -r 1a75b06e15d8 sbin/raidctl/rf_configure.c
--- a/sbin/raidctl/rf_configure.c       Tue Jun 14 08:06:07 2022 +0000
+++ b/sbin/raidctl/rf_configure.c       Tue Jun 14 08:06:13 2022 +0000
@@ -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 <sys/cdefs.h>
 
 #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 @@
 #include <strings.h>
 #include <err.h>
 #include <util.h>
+#include <assert.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 
@@ -212,29 +213,45 @@
                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);