Re: CVS commit: src/sbin/raidctl

[christos%astron.com@localhost (Christos Zoulas)]

In article <20220614080613.B4570FB1B%cvs.NetBSD.org@localhost>,
Robert Elz <source-changes-d%NetBSD.org@localhost> wrote:
>-=-=-=-=-=-
>
>Module Name:	src
>Committed By:	kre
>Date:		Tue Jun 14 08:06:13 UTC 2022
>
>Modified Files:
>	src/sbin/raidctl: rf_configure.c
>
>Log Message:
>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.

There is really no type consistency

raidframevar.h: char    maxOutstandingDiskReqs; /* # concurrent reqs to be sent to a
raidframevar.h: int maxOutstanding;   /* maxOutstanding disk requests */
rf_diskqueue.h: long    maxOutstanding; /* max # of I/Os that can be outstanding on a
rf_raid.h:      int maxOutstanding;   

Nevertheless why don't we make the char one unsigned char? It will not
be an ABI change and it will make it 255 for everyone.

christos