Hi Neel,
Neel Chauhan wrote on Tue, Aug 29, 2023 at 02:48:54PM -0700:
> I have noticed a bug. When running "sysctl ." or "sysctl kern." without
> my other patch, I get an extra space between "name" and "in":
>> sysctl: top level name in . is invalid
Technically, that message is correct. The name being complained about
is a zero length string, and of course there are space characters
marking a word boundary before and after it, resulting in two adjacent
space characters around a zero-length word. But i see how some users
might find that confusing.
If people want that improved, see below for a patch that actually
improves the diagnostic message, resulting in:
$ sysctl kern.=test
sysctl: empty second level name in kern.
$ sysctl kern.
sysctl: empty second level name in kern.
$ sysctl .=test
sysctl: empty top level name in .
$ sysctl .
sysctl: empty top level name in .
$ sysctl =test
sysctl: empty top level name in
$ sysctl ''
sysctl: empty top level name in
Note this only happens for pathological names that are completely
empty or end in a dot, so i'm not sure it is very important.
Besides, there are other ways to provoke highly confusing error
messages by providing insane arguments. Consider, for example:
$ sysctl ' '
sysctl: top level name in is invalid
$ sysctl 'kern.ostype'
kern.ostype=OpenBSD
$ sysctl 'kern.os^Atype'
sysctl: second level name ostype in kern.ostype is invalid
$ sysctl 'kern.os^Gtype'
sysctl: second level name ostype in kern.ostype is invalid
$ sysctl '^H'
sysctl: top level name in is invalid
I'm not convinced accurately diagnosing every possible kind
of insane input is a priority.
Regarding the patch, note that the condition *bufp == NULL that
is being tested cannot actually happen because all callers test
that very condition right before calling findname(), and they
have to do that because they all want to call listall() in that
case.
Strangely, the property that *bufp cannot be NULL was already present
in McKusick's very first version of the program dated "5.1 93/03/30
23:48:06", about 3 months before the initial 4.4BSD release. I have
no idea why he considered testing that useful. Maybe he fell pray
to the confusing design of the strsep(3) API and actually wanted to
test for *name == '\0' but inadvertently wrote name == NULL and no
one ever noticed?
Maybe one argument in favor of my patch is that it makes the
source code of findname() easier to read in addition to making
the error message less confusing in your corner case.
Lightly tested so far.
Yours,
Ingo
Index: sysctl.c
===================================================================
RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
retrieving revision 1.259
diff -u -p -r1.259 sysctl.c
--- sysctl.c 17 May 2023 22:12:51 -0000 1.259
+++ sysctl.c 29 Aug 2023 23:31:09 -0000
@@ -2948,8 +2948,13 @@ findname(char *string, char *level, char
char *name;
int i;
- if (namelist->list == 0 || (name = strsep(bufp, ".")) == NULL) {
+ if (namelist->list == 0) {
warnx("%s: incomplete specification", string);
+ return (-1);
+ }
+ name = strsep(bufp, ".");
+ if (*name == '\0') {
+ warnx("empty %s level name in %s", level, string);
return (-1);
}
for (i = 0; i < namelist->size; i++)
