Hi all, any thoughts on this behavior?
My main interest in this was that I was trying to add OpenBSD
support to a 3rd party, multi-platform sysctl library. In the
current implementation, this library was relying on the behavior
below to determine the required buffer size before performing the
read, which was failing for any entry that internally used the
sysctl_struct() function (sysctl_rdstruct() works fine).
I guess as a workaround a large oldp buffer could always be
passed in.
Thanks,
Nate
On Sun, Oct 03, 2021 at 06:32:38PM -0700, Nathan Houghton wrote:
> Hi all,
>
> I think I found some cases where the "oldlenp" value is not properly
> updated according to the details in the manual page. In particular
> this section from sysctl(2):
>
> The size of the available data can be determined by calling sysctl() with
> a NULL parameter for oldp. The size of the available data will be
> returned in the location pointed to by oldlenp.
>
> This seems to work fine for most cases, but when testing the program
> below I found that for some MIB paths, the oldlenp value is not
> updated.
>
> System details: OpenBSD 6.9, amd64, with all patches applied via syspatch
>
> ---8<----
> #include <stdio.h>
>
> #include <sys/queue.h>
> #include <sys/types.h>
> #include <sys/sysctl.h>
> #include <sys/socket.h>
>
> #include <netinet/in.h>
> #include <netinet/tcp.h>
> #include <netinet/tcp_timer.h>
> #include <netinet/tcp_var.h>
>
> int main(int argc, char **argv)
> {
> #if 1
> /* working case, prints: "r: 0, len: 4" */
> int path[] = { CTL_NET, PF_INET, IPPROTO_TCP, TCPCTL_SYN_HASH_SIZE };
> #else
> /* failing case, prints: "r: 0, len: 0" */
> int path[] = { CTL_NET, PF_INET, IPPROTO_TCP, TCPCTL_BADDYNAMIC };
> #endif
>
> size_t buflen = 0;
> int r = sysctl(path, sizeof(path) / sizeof(path[0]), NULL, &buflen,
> NULL, 0);
> printf("r: %d, len: %zu\n", r, buflen);
>
> return 0;
> }
> ---8<----
>
> The "TCPCTL_SYN_HASH_SIZE" MIB path ends up being handled by function
> sysctl_int_bounded():
>
> ---8<----
> int
> sysctl_int_bounded(void *oldp, size_t *oldlenp, void *newp, size_t newlen,
> int *valp, int minimum, int maximum)
> {
> int error = 0;
> int val;
>
> if (oldp && *oldlenp < sizeof(int))
> return (ENOMEM);
> if (newp && newlen != sizeof(int))
> return (EINVAL);
> *oldlenp = sizeof(int); <== oldlenp updated
> ...
> ---8<----
>
> The "TCPCTL_BADDYNAMIC" MIB path ends up being handled by function
> sysctl_struct():
>
> ---8<----
> int
> sysctl_struct(void *oldp, size_t *oldlenp, void *newp, size_t newlen, void
> *sp,
> size_t len)
> {
> int error = 0;
>
> if (oldp && *oldlenp < len)
> return (ENOMEM);
> if (newp && newlen > len)
> return (EINVAL);
> if (oldp) {
> *oldlenp = len; <== value only conditionally updated
> error = copyout(sp, oldp, len);
> }
> if (error == 0 && newp)
> error = copyin(newp, sp, len);
> return (error);
> }
> ---8<----
>
> Should this update to oldlenp be moved up (something like the patch
> below)? Or this intentional / expected behavior?
>
> Thanks,
> Nate
>
> ---8<----
> Index: kern/kern_sysctl.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
> retrieving revision 1.389
> diff -u -p -r1.389 kern_sysctl.c
> --- kern/kern_sysctl.c 8 Feb 2021 10:51:02 -0000 1.389
> +++ kern/kern_sysctl.c 4 Oct 2021 01:24:34 -0000
> @@ -1043,8 +1043,8 @@ sysctl_struct(void *oldp, size_t *oldlen
> return (ENOMEM);
> if (newp && newlen > len)
> return (EINVAL);
> + *oldlenp = len;
> if (oldp) {
> - *oldlenp = len;
> error = copyout(sp, oldp, len);
> }
> if (error == 0 && newp)
> ---8<----
>
