Hi Neel,
Neel Chauhan wrote on Tue, Aug 29, 2023 at 02:36:57PM -0700:
> On 2023-08-29 14:23, Theo Buehler wrote:
>> Neel Chauhan:
>>> + if (string[strlen(string) - 1] == '.')
>>> + buf[strlen(string) - 1] = '\0';
>> Careful with out-of-bounds accesses. What if string is "" ? Probably
>> easiest to do "len = strlen(string);" and if (len > 0 && ...).
> Good catch!
> Does this look better:
Not much better, and my main complaint isn't even that storing the
return value of strlen(3) in an int variable is bad style.
Your patch is still incorrect in so far as with a command like
sysctl 'vm.malloc_conf=S>.'
your code would happily and silently remove the period from
the end of the value whereas in
sysctl vm.malloc_conf.=S
it would still complain in exactly the same way as it does now.
I'm not sure any of the sysctl string nodes that currently exist
provide a use case for a trailing period in the value, but i wouldn't
exclude such a use case arising in the future. For example, malloc_conf
already supports some trailing non-letter characters in the value,
and kern.hostname already supports periods in the middle of the value,
right now.
Besides, i believe that in security-critical tools, parsing should be
strict, and trying to guess what people mean by invalid syntax is not
generally a good idea.
What's next? Should "sysctl kern.." also do something, and if so, what?
I don't see what benefit is provided in making "sysctl vm.malloc_conf."
work, but i do see how flooding someone with multiple pages of
output might be unwelcome if they wanted to type "sysctl kern.tty"
but somehow forget the "tty" part and hit enter after "sysctl kern." -
the trailing dot does indicate that they probably only wanted a
sub-node, or doesn't it?
Yours,
Ingo
> Index: sbin/sysctl/sysctl.c
> ===================================================================
> RCS file: /cvs/src/sbin/sysctl/sysctl.c,v
> retrieving revision 1.259
> diff -u -p -u -r1.259 sysctl.c
> --- sbin/sysctl/sysctl.c 17 May 2023 22:12:51 -0000 1.259
> +++ sbin/sysctl/sysctl.c 29 Aug 2023 21:36:19 -0000
> @@ -377,6 +377,9 @@ parse(char *string, int flags)
>
> (void)strlcpy(buf, string, sizeof(buf));
> bufp = buf;
> + len = strlen(string);
> + if (len > 0 && string[len - 1] == '.')
> + buf[len - 1] = '\0';
> if ((cp = strchr(string, '=')) != NULL) {
> *strchr(buf, '=') = '\0';
> *cp++ = '\0';
