svn commit: r322893 - head/bin/dd
Bruce Evans
brde at optusnet.com.au
Fri Aug 25 22:14:13 UTC 2017
On Fri, 25 Aug 2017, Alan Somers wrote:
> Log:
> dd(1): Incorrect casting of arguments
It is indeed now incorrect.
> dd(1) casts many of its numeric arguments from uintmax_t to intmax_t and
> back again to detect whether or not the original arguments were negative.
> This is not correct, and causes problems with boundary cases, for example
> when count is SSIZE_MAX-1.
The casts were correct. Did someone break the range checks? I wrote most
of the cast, but would rarely write range checks using unportable casts.
SSIZE_MAX-1 isn't a boundary case. The boundary is at SSIZE_MAX. %zd
might work for it not. But %zd is wrong for SSIZE_MAX-1, since the
expression might expand the type.
> Modified: head/bin/dd/args.c
> ==============================================================================
> --- head/bin/dd/args.c Fri Aug 25 14:42:11 2017 (r322892)
> +++ head/bin/dd/args.c Fri Aug 25 15:31:55 2017 (r322893)
> @@ -41,6 +41,7 @@ __FBSDID("$FreeBSD$");
>
> #include <sys/types.h>
>
> +#include <ctype.h>
Unrelated change.
> #include <err.h>
> #include <errno.h>
> #include <inttypes.h>
> @@ -184,7 +185,7 @@ f_bs(char *arg)
>
> res = get_num(arg);
> if (res < 1 || res > SSIZE_MAX)
> - errx(1, "bs must be between 1 and %jd", (intmax_t)SSIZE_MAX);
> + errx(1, "bs must be between 1 and %zd", SSIZE_MAX);
> in.dbsz = out.dbsz = (size_t)res;
> }
>
This is incorrect. The old version used the portable method of casting
a signed value of unknown type to intmax_t before %zd existed. %zd still
isn't useable in either theory or practice for printing ssize_t's.
Theory:
C99 defines %zd as being for the signed type "corresponding" to size_t.
Who knows what "corresponding" is except for pure 2's complement with
no padding bits? POSIX only defines ssize_t circularly as being "any
signed integer type capable of representing the range -1 through
SSIZE_MAX" where SSIZE_MAX is the maximum of ssize_t". It has a few
restrictions that make this not completely circular, but I couldn't
find anywhere where it states the usual restriction on limits -- that
they have the type of the default promotion of the type that they limit.
Practice:
As reported in other replies, some arches define SSIZE_MAX in a way that
makes its type not the default promotion of ssize_t. Most likely
SSIZE_MAX is bogusly long and ssize_t is int, or vice versa, on an
arch where int and long have the same representation. Printf format
checking reports this type mismatch because it would be fatal on other
arches.
> @@ -195,22 +196,22 @@ f_cbs(char *arg)
>
> res = get_num(arg);
> if (res < 1 || res > SSIZE_MAX)
> - errx(1, "cbs must be between 1 and %jd", (intmax_t)SSIZE_MAX);
> + errx(1, "cbs must be between 1 and %zd", SSIZE_MAX);
> cbsz = (size_t)res;
> }
Similarly.
>
> static void
> f_count(char *arg)
> {
> - intmax_t res;
> + uintmax_t res;
This breaks the not so careful checking for negative args.
dd wants to allow negative args for seek offsets and not much else.
It has a badly written get_off_t() to handle the offsets, but old code
like this still used the old function get_num() that returns an unsigned
type. This is a feature since get_off_t() is so badly written that it
is better to convert get_num() almost as done here. My version does this
internally:
X /*
X * Convert an expression of the above forms to an off_t. This version does
X * not handle negative numbers perfectly. It assumes 2's complement and
X * maybe nonnegative multipliers. I hope perfect handling is not necessary
X * for dd.
X */
X static off_t
X get_off_t(const char *val)
X {
X u_quad_t num;
X
X num = get_num(val);
X if (num != (u_quad_t)(off_t)num)
X errx(1, "%s: offset too large", oper);
X return ((off_t)num);
X }
This still uses the bogus type u_quad_t since that is what get_num() returns
in the old version of dd that this patch is for.
There should be no restriction except UINTMAX_MAX on counts. This is fixed
in my version:
X static void
X f_count(char *arg)
X {
X
X cpy_cnt = get_num(arg);
X #if 0
X if (cpy_cnt == 0)
X terminate(0);
X #endif
X }
get_num() already did correct range checking.
A count of 0 should mean infinity.
Code elsewhere needs be careful to go to infinity for count 0 and to
not multiply a large count by a block size. In practice, counts of
nearly UINTMAX_MAX are unreachable.
>
> - res = (intmax_t)get_num(arg);
> - if (res < 0)
> - errx(1, "count cannot be negative");
> + res = get_num(arg);
> + if (res == UINTMAX_MAX)
> + errc(1, ERANGE, "%s", oper);
This is nonsense error checking. get_num() already did correct checking
and exited on range errors. A value of UINTMAX_MAX is not necessarily an
error. After calling strtoumax(), it might be either a range error or at
the end of the range (including the negative end). Since get_num() returned,
it was not a range error. The check here does extra work to break the case
where it is in-range.
> if (res == 0)
> - cpy_cnt = (uintmax_t)-1;
> + cpy_cnt = UINTMAX_MAX;
> else
> - cpy_cnt = (uintmax_t)res;
> + cpy_cnt = res;
> }
I count of 0 always meant infinity. It was converted to the huge value
(uintmax_t)-1 which is an alternative spelling of UINTMAX_MAX. My version
makes changes elswhere so that 0 works as itself and the user-specified
UINTMAX_MAX is not corrupted.
>
> static void
> @@ -219,7 +220,7 @@ f_files(char *arg)
>
> files_cnt = get_num(arg);
> if (files_cnt < 1)
> - errx(1, "files must be between 1 and %jd", (uintmax_t)-1);
> + errx(1, "files must be between 1 and %zu", SIZE_MAX);
> }
%jd here was wrong. The value has type uintmax_t. It was printed as -1.
Now a different wrong value is printed. The maximum returned by get_num()
is still UINTMAX_MAX. This differs from SIZE_MAX on 64-bit arches.
My version removes the range check and allows a count of 0.
>
> static void
> @@ -240,8 +241,8 @@ f_ibs(char *arg)
> if (!(ddflags & C_BS)) {
> res = get_num(arg);
> if (res < 1 || res > SSIZE_MAX)
Range checks like this are of course correct. Some of the ones with bogus
casts might be for portability to systems without OFF_MAX and/or UOFF_MAX.
> - errx(1, "ibs must be between 1 and %jd",
> - (intmax_t)SSIZE_MAX);
> + errx(1, "ibs must be between 1 and %zd",
> + SSIZE_MAX);
> in.dbsz = (size_t)res;
> }
> }
The change is unportable, as above
> @@ -261,8 +262,8 @@ f_obs(char *arg)
> if (!(ddflags & C_BS)) {
> res = get_num(arg);
> if (res < 1 || res > SSIZE_MAX)
> - errx(1, "obs must be between 1 and %jd",
> - (intmax_t)SSIZE_MAX);
> + errx(1, "obs must be between 1 and %zd",
> + SSIZE_MAX);
> out.dbsz = (size_t)res;
> }
> }
Unportable.
>
> Modified: head/bin/dd/conv.c
> ==============================================================================
> --- head/bin/dd/conv.c Fri Aug 25 14:42:11 2017 (r322892)
> +++ head/bin/dd/conv.c Fri Aug 25 15:31:55 2017 (r322893)
> @@ -133,7 +133,7 @@ block(void)
> */
> ch = 0;
> for (inp = in.dbp - in.dbcnt, outp = out.dbp; in.dbcnt;) {
> - maxlen = MIN(cbsz, in.dbcnt);
> + maxlen = MIN(cbsz, (size_t)in.dbcnt);
> if ((t = ctab) != NULL)
> for (cnt = 0; cnt < maxlen && (ch = *inp++) != '\n';
> ++cnt)
Bogus cast to compensate for dbcnt being changed to ssize_t.
ssize_t is a necessary evil holding the return value of read() and
write() (except the return type should be plain int as in V7 and ssize_t
shouldn't exist) (but the main design error was changing the arg type
of read() and write() from int to unsigned and then to size_t. The
different types give a type morass and args larger than than the signed
max are unportable at best). Buffer sizes should have size <= SSIZE_MAX
because the behaviour of read() and write() is implementation-defined
and hard to use betteen SSIZE_MAX+(size_t)1 and SIZE_MAX. But don't
use ssize_t for anything else. Not even for buffer sizes that it can
hold. You also changed dbsz to ssize_t. That is the wrong type to
pass to read() and write(). It gets converted implicitly by the
prototype. The above cast does the same conversion to hide the bug
that the types don't match. Both of the implicit conversions might
be errors, but compilers don't warn enough for ones in prototypes
because they are too common.
> @@ -146,7 +146,7 @@ block(void)
> * Check for short record without a newline. Reassemble the
> * input block.
> */
> - if (ch != '\n' && in.dbcnt < cbsz) {
> + if (ch != '\n' && (size_t)in.dbcnt < cbsz) {
> (void)memmove(in.db, in.dbp - in.dbcnt, in.dbcnt);
> break;
> }
Lots more type poisoning.
You didn't spread it to the memmove() call. The second and third args now
have type ssize_t instead of size_t. This is hidden by thr prototype.
Signed poisoning of cbsz would break the warning about the type mismatch
in the comparison.
I actually don't like unsigned types, but dd has always used them a lot
and needs them to reach 4G on 32-bit systems. All unsigned works almost
as well as all signed.
> @@ -228,7 +228,7 @@ unblock(void)
> * translation has to already be done or we might not recognize the
> * spaces.
> */
> - for (inp = in.db; in.dbcnt >= cbsz; inp += cbsz, in.dbcnt -= cbsz) {
> + for (inp = in.db; (size_t)in.dbcnt >= cbsz; inp += cbsz, in.dbcnt -= cbsz) {
> for (t = inp + cbsz - 1; t >= inp && *t == ' '; --t)
> ;
> if (t >= inp) {
This cast also breaks the formatting.
>
> Modified: head/bin/dd/dd.c
> ==============================================================================
> --- head/bin/dd/dd.c Fri Aug 25 14:42:11 2017 (r322892)
> +++ head/bin/dd/dd.c Fri Aug 25 15:31:55 2017 (r322893)
> @@ -204,10 +204,10 @@ setup(void)
> * record oriented I/O, only need a single buffer.
> */
> if (!(ddflags & (C_BLOCK | C_UNBLOCK))) {
> - if ((in.db = malloc(out.dbsz + in.dbsz - 1)) == NULL)
> + if ((in.db = malloc((size_t)out.dbsz + in.dbsz - 1)) == NULL)
> err(1, "input buffer");
> out.db = in.db;
> - } else if ((in.db = malloc(MAX(in.dbsz, cbsz) + cbsz)) == NULL ||
> + } else if ((in.db = malloc(MAX((size_t)in.dbsz, cbsz) + cbsz)) == NULL ||
> (out.db = malloc(out.dbsz + cbsz)) == NULL)
> err(1, "output buffer");
>
Signed poisoning.
> @@ -405,7 +405,7 @@ dd_in(void)
> ++st.in_full;
>
> /* Handle full input blocks. */
> - } else if ((size_t)n == in.dbsz) {
> + } else if ((size_t)n == (size_t)in.dbsz) {
> in.dbcnt += in.dbrcnt = n;
> ++st.in_full;
>
Old bogus cast. n already has type size_t. New signed poisoning.
> @@ -562,7 +562,7 @@ dd_out(int force)
> outp += nw;
> st.bytes += nw;
>
> - if ((size_t)nw == n && n == out.dbsz)
> + if ((size_t)nw == n && n == (size_t)out.dbsz)
> ++st.out_full;
> else
> ++st.out_part;
>
Lots more signed poisoning.
nw holds the value returned by write(), so it needs to be signed (ssize_t).
This already poisoned most uses of it.
> Modified: head/bin/dd/dd.h
> ==============================================================================
> --- head/bin/dd/dd.h Fri Aug 25 14:42:11 2017 (r322892)
> +++ head/bin/dd/dd.h Fri Aug 25 15:31:55 2017 (r322893)
> @@ -38,10 +38,9 @@
> typedef struct {
> u_char *db; /* buffer address */
> u_char *dbp; /* current buffer I/O address */
> - /* XXX ssize_t? */
Comment written before someone understood the full brokenness of ssize_t.
> - size_t dbcnt; /* current buffer byte count */
Counts should be signed (buffer counts for passing to read()/write() or
just adding up).
> - size_t dbrcnt; /* last read byte count */
The return value might need to be signed, but it should be converted to
unsigned as soon as possible (after checking for errors) to minimise
signed poisoning.
> - size_t dbsz; /* block size */
Block sizes should be signed.
> + ssize_t dbcnt; /* current buffer byte count */
> + ssize_t dbrcnt; /* last read byte count */
> + ssize_t dbsz; /* block size */
>
> #define ISCHR 0x01 /* character device (warn on short) */
> #define ISPIPE 0x02 /* pipe-like (see position.c) */
>
> Modified: head/bin/dd/position.c
> ==============================================================================
> --- head/bin/dd/position.c Fri Aug 25 14:42:11 2017 (r322892)
> +++ head/bin/dd/position.c Fri Aug 25 15:31:55 2017 (r322893)
> @@ -207,7 +207,7 @@ pos_out(void)
> n = write(out.fd, out.db, out.dbsz);
> if (n == -1)
> err(1, "%s", out.name);
> - if ((size_t)n != out.dbsz)
> + if (n != out.dbsz)
> errx(1, "%s: write failure", out.name);
> }
> break;
This moves the ugly ugly conversions to an implicit one in the function call.
The error handling is still broken. The non-error of a short write is not
handled and this is misreported as a write failure (though correctly using
errx()).
The signed poisoning makes the critical old and new bugs hard to see.
Bruce
More information about the svn-src-head
mailing list