svn commit: r317061 - in head: libexec/rpc.rstatd sys/amd64/amd64 sys/amd64/include sys/arm/arm sys/arm/include sys/arm64/include sys/cddl/contrib/opensolaris/uts/common/fs/zfs sys/compat/linprocfs...

[Bruce Evans]

On Tue, 18 Apr 2017, Alan Somers wrote:

> On Mon, Apr 17, 2017 at 11:34 AM, Gleb Smirnoff <glebius at freebsd.org> wrote:

Please trim quotes.

[... 5 lines of quotes trimmed]

>> Log:
>>   - Remove 'struct vmmeter' from 'struct pcpu', leaving only global vmmeter
>>     in place.  To do per-cpu stats, convert all fields that previously were
>>     maintained in the vmmeters that sit in pcpus to counter(9).

[... 21 lines of quotes trimmed]

[... more than 50 lines of quotes trimmed]

>>   head/usr.bin/top/machine.c
>>   head/usr.bin/vmstat/vmstat.c

The previous 2 lines turn out to be relevant.  I missed the update to
vmstat and checked a wrong version in my bug report described below
I checked an updated top, but the change seemed too small to help.
systat was not updated.

> This change broke backwards compatibility with old top binaries.  When
> I use a kernel at version 317094 but a top from 14-April, I get the
> error "top: sysctl(vm.stats.vm.v_swappgsin...) failed: Cannot allocate
> memory".  I get the same error when running top from an 11.0-RELEASE
> jail.  Can you please add backward compatibility shims?

I sent a much longer (30 times longer) bug report to the author of the
bug.

Most or all statistics utilities that use vmmeter are broken.  vmstat
is too broken to even notice the bug -- it silently ignores the error,
an has type errors which prevent other errors which it doesn't ignore.
The result is silently truncating to 32 bits, like a quick fix would
do intentionally.  systat -v detects the errors but prints warning
messages to the status line where they overwrite each other so make
the problem look smaller than it is.  The result is unsilently
truncating to 32 bits.

I've never seen any backwards compatibility shims added for sysctls.
None should be needed, but there are few or no utilities other than
sysctl(8) which use sysctl(3) in a portable way.  Utilities tend to
hard-code types and thus break when the types are expanded or shrunk.
Especially broken ones like vmstat even ignore the error for their
size being too small.  There is no error for the size being too small,
so callers must check if the size returned is the size supplied.
vmstat is too broken to check this too.  systat -v does check.

Note that using the types in the system header is a form of hard-coding.
This gives binaries that only work when run on the same system as the
headers.  Using the "any" type is best done using a smart wrapper or
perhaps a smarter sysctl(3), and many utilities already have dumb
wrappers.  The one in vmstat() shows how not to do it:

X	static int
X	mysysctl(const char *name, void *oldp, size_t *oldlenp,
X	    void *newp, size_t newlen)
X	{
X		int error;
X 
X		error = sysctlbyname(name, oldp, oldlenp, newp, newlen);
X		if (error != 0 && errno != ENOMEM)
X			xo_err(1, "sysctl(%s)", name);
X		return (error);
X	}

ENOMEM here means that our variable is too small.  It is not easy to expand
correctly, but it is easy to check if it is small enough to cause a problem.
Something like:

 		// Zero-extend in case our variable is larger than the
 		// system's.  We only support unsigned variables.
 		bzero(oldp, *oldlenp);

 		// Don't use our garbage new* variables.  All calls are
 		// read-only since we are a stats utility, and this wrapper
 		// doesn't support writing.
 		if (sysctlbyname(name, oldp, oldlenp, NULL, 0) == 0)
 			return (0)

 		u_char buf[640 * 1024];	// should be enough for anyint
 		size_t buflen;
 		size_t off;

 		buflen = sizeof(buf);
 		error = sysctlbyname(name, buf, buflen, NULL, 0);
 		if (error != 0)
 			xo_err(1, "sysctl(%s)", name);
 		for (off = *oldlenp; buf < bufsize; off++) {
 			// Check for zero-extension.
 			// This version only supports unsigned types.
 			if (buf[off] != 0) {
 				errno = EOVERFLOW;
#ifdef CALLERS_PROBLEM
 				// sysctl(3) should always have done this
 				// too.  That is, return EOVERFLOW instead
 				// of ENOMEM when the result is too large
 				// to fit, and don't return an error if
 				// it does fit.
 				return (-1);
#else
 				// Now a useful error message.
 				xo_err(1, "sysctl(%s)", name);
#endif
 			}
 		}

Then expand sizes in the caller if necessary.  u_int would work in most
cases, and so would u_char for small and boolean variables.  CALLERS_PROBLEM
allows use of small types when only differences are needed, as for some
cases in vmstat.  uintmax_t would work in more cases.  But it is still
a form of hard-coding -- it will break when the system uintmax_t is
increased to {128, 256, 512, ...}  bits while the user uintmax_t remains
uint64_t, and the extra bits are actually used.  64-bit types rarely
overflow, but if the system type is larger then 64 bytes was not enough
for anyint.

The bugs in vmstat now go in the other direction.  It now hard-codes
uint64_t instead of u_int for the expanded types.  Its missing error
handling is now a more serious problem.  When run on an old kernel,
sysctl() returns 32 bits into the 64-bit variables and the error is
not detected.  Perhaps the variables are initially zero so this is
not a problem, but the zeroing should be more intentional.  It is very
easy to add to the sysctl wrapper, as above.

Bruce