svn commit: r252672 - head/sbin/nvmecontrol

[Bruce Evans]

On Thu, 4 Jul 2013, Jim Harris wrote:

> Log:
>  Fix printf argument mismatch reported by gcc on i386.

This just substitutes one printf format with another.

> Modified: head/sbin/nvmecontrol/firmware.c
> ==============================================================================
> --- head/sbin/nvmecontrol/firmware.c	Thu Jul  4 00:16:43 2013	(r252671)
> +++ head/sbin/nvmecontrol/firmware.c	Thu Jul  4 00:26:24 2013	(r252672)
> @@ -82,7 +82,7 @@ read_image_file(char *path, void **buf,
> 		exit(EX_IOERR);
> 	}
> 	if ((*buf = malloc(sb.st_size)) == NULL) {
> -		fprintf(stderr, "Unable to malloc %zd bytes.\n",
> +		fprintf(stderr, "Unable to malloc %jd bytes.\n",
> 		    sb.st_size);
> 		close(fd);
> 		exit(EX_IOERR);
> @@ -95,7 +95,7 @@ read_image_file(char *path, void **buf,
> 	}
> 	if (*size != sb.st_size) {
> 		fprintf(stderr, "Error reading '%s', "
> -		    "read %zd bytes, requested %zd bytes\n",
> +		    "read %zd bytes, requested %jd bytes\n",
> 		    path, *size, sb.st_size);
> 		close(fd);
> 		exit(EX_IOERR);

st_size has type off_t.  The format is for intmax_t.  off_t is only
accidentally compatible with off_t (on more arches that with ssize_t,
so compile-time testing takes longer to find the bug).

There are many other type errors visible in this patch:
- st_size has type off_t.  It is blindly converted to size_t when it is
   passed to malloc().  The message for malloc() failure prints the
   non- converted size, but says that the non-converted size was
   requested.  Careful code would convert to size_t and check that the
   result fits.  The converted value has the correct type for printing
   with %zu.  (The old printf format also has a sign error in the format.).

- st_size has type off_t.  It is blindly converted to size_t when it is
   passed to read().  And although read() takes a size_t arg, ones larger
   than SSIZE_MAX give implementation-defined behaviour.  On FreeBSD,
   the behaviour is to fail.  The message for read() failure prints the
   non-converted size, but says that the converted size was requested.
   Careful code would convert to size_t and check that the result fits in
   ssize_t.  The converted value has the correct type and range for printing
   with %zd.

Many style bugs are visible in this patch:
- the err() family is not used.  This gives secondary bugs:
   - the program name is not put in the the error messages in another way
   - the string for the read() error is not put in the message in another
     way.  This leads back to a non-style bug: the condition for an error
     is not (*size != sb.st_size); that can be for a short read; but using
     err() would only be correct if there is actually an error
   - warn(); ...; exit(); would have to be used instead of err(), since
     close(fd) might clobber errno.  But close() before exit() is bogus
     unless errors in close() are checked for and handled, and they are
     not.
- sysexits.h is used
- EX_IOERR for malloc() failure is just wrong
- the error messages are capitalized
- the first error message is terminated with a "."
- the second error message is obfuscated at the source level using string
   concatenation and splitting it across multiple lines
- the second error message has grammar errors (2 comma splices, with the
   error largest for the first).

Many programs use similar sloppy error checking for read().  This is only
broken if the file size exceeds SSIZE_MAX or short reads occur.  Neither
is likely to occur for image files.  But neither is malloc() failure.

Fixing most of these bugs gives:

 	size_t filesize;
 	...
 	if (sb.st_size > SSIZE_MAX)
 		errx(1, "size of file '%s is too large (%jd bytes)",
 		    (intmax_t)sb.st_size);
 	filesize = sb.st_size;
 	if ((*buf = malloc(filesize)) == NULL)
 		errx(1, "unable to malloc %zd bytes", filesize);
...
 	/* Repeat above size check if necessary (better do it up-front).

 	/* XXX still assume no short reads. */
 	if (*size != filesize)
 		err(1, "error reading '%s' (read %zd bytes; requested %zd)",
 		    path, *size, filesize);

Bruce