WIP: sysctl: Respect max length when treat a variable string as a constant string

[zlei]

Summary

D48511 has too many comments to review.

Test Plan

See test plan in D48511 .

[zlei]

sys/kern/kern_sysctl.c

1903

I'd keep arg2 assigned, in case consumer want to write to such sysctl knob,

static char buff[] = "some long long content";
SYSCTL_STRING(_sysctl, OID_AUTO, buff, CTLFLAG_RW, buff, 0,
    "Read-write string buff");

I have not searched the above usage, but just in case.

[zlei]

sys/kern/kern_sysctl.c

1903

Emm, CTLFLAG_RW is wong usage. Consider this one.

--- SYSCTL_STRING(_sysctl, OID_AUTO, buff, CTLFLAG_RW, buff, 0,
+++ SYSCTL_STRING(_sysctl, OID_AUTO, buff, CTLFLAG_RDTUN, buff, 0,
    "Read-write string buff");

[zlei]

@olce You can click "Hide All Inlines" to help reviewing.

[avg]

Still looks good to me.

[olce]

This generally looks good (and much better than D48511) except for what I stated in an inline comment (don't mess with arg2, I insist on that).

Although not blocking because it doesn't affect the correctness of your change and current consumer expectations, my remark in D48511 that there isn't much sense in introducing the -1/+1 computation if you don't go all the way to ensure that a terminating NUL will be output in all cases still stands (the case where arg1 has len arg2 without being NUL terminated is not handled properly; in theory, it shouldn't happen, so a simple KASSERT() is probably enough, but that requires an audit of all uses of sysctl_handle_string(); else, just establish an alternative for the first SYSCTL_OUT() when outlen equals arg2 and arg1[arg2 - 1] == '\0' and force a NUL at tmparg[arg2 - 1] after the copy, and then replacing the strnlen() with a mere strlen()).

sys/kern/kern_sysctl.c

1903

I'd keep arg2 assigned, in case consumer want to write to such sysctl knob, (snip)

Please don't, this is in contradiction with current usage, the herald comment and probably common sense. Consumers can already cater to the scenario you described by just using sizeof(buff) in place of 0:

SYSCTL_STRING(_sysctl, OID_AUTO, buff, CTLFLAG_RW, buff, sizeof(buff), "Read-write string buff");

Emm, CTLFLAG_RW is wong usage.

Actually it is CTLFLAG_RDTUN that seems wrong. The generic sysctl machinery won't let you write into such a knob anyway. CTLFLAG_WRTUN must be used instead.

So I think you're trying to cater to a non-existent case here, or am I missing something?

arg2 shouldn't be modified, as explained in D48511.

[kib]

sys/kern/kern_sysctl.c

1916–1917

The check for NULL is not needed.

[zlei]

sys/kern/kern_sysctl.c

1903

I'd keep arg2 assigned, in case consumer want to write to such sysctl knob, (snip)

Please don't, this is in contradiction with current usage, the herald comment and probably common sense. Consumers can already cater to the scenario you described by just using sizeof(buff) in place of 0:

SYSCTL_STRING(_sysctl, OID_AUTO, buff, CTLFLAG_RW, buff, sizeof(buff), "Read-write string buff");

I agree with you that a CTLFLAG_WR flag with a const string ( arg2 == 0 ) is wrong.

Emm, CTLFLAG_RW is wong usage.

Actually it is CTLFLAG_RDTUN that seems wrong. The generic sysctl machinery won't let you write into such a knob anyway. CTLFLAG_WRTUN must be used instead.

So I think you're trying to cater to a non-existent case here, or am I missing something?

arg2 shouldn't be modified, as explained in D48511.

Indeed developers are creative, I see real usage of CTLFLAG_RDTUN.

% grep -rA2 'SYSCTL_STRING' sys | grep -C1 'CTLFLAG_RDTUN'
--
sys/kern/subr_prf.c:SYSCTL_STRING(_kern, OID_AUTO, boot_tag, CTLFLAG_RDTUN | CTLFLAG_NOFETCH,
sys/kern/subr_prf.c-    current_boot_tag, 0, "Tag added to dmesg at start of boot");
--
--
sys/kern/kern_cons.c:SYSCTL_STRING(_kern, OID_AUTO, vty, CTLFLAG_RDTUN | CTLFLAG_NOFETCH, vty_name,
sys/kern/kern_cons.c-    0, "Console vty driver");
--
--
sys/powerpc/powerpc/platform.c:SYSCTL_STRING(_hw, OID_AUTO, platform, CTLFLAG_RDTUN,
sys/powerpc/powerpc/platform.c-    plat_name, 0, "Platform currently in use");
--
--
sys/security/mac_lomac/mac_lomac.c:SYSCTL_STRING(_security_mac_lomac, OID_AUTO, trusted_interfaces, CTLFLAG_RDTUN,
sys/security/mac_lomac/mac_lomac.c-    trusted_interfaces, 0, "Interfaces considered 'trusted' by MAC/LOMAC");
--
--
sys/security/mac_biba/mac_biba.c:SYSCTL_STRING(_security_mac_biba, OID_AUTO, trusted_interfaces, CTLFLAG_RDTUN,
sys/security/mac_biba/mac_biba.c-    trusted_interfaces, 0, "Interfaces considered 'trusted' by MAC/Biba");
--
sys/geom/part/g_part.c:SYSCTL_STRING(_kern_geom_part, OID_AUTO, separator,
sys/geom/part/g_part.c-    CTLFLAG_RDTUN, &g_part_separator, sizeof(g_part_separator),
sys/geom/part/g_part.c-    "Partition name separator");
--
--
sys/dev/cxgbe/t4_main.c:SYSCTL_STRING(_hw_cxgbe, OID_AUTO, config_file, CTLFLAG_RDTUN, t4_cfg_file,
sys/dev/cxgbe/t4_main.c-    sizeof(t4_cfg_file), "Firmware configuration file");
--
--
sys/arm/arm/platform.c:SYSCTL_STRING(_hw, OID_AUTO, platform, CTLFLAG_RDTUN | CTLFLAG_NOFETCH, plat_name, 0,
sys/arm/arm/platform.c-    "Platform currently in use");

But I think a combination of arg2 == 0 and CTLFLAG_RDTUN is wrong. The CTLFLAG_NOFETCH version is good.

[olce]

Good to go (with setting arg2 removed, but that could happen in a separate commit if you don't want to hold the changes here).

I've seen your other revisions about fixing bad uses of a 0 arg2, will comment there.

Thanks a lot for improving the current situation, and farther than just the initial bug you wanted to fix required.

sys/kern/kern_sysctl.c

1903

Indeed developers are creative, I see real usage of CTLFLAG_RDTUN.
But I think a combination of arg2 == 0 and CTLFLAG_RDTUN is wrong. The CTLFLAG_NOFETCH version is good.

I think you're right. Knob handlers can be called with a non-NULL req->newptr even if CTLFLAG_WR is not present for the initial setting from the environment on CTLFLAG_TUN, except if CTLFLAG_NOFETCH is present. So, if CTLFLAG_NOFETCH is not present, arg2 should not be 0.

[zlei]

In D48881#1115188, @olce wrote:

Good to go (with setting arg2 removed, but that could happen in a separate commit if you don't want to hold the changes here).

Given with setting arg2` removed` is a behavior change, I'd prefer a separate commit.

I've seen your other revisions about fixing bad uses of a 0 arg2, will comment there.

Thanks a lot for improving the current situation, and farther than just the initial bug you wanted to fix required.

And I found another issue, related to https://reviews.freebsd.org/D30433.

Thanks all for your reviewing !