Date: Sun, 17 Nov 2002 15:35:36 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Julian Elischer <julian@elischer.org> Cc: Nate Lawson <nate@root.org>, Tim Robbins <tjr@FreeBSD.ORG>, FreeBSD current users <current@FreeBSD.ORG> Subject: Re: Sign fixes for disklabel(8) Message-ID: <20021117144918.J21008-100000@gamplex.bde.org> In-Reply-To: <Pine.BSF.4.21.0211161317530.53326-200000@InterJet.elischer.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 16 Nov 2002, Julian Elischer wrote:
> On Sat, 16 Nov 2002, Bruce Evans wrote:
> > Removing clauses gives maximal diffs and loses even some of the sub-minimal
> > bounds checking (values less than 0 were just errors in some cases, but
> > they are now converted to large unsigned values and not checked at all).
>
> but that make sthem the same as the "almost as large" bad values that
> were not checked before either.. The onlything that the code removed
> might serve would be to restrict times (in mSecs) to something like
> "less than 10 seconds" or something, but for exaple on some forms of
> floppy tape, it took almost that long to change "tracks" so it is
> hard to try predict what a legal value is.. (probably less than 1 day
> :-)
>
> removing the clause for testing < 0 (as it can no longer happen)
> seems easiest. A stupid value in the disklabel will stand out but will
> probably be ignored anyhow.
I dont't like changing broken code to different (slightly more) broken
code. Stupid values like 72736 for d_rpm actually get truncated to the
nondescript value 7200 by a later non-bounds-checked conversion (your
changes don't affect this).
> > > A sensible thing might be to compare against a "sensible" value
> > > but who knows what that value should be?
> >
> > Values should only be restricted to avoid ones that can't possibly work.
> > This is mostly already done.
Actually, it is mostly not already done, since we had `int v' and we blindly
assign this to u_int16_t's and u_int32_t's.
> > > I include the new diff for your coments
> >
> > I'll wait for a later one with more of the suggested changes incorporated
> > (strtoul() should use base 10, and most things shouldn't be changed, etc.).
>
> does it matter? I have had a few times when I'd have liked to be able to
> put a number in in hex. To be exactly compatible in NOT handling
> the 0x and leading '0' cases I suppose so..
Not a lot. I wouldn't mind support for hex but don't want support for
octal. I just want hex supported intentionally if at all.
% Index: disklabel.c
% ===================================================================
% RCS file: /home/ncvs/src/sbin/disklabel/disklabel.c,v
% retrieving revision 1.62
% diff -u -r1.62 disklabel.c
% --- disklabel.c 8 Oct 2002 12:13:19 -0000 1.62
% +++ disklabel.c 16 Nov 2002 08:23:14 -0000
% @@ -973,7 +974,7 @@
% }
% if (cpp < &dktypenames[DKMAXTYPES])
% continue;
% - v = atoi(tp);
% + v = strtoul(tp, NULL, 0);
% if ((unsigned)v >= DKMAXTYPES)
% fprintf(stderr, "line %d:%s %d\n", lineno,
% "Warning, unknown disk type", v);
Er, see above. Also, this still has a bogus cast to unsigned.
% @@ -1006,8 +1007,8 @@
% }
% continue;
% }
% - if (sscanf(cp, "%d partitions", &v) == 1) {
% - if (v == 0 || (unsigned)v > MAXPARTITIONS) {
% + if (sscanf(cp, "%u partitions", &v) == 1) {
% + if (v == 0 || v > MAXPARTITIONS) {
% fprintf(stderr,
% "line %d: bad # of partitions\n", lineno);
% lp->d_npartitions = MAXPARTITIONS;
The scanf family is another bad way to parse input. It is fundamentally
broken in the same way as atoi() -- its behaviour is undefined if a
value is unrepresentable (you can't determine how many items [*]scanf
found, since the value is garbage if it found an unrepresentable one).
But this shouldn't be fixed now.
% @@ -1027,8 +1028,8 @@
% continue;
% }
% if (streq(cp, "bytes/sector")) {
% - v = atoi(tp);
% - if (v <= 0 || (v % DEV_BSIZE) != 0) {
% + v = strtoul(tp, NULL, 0);
% + if (v == 0 || (v % DEV_BSIZE) != 0) {
% fprintf(stderr,
% "line %d: %s: bad sector size\n",
% lineno, tp);
All of the above had adequate ranges and bounds checking even with 16-bit
ints (modulo standard bugs in atoi() and sscanf()) and shouldn't be changed
now.
% @@ -1038,8 +1039,8 @@
% continue;
% }
% if (streq(cp, "sectors/track")) {
% - v = atoi(tp);
% - if (v <= 0) {
% + v = strtoul(tp, NULL, 0);
% + if (v == 0) {
% fprintf(stderr, "line %d: %s: bad %s\n",
% lineno, tp, cp);
% errors++;
% @@ -1048,8 +1049,8 @@
% continue;
% }
% if (streq(cp, "sectors/cylinder")) {
% - v = atoi(tp);
% - if (v <= 0) {
% + v = strtoul(tp, NULL, 0);
% + if (v == 0) {
% fprintf(stderr, "line %d: %s: bad %s\n",
% lineno, tp, cp);
% errors++;
% @@ -1058,8 +1059,8 @@
% continue;
% }
% if (streq(cp, "tracks/cylinder")) {
% - v = atoi(tp);
% - if (v <= 0) {
% + v = strtoul(tp, NULL, 0);
% + if (v == 0) {
% fprintf(stderr, "line %d: %s: bad %s\n",
% lineno, tp, cp);
% errors++;
These were adequate as above except they are missing checking of the upper
bound and some of them are getting close to the 32767 limit for 16-bit ints.
They shouldn't be changed now.
% @@ -1068,8 +1069,8 @@
% continue;
% }
% if (streq(cp, "cylinders")) {
% - v = atoi(tp);
% - if (v <= 0) {
% + v = strtoul(tp, NULL, 0);
% + if (v == 0) {
% fprintf(stderr, "line %d: %s: bad %s\n",
% lineno, tp, cp);
% errors++;
As above, except a limit of 32767 cylinders is definitely too small.
% @@ -1078,8 +1079,8 @@
% continue;
% }
% if (streq(cp, "sectors/unit")) {
% - v = atoi(tp);
% - if (v <= 0) {
% + v = strtoul(tp, NULL, 0);
% + if (v == 0) {
% fprintf(stderr, "line %d: %s: bad %s\n",
% lineno, tp, cp);
% errors++;
This needs changes since we want to support up to 2^32-1 sectors/unit.
I would use the above change, except with a new variable "u_long ulv;"
and s/v/ulv/, and a base of 10, and a check that ulv fits in u_int32_t.
% @@ -1088,8 +1089,8 @@
% continue;
% }
% if (streq(cp, "rpm")) {
% - v = atoi(tp);
% - if (v <= 0) {
% + v = strtoul(tp, NULL, 0);
% + if (v == 0) {
% fprintf(stderr, "line %d: %s: bad %s\n",
% lineno, tp, cp);
% errors++;
% @@ -1098,8 +1099,8 @@
% continue;
% }
% if (streq(cp, "interleave")) {
% - v = atoi(tp);
% - if (v <= 0) {
% + v = strtoul(tp, NULL, 0);
% + if (v == 0) {
% fprintf(stderr, "line %d: %s: bad %s\n",
% lineno, tp, cp);
% errors++;
% @@ -1108,43 +1109,23 @@
% continue;
% }
% if (streq(cp, "trackskew")) {
% - v = atoi(tp);
% - if (v < 0) {
% - fprintf(stderr, "line %d: %s: bad %s\n",
% - lineno, tp, cp);
% - errors++;
% - } else
% - lp->d_trackskew = v;
% + v = strtoul(tp, NULL, 0);
% + lp->d_trackskew = v;
% continue;
% }
% if (streq(cp, "cylinderskew")) {
% - v = atoi(tp);
% - if (v < 0) {
% - fprintf(stderr, "line %d: %s: bad %s\n",
% - lineno, tp, cp);
% - errors++;
% - } else
% - lp->d_cylskew = v;
% + v = strtoul(tp, NULL, 0);
% + lp->d_cylskew = v;
% continue;
% }
% if (streq(cp, "headswitch")) {
% - v = atoi(tp);
% - if (v < 0) {
% - fprintf(stderr, "line %d: %s: bad %s\n",
% - lineno, tp, cp);
% - errors++;
% - } else
% - lp->d_headswitch = v;
% + v = strtoul(tp, NULL, 0);
% + lp->d_headswitch = v;
% continue;
% }
% if (streq(cp, "track-to-track seek")) {
% - v = atoi(tp);
% - if (v < 0) {
% - fprintf(stderr, "line %d: %s: bad %s\n",
% - lineno, tp, cp);
% - errors++;
% - } else
% - lp->d_trkseek = v;
% + v = strtoul(tp, NULL, 0);
% + lp->d_trkseek = v;
% continue;
% }
% /* the ':' was removed above */
All as above (no changes needed case).
% @@ -1182,7 +1163,7 @@
% return (1); \
% } else { \
% cp = tp, tp = word(cp); \
% - (n) = atoi(cp); \
% + (n) = strtoul(cp, NULL, 0); \
% } \
% } while (0)
%
% @@ -1194,7 +1175,7 @@
% } else { \
% char *tmp; \
% cp = tp, tp = word(cp); \
% - (n) = strtol(cp,&tmp,10); \
% + (n) = strtoul(cp, &tmp, 10); \
% if (tmp) (w) = *tmp; \
% } \
% } while (0)
Macros get in the way of doing this right. Correct code might need a
different macro for every field to get the field type and bounds correct,
but macros are extremely unsuitable for this (in disklabel there would
be almost as many macros as fields).
Note that base 10 was used in the one previous call to the strtol() family.
% @@ -1209,14 +1190,14 @@
% struct partition *pp;
% char *cp;
% const char **cpp;
% - int v;
% + unsigned int v;
%
% pp = &lp->d_partitions[part];
% cp = NULL;
%
% v = 0;
% NXTWORD(part_size_type[part],v);
% - if (v < 0 || (v == 0 && part_size_type[part] != '*')) {
% + if (v == 0 && part_size_type[part] != '*') {
% fprintf(stderr, "line %d: %s: bad partition size\n", lineno,
% cp);
% return (1);
% @@ -1225,8 +1206,8 @@
%
% v = 0;
% NXTWORD(part_offset_type[part],v);
% - if (v < 0 || (v == 0 && part_offset_type[part] != '*' &&
% - part_offset_type[part] != '\0')) {
% + if (v == 0 && part_offset_type[part] != '*' &&
% + part_offset_type[part] != '\0') {
% fprintf(stderr, "line %d: %s: bad partition offset\n", lineno,
% cp);
% return (1);
These should use ulv, etc.
% @@ -1240,7 +1221,7 @@
% pp->p_fstype = cpp - fstypenames;
% } else {
% if (isdigit(*cp))
% - v = atoi(cp);
% + v = strtoul(cp, NULL, 0);
% else
% v = FSMAXTYPES;
% if ((unsigned)v >= FSMAXTYPES) {
No changes needed. The changes mainly give unintentional hex support and
a bogus cast to unsigned.
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20021117144918.J21008-100000>