Bug 236714
| Summary: | Clang problem with rctl(8) | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Edward Tomasz Napierala <trasz> |
| Component: | bin | Assignee: | Mateusz Guzik <mjg> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | CC: | dim, emaste, kib, markj, mjg |
| Priority: | --- | ||
| Version: | CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
It's a regression from r341182. The kernel is exporting racct_enable as a bool. Thanks! I wonder if we could perhaps return an error from sysctl(3) in this case? (In reply to Edward Tomasz Napierala from comment #2) It would probably be better to detect the case where the old buffer is larger than 1 byte, and zero the full buffer before writing out the value. (In reply to Mark Johnston from comment #1) > It's a regression from r341182. The kernel is exporting racct_enable as a > bool. Yes indeed, I did a bit of debugging, and it turns out that if you pass a pointer to an int (and the length of an int, obviousl), the sysctl does not modify the variable at all... (In reply to Mark Johnston from comment #3) I don't think that's feasible since consumers can pass arbitrary sizes, and some probably pass more than they intended in a bogus manner where they would not be prepared to sudden zeroing of the area passed. Looks like the bug at hand can be easily taken care of with just resizing the var to bool in the userspace tool. While the previous binary remains broken I don't think it's a big deal since this is current-only. that is: diff --git a/usr.bin/rctl/rctl.c b/usr.bin/rctl/rctl.c index 74073c13207e..f8b5115a4507 100644 --- a/usr.bin/rctl/rctl.c +++ b/usr.bin/rctl/rctl.c @@ -378,8 +378,9 @@ print_rules(char *rules, int hflag, int nflag) static void enosys(void) { - int error, racct_enable; size_t racct_enable_len; + int error; + bool racct_enable; racct_enable_len = sizeof(racct_enable); error = sysctlbyname("kern.racct.enable", (In reply to Mateusz Guzik from comment #5) Well, it's fine in the (probably more common) case where the kernel changed a type from int to bool and userspace applications are not immediately updated to match. But I agree that it could cause some unpleasant surprises, and your diff looks fine to me. What is the problem checking for exactly old or new buffer size ? Look e.g. at the sysctl_bufspace() in vfs_bio.c. I thought markj wanted to handle a general case. I did not check if anything depends on the affected sysctl in userspace, so that's my bad. However, since the change is only in current and the tool can be modified I don't think there is much use of adding aforementioned support there. (In reply to Mateusz Guzik from comment #8) Is rctl(8) useful in any way for non-root user, or for root user in a jail ? If answer to any of this question is yes, we should provide ABI compat. Otherwise, indeed, it does not matter. It's all gated with priv_check calls defaulting to not allowing jailed root to do anything. So someone would have to have a dedicated mac module granting this. So if trasz is fine with the patch, I'll commit it. Patch looks good, thank you! What about the underlying issue, which is sysctl(3) silently failing when it receives an improperly sized buffer? Would it be feasible to make it return an error, so it's easier to fix next time it happens? It is not being ignored. There is non-zero garbage in an area not written to by sysctl, which makes the zero comparison fail.
diff --git a/usr.bin/rctl/rctl.c b/usr.bin/rctl/rctl.c
index 74073c13207e..8fb9e862aee8 100644
--- a/usr.bin/rctl/rctl.c
+++ b/usr.bin/rctl/rctl.c
@@ -378,12 +378,15 @@ print_rules(char *rules, int hflag, int nflag)
static void
enosys(void)
{
- int error, racct_enable;
+ int error, racct_enable = ~(0);
size_t racct_enable_len;
+
+ printf("%x\n", racct_enable);
racct_enable_len = sizeof(racct_enable);
error = sysctlbyname("kern.racct.enable",
&racct_enable, &racct_enable_len, NULL, 0);
+ printf("%x\n", racct_enable);
if (error != 0) {
if (errno == ENOENT)
Results in:
ffffffff
ffffff00
a.out: failed to show rules for '::': Function not implemented
A commit references this bug: Author: mjg Date: Wed Apr 3 20:37:14 UTC 2019 New revision: 345853 URL: https://svnweb.freebsd.org/changeset/base/345853 Log: rctl: fix sysctl kern.racct.enable use after r341182 The value was changed from int to bool. Since the new type is smaller, the rest of the variable in the caller was left unitialized. PR: 236714 Reported by: trasz Diagnosed by: markj Sponsored by: The FreeBSD Foundation Changes: head/usr.bin/rctl/rctl.c |
I've stumbled upon something that seems to me to be a miscompilation issue in rctl(8). Basically, this code in usr.bin/rctl/rctl.c: racct_enable_len = sizeof(racct_enable); error = sysctlbyname("kern.racct.enable", &racct_enable, &racct_enable_len, NULL, 0); if (error != 0) { if (errno == ENOENT) errx(1, "RACCT/RCTL support not present in kernel; see rctl(8) for details"); err(1, "sysctlbyname"); } if (racct_enable == 0) errx(1, "RACCT/RCTL present, but disabled; enable using kern.racct.enable=1 tunable"); Doesn't work as expected unless the racct_enable is initialized before calling sysctlbyname(3). Without this change: % doas rctl rctl: failed to show rules for '::': Function not implemented With the change: % doas rctl rctl: RACCT/RCTL present, but disabled; enable using kern.racct.enable=1 tunable