svn commit: r281787 - head/sbin/dmesg

[Bruce Evans]

On Mon, 20 Apr 2015, Eric van Gyzen wrote:

> Log:
>  dmesg: accommodate message buffer growth between the sysctl calls
>
>  Allocate 12.5% extra space to avoid ENOMEM when the message buffer
>  is growing steadily.

This is bogus allocation.  The message buffer has a small fixed size
so that it fits in physical memory in the kernel, and rarely changes
its size after it fills up.  Userland can easily statically allocate
a buffer 1000 if not 1000000 times as large as the full size in
virtual memory.  This is much easier to do than even half-baked
dynamic allocation.

> Modified: head/sbin/dmesg/dmesg.c
> ==============================================================================
> --- head/sbin/dmesg/dmesg.c	Mon Apr 20 20:06:25 2015	(r281786)
> +++ head/sbin/dmesg/dmesg.c	Mon Apr 20 20:07:39 2015	(r281787)
> @@ -118,6 +118,9 @@ main(int argc, char *argv[])
> 		 */
> 		if (sysctlbyname("kern.msgbuf", NULL, &buflen, NULL, 0) == -1)
> 			err(1, "sysctl kern.msgbuf");
> +		/* Allocate extra room for growth between the sysctl calls. */
> +		buflen += buflen/8;
> +		/* Allocate more than sysctl sees, for room to append \n\0. */
> 		if ((bp = malloc(buflen + 2)) == NULL)
> 			errx(1, "malloc failed");
> 		if (sysctlbyname("kern.msgbuf", bp, &buflen, NULL, 0) == -1)

This still fails if the size grows more than 12.5%.

This adds some style bugs:
- missing spaces around binary operator
- verboseness.  There is already a verbose comment about the method just
   above here
- new verbose comments not made less verbose by not adding newline delimiters
   for the block (a single statement) of code that they describe, unlike
   some old single-line comments in the file.
- (old style bug).  Expansion of the verbose comment above gave a worse
   comment scope obfuscation.  Code to adjust the buffer actually needs to
   be in a different block, but the comment about this code is appended
   to the main comment and the adjustment is not even appended.

Non-half-baked dynamic initialization would check the error returned by
the first sysctl, and if it is ENOMEM, then retry.  This is actually
simpler, since it doesn't need the second sysctl (except dynamically,
and almost never then).  Allocate some memory before starting the
loop.  This doesn't need any comments since it is obviously the only
correct way to determine the amount of memory needed if the size can
change.

Dynamic allocation is still overkill.  For the initial size, use
MSG_BUFSIZE (+2) from <sys/msgbuf.h>.  MSG_BUFSIZE is not quite unusable
outside of the kernel.  It is just a default, so cannot be used correctly
outside of the kernel, but given that it exists, it is slightly better
than an arbitrary value for the initial size, since that gives only 1
iteration of the loop in the usual case where dmesg was compiled with
the same sources as the kernel and the user didn't change the default.

Untested version:

X 	buflen = MSGBUFSIZ / 2;
X 	bp = NULL;
X 	do {
X 		buflen *= 2;
X 		if ((bp = reallocf(bp, buflen + 2)) == NULL)
X 			errx(1, "malloc failed");
X 		if (sysctlbyname("kern.msgbuf", bp, &buflen, NULL, 0) == 0)
X 			break;
X 		if (errno != ENOMEM)
X 			err(1, "sysctl kern.msgbuf");
X 	} while (0);

The error handling is still painful.  malloc() failure can't happen, but
I preserved the check for it.  buflen *= 2 can overflow, but I omitted
checking for it.  This should be safe since the overflow only malloc()
failures (that can't happen) have happened.

phk has railed against the practice of starting with a small size and
doubling it.  Starting with a large size and hoping to never change it
is better for bloatware.  The above does the latter, except the "large"
size is small.  Doubling it is correct since anyone changing the
default shouldn't make small adjustments.  But small utilities don't
need any dynamic allocation.  They can allocate "large" buffers that
are actually small using static allocation.

Another not so good way to do the above is have a sysctl that returns
the full msgbuf size.  This cannot change.  Then the only change needed
in the original code is to change the first sysctl to determine the
full size.  All kernel parameters that are not constant because they
may be changed by kernel options should have such a sysctl and shouldn't
have any manifest constant (except as a default or minimum value, like
POSIX minimum values for sysconf()).  But using such a constant here
just takes more code.  It takes 1 sysctl() to determine the size, then
a malloc(), then 1 sysctl() (but not in a loop) to use the size.
Fully dynamical allocation starting with a reasonably large size usually
only needs 1 malloc() and 1 sysctl().

Elsewhere, in much more important places, there is an enormous amount
of sloppy code that abuses POSIX manifest constants like PATH_MAX
instead of technically correct dynamic code using sysconf(), since
static allocation is usually good enough and dynamic allocation is
so painful.

E.g., static allocation using PATH_MAX is:

 	char path[PATH_MAX];		/* technically broken */
 	char path2[MAXPATHLEN];		/* honestly unportable */

and using sysconf() it is:

 	long len = sysconf(_SC_PATH_MAX);
 	/* Several lines to check for and handle errors in sysconf(). */
 	/* Several lines to check for and handle len being unrepresentable
 	 * size_t.
 	 */
 	path = malloc(len);
 	/* Several lines to check for and handle errors in malloc(). */

and using PATH_MAX and sysconf() it is:

#ifdef PATH_MAX
 	char path[PATH_MAX];		/* assume comp. detects if too large
#else
 	/* Above code for dynamic case. */
#endif

Bruce