Re: Sign fixes for disklabel(8)
On Fri, 15 Nov 2002, Nate Lawson wrote:
On Sat, 16 Nov 2002, Tim Robbins wrote:
On Fri, Nov 15, 2002 at 03:59:25PM -0800, Julian Elischer wrote:
Here are the diffs to allow disklabel to correctly create partitions
1TB (up to 2TB is useful with UFS2) pending a different partitionning
scheme. It also allows you to correctly make smaller partitions beyond
1TB which is nice if you don't want to waste 800GB on an array :-)
permission to commit please?
(pending comments by others)
(also the sysinstall changes posted before)
(also the bluetooth code)
[...]
- v = atoi(tp);
+ v = strtoul(tp, NULL, 0);
if (v = 0 || (v % DEV_BSIZE) != 0) {
fprintf(stderr,
line %d: %s: bad sector size\n,
Should be == 0, not = 0 since v is unsigned.
good catch.
I've altered all the checks of v to be unsigned..
[...]
- v = atoi(tp);
+ v = strtoul(tp, NULL, 0);
if (v 0) {
fprintf(stderr, line %d: %s: bad %s\n,
lineno, tp, cp);
v 0 is impossible.
In the overflow case, strtoul returns ULONG_MAX. Or if you're interested
in catching invalid characters, use endptr.
I'm not that interested in catching those cases.. they were not caught
before and I'm looking for the minimal diff..
I've just removed the clause. See new patch attached..
A sensible thing might be to compare against a sensible value
but who knows what that value should be?
I include the new diff for your coments
Any comments from anyone about the sysinstall fixes would also be
apreciated..
-Nate
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
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 - 1.62
+++ disklabel.c 16 Nov 2002 08:23:14 -
@@ -943,7 +943,8 @@
const char **cpp;
unsigned int part;
char *tp, line[BUFSIZ];
- int v, lineno = 0, errors = 0;
+ unsigned int v;
+ int lineno = 0, errors = 0;
int i;
lp-d_bbsize = BBSIZE; /* XXX */
@@ -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);
@@ -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;
@@ -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);
@@ -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
Re: Sign fixes for disklabel(8)
On Fri, 15 Nov 2002, Julian Elischer wrote: (patches not copied) also two now unneeded occurrences of (unsigned)v replaced by v To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Sign fixes for disklabel(8)
On Sat, 16 Nov 2002, Tim Robbins wrote:
On Fri, Nov 15, 2002 at 03:59:25PM -0800, Julian Elischer wrote:
Here are the diffs to allow disklabel to correctly create partitions
1TB (up to 2TB is useful with UFS2) pending a different partitionning
scheme. It also allows you to correctly make smaller partitions beyond
1TB which is nice if you don't want to waste 800GB on an array :-)
Labels should be able to handle disks much larger than 1TB (under -current
where there is no significant daddr_t limit) by fudging the sector size.
E.g., with a sector size of 8K should work up to 16TB (or 8TB without your
fixes) without even requiring larger ffs block sizes than the default of
16K. (I used 8K instead of 16K in this example because ffs has problems
reading the superblock starting at 8K.)
[...]
- int v, lineno = 0, errors = 0;
+ unsigned int v;
Style bug: unsigned int is spelled u_int everywhere else in disklabel.c
except in one place (which shouldn't even use an unsigned type).
Badly chosen type: 'v' is general purpose; it should have type u_long if
anything, though it wants to be uint32_t for sector counts.
+ int lineno = 0, errors = 0;
int i;
[...]
- v = atoi(tp);
+ v = strtoul(tp, NULL, 0);
This code is almost as low-quality as before. The following errors are
not checked for:
- garbage input
- overflow inside strtoul()
- overflow on assignment of the result of strtoul() to v (since we chose
a poor type for v). The old poorly chosen type was at least suitable
for holding the result of atoi().
if ((unsigned)v = DKMAXTYPES)
fprintf(stderr, line %d:%s %d\n, lineno,
Warning, unknown disk type, v);
This cast is redundant since v is already unsigned. The fprintf() format
string needs to use %u instead of %d.
But v really wants to be a plain int here.
Use 10 as the base argument to
stroul(), not 0 otherwise numbers with leading zeros will be interpreted
as octal.
[...]
- v = atoi(tp);
+ v = strtoul(tp, NULL, 0);
if (v = 0 || (v % DEV_BSIZE) != 0) {
fprintf(stderr,
line %d: %s: bad sector size\n,
Should be == 0, not = 0 since v is unsigned.
[...]
- v = atoi(tp);
+ v = strtoul(tp, NULL, 0);
if (v 0) {
fprintf(stderr, line %d: %s: bad %s\n,
lineno, tp, cp);
v 0 is impossible.
These v's really want to be plain ints too. The old code was correct for
all these v's, modulo the usual sloppy parsing giving by atoi().
The sloppy parsing shouldn't be fixed now. Just use a different variable
and different parsing for the sector counts.
Bruce
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Re: Sign fixes for disklabel(8)
On Sat, 16 Nov 2002, Julian Elischer wrote: On Fri, 15 Nov 2002, Nate Lawson wrote: In the overflow case, strtoul returns ULONG_MAX. Or if you're interested in catching invalid characters, use endptr. I'm not that interested in catching those cases.. they were not caught before and I'm looking for the minimal diff.. I've just removed the clause. See new patch attached.. 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). 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. 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.). Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Sign fixes for disklabel(8)
On Sat, 16 Nov 2002, Bruce Evans wrote:
On Sat, 16 Nov 2002, Tim Robbins wrote:
On Fri, Nov 15, 2002 at 03:59:25PM -0800, Julian Elischer wrote:
Here are the diffs to allow disklabel to correctly create partitions
1TB (up to 2TB is useful with UFS2) pending a different partitionning
scheme. It also allows you to correctly make smaller partitions beyond
1TB which is nice if you don't want to waste 800GB on an array :-)
Labels should be able to handle disks much larger than 1TB (under -current
where there is no significant daddr_t limit) by fudging the sector size.
E.g., with a sector size of 8K should work up to 16TB (or 8TB without your
fixes) without even requiring larger ffs block sizes than the default of
16K. (I used 8K instead of 16K in this example because ffs has problems
reading the superblock starting at 8K.)
Unfortunatly the fdisk label then becomes a problem for compatible
machines. The standard for ia64 machines looks like being hte way to go
and we already have tools a code to handle it..
[...]
- int v, lineno = 0, errors = 0;
+ unsigned int v;
Style bug: unsigned int is spelled u_int everywhere else in disklabel.c
is it compulsary? unsigned int is standard C.
u_int is used in exactly one place in disklabel.c where
unsigned occured in many so to maintain style I should change the one
u_int to unsigned int.
I'm happy to change to u_int though. (it's shorter)
except in one place (which shouldn't even use an unsigned type).
where?
Badly chosen type: 'v' is general purpose; it should have type u_long if
anything, though it wants to be uint32_t for sector counts.
do you mind if I don't rewrite the whole program?
'v' works so I'll leave it.
The aim is not to fix every bde inthe program but to be able to
partition a 2TB drive.
+ int lineno = 0, errors = 0;
int i;
[...]
- v = atoi(tp);
+ v = strtoul(tp, NULL, 0);
This code is almost as low-quality as before. The following errors are
not checked for:
almost but not worse.. and now it works, for the criteria I'm
looking for (2TB drives)..
- garbage input
- overflow inside strtoul()
- overflow on assignment of the result of strtoul() to v (since we chose
a poor type for v). The old poorly chosen type was at least suitable
for holding the result of atoi().
The new one is suitable for strtoul() which is alll that is used now.
if ((unsigned)v = DKMAXTYPES)
fprintf(stderr, line %d:%s %d\n, lineno,
Warning, unknown disk type, v);
This cast is redundant since v is already unsigned. The fprintf() format
string needs to use %u instead of %d.
But v really wants to be a plain int here.
why? all teh types are +ve and lp-d_type is unsigned.
Use 10 as the base argument to
stroul(), not 0 otherwise numbers with leading zeros will be interpreted
as octal.
done
(see next email for new diff)
[...]
- v = atoi(tp);
+ v = strtoul(tp, NULL, 0);
if (v = 0 || (v % DEV_BSIZE) != 0) {
fprintf(stderr,
line %d: %s: bad sector size\n,
Should be == 0, not = 0 since v is unsigned.
[...]
- v = atoi(tp);
+ v = strtoul(tp, NULL, 0);
if (v 0) {
fprintf(stderr, line %d: %s: bad %s\n,
lineno, tp, cp);
v 0 is impossible.
These v's really want to be plain ints too. The old code was correct for
all these v's, modulo the usual sloppy parsing giving by atoi().
The sloppy parsing shouldn't be fixed now. Just use a different variable
and different parsing for the sector counts.
sector counts can not be -ve so it should be unsigned..
I may substitute a u_int16_t variable for some.
Bruce
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Re: Sign fixes for disklabel(8)
On Sat, 16 Nov 2002, Bruce Evans wrote:
On Sat, 16 Nov 2002, Julian Elischer wrote:
On Fri, 15 Nov 2002, Nate Lawson wrote:
In the overflow case, strtoul returns ULONG_MAX. Or if you're interested
in catching invalid characters, use endptr.
I'm not that interested in catching those cases.. they were not caught
before and I'm looking for the minimal diff..
I've just removed the clause. See new patch attached..
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.
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.
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..
Ok here's a diff
Bruce
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Index: disklabel.c
===
RCS file: /usr/cvs/src/sbin/disklabel/disklabel.c,v
retrieving revision 1.62
diff -u -r1.62 disklabel.c
--- disklabel.c 8 Oct 2002 12:13:19 - 1.62
+++ disklabel.c 16 Nov 2002 22:18:27 -
@@ -732,7 +732,7 @@
const struct partition *pp;
fprintf(f, # %s:\n, specname);
- if ((unsigned) lp-d_type DKMAXTYPES)
+ if (lp-d_type DKMAXTYPES)
fprintf(f, type: %s\n, dktypenames[lp-d_type]);
else
fprintf(f, type: %u\n, lp-d_type);
@@ -778,7 +778,7 @@
if (pp-p_size) {
fprintf(f, %c: %8lu %8lu , 'a' + i,
(u_long)pp-p_size, (u_long)pp-p_offset);
- if ((unsigned) pp-p_fstype FSMAXTYPES)
+ if (pp-p_fstype FSMAXTYPES)
fprintf(f, %8.8s, fstypenames[pp-p_fstype]);
else
fprintf(f, %8d, pp-p_fstype);
@@ -941,9 +941,10 @@
{
char *cp;
const char **cpp;
- unsigned int part;
+ u_int part;
char *tp, line[BUFSIZ];
- int v, lineno = 0, errors = 0;
+ u_int v;
+ int lineno = 0, errors = 0;
int i;
lp-d_bbsize = BBSIZE; /* XXX */
@@ -973,8 +974,8 @@
}
if (cpp dktypenames[DKMAXTYPES])
continue;
- v = atoi(tp);
- if ((unsigned)v = DKMAXTYPES)
+ v = strtoul(tp, NULL, 10);
+ if (v = DKMAXTYPES)
fprintf(stderr, line %d:%s %d\n, lineno,
Warning, unknown disk type, v);
lp-d_type = v;
@@ -1001,13 +1002,13 @@
}
if (streq(cp, drivedata)) {
for (i = 0; (cp = tp) *cp != '\0' i NDDATA;) {
- lp-d_drivedata[i++] = atoi(cp);
+ lp-d_drivedata[i++] = strtoul(cp, NULL, 10);
tp = word(cp);
}
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;
@@ -1027,8 +1028,8 @@
continue;
}
if (streq(cp, bytes/sector)) {
- v = atoi(tp);
-
Re: Sign fixes for disklabel(8)
* De: Julian Elischer [EMAIL PROTECTED] [ Data: 2002-11-16 ] [ Subjecte: Re: Sign fixes for disklabel(8) ] + u_int v; [...] + v = strtoul(tp, NULL, 10); strtoul returns 'unsigned long', not 'unsigned int', and this can overflow. The type of 'v' is poorly chosen, as bde said. Yes it was fine for 'atoi', but 'atoi' is not 'strtoul'. -- Juli Mallett [EMAIL PROTECTED] OpenDarwin, Mono, FreeBSD Developer. ircd-hybrid Developer, EFnet addict. FreeBSD on MIPS-Anything on FreeBSD. To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: Sign fixes for disklabel(8)
On Fri, Nov 15, 2002 at 03:59:25PM -0800, Julian Elischer wrote:
Here are the diffs to allow disklabel to correctly create partitions
1TB (up to 2TB is useful with UFS2) pending a different partitionning
scheme. It also allows you to correctly make smaller partitions beyond
1TB which is nice if you don't want to waste 800GB on an array :-)
permission to commit please?
(pending comments by others)
(also the sysinstall changes posted before)
(also the bluetooth code)
[...]
- int v, lineno = 0, errors = 0;
+ unsigned int v;
+ int lineno = 0, errors = 0;
int i;
[...]
- 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);
This cast is redundant since v is already unsigned. The fprintf() format
string needs to use %u instead of %d. Use 10 as the base argument to
stroul(), not 0 otherwise numbers with leading zeros will be interpreted
as octal.
[...]
- v = atoi(tp);
+ v = strtoul(tp, NULL, 0);
if (v = 0 || (v % DEV_BSIZE) != 0) {
fprintf(stderr,
line %d: %s: bad sector size\n,
Should be == 0, not = 0 since v is unsigned.
[...]
- v = atoi(tp);
+ v = strtoul(tp, NULL, 0);
if (v 0) {
fprintf(stderr, line %d: %s: bad %s\n,
lineno, tp, cp);
v 0 is impossible.
Tim
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
Re: Sign fixes for disklabel(8)
On Sat, 16 Nov 2002, Tim Robbins wrote:
On Fri, Nov 15, 2002 at 03:59:25PM -0800, Julian Elischer wrote:
Here are the diffs to allow disklabel to correctly create partitions
1TB (up to 2TB is useful with UFS2) pending a different partitionning
scheme. It also allows you to correctly make smaller partitions beyond
1TB which is nice if you don't want to waste 800GB on an array :-)
permission to commit please?
(pending comments by others)
(also the sysinstall changes posted before)
(also the bluetooth code)
[...]
- v = atoi(tp);
+ v = strtoul(tp, NULL, 0);
if (v = 0 || (v % DEV_BSIZE) != 0) {
fprintf(stderr,
line %d: %s: bad sector size\n,
Should be == 0, not = 0 since v is unsigned.
[...]
- v = atoi(tp);
+ v = strtoul(tp, NULL, 0);
if (v 0) {
fprintf(stderr, line %d: %s: bad %s\n,
lineno, tp, cp);
v 0 is impossible.
In the overflow case, strtoul returns ULONG_MAX. Or if you're interested
in catching invalid characters, use endptr.
-Nate
To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message
