diff mbox series

[1/2] xfs_db: btheight should check geometry more carefully

Message ID 156944765385.303060.16945955453073433913.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs_db: fixes to the btheight command | expand

Commit Message

Darrick J. Wong Sept. 25, 2019, 9:40 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

The btheight command needs to check user-supplied geometry more
carefully so that we don't hit floating point exceptions.

Coverity-id: 1453661, 1453659
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/btheight.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 82 insertions(+), 6 deletions(-)

Comments

Carlos Maiolino Sept. 26, 2019, 9:11 a.m. UTC | #1 
On Wed, Sep 25, 2019 at 02:40:53PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The btheight command needs to check user-supplied geometry more
> carefully so that we don't hit floating point exceptions.
> 
> Coverity-id: 1453661, 1453659
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  

Patch looks good, but.


> +	if (record_size > blocksize) {
> +		fprintf(stderr,
> +			_("%s: record size must be less than %u bytes.\n"),

	Couldn't this message maybe be better saying "less than current blocksize"?
	Saying it is less than X bytes sounds kind of meaningless, requiring a
	trip to the code to understand what exactly 'bytes' mean here.

	Maybe something like:

	_("%s: record size must be less than current block size (%u).\n"),


Same for the next two.

> +			tag, blocksize);
> +		goto out;
> +	}
> +
> +	if (key_size > blocksize) {
> +		fprintf(stderr,
> +			_("%s: key size must be less than %u bytes.\n"),
> +			tag, blocksize);
> +		goto out;
> +	}
> +
> +	if (ptr_size > blocksize) {
> +		fprintf(stderr,
> +			_("%s: pointer size must be less than %u bytes.\n"),
> +			tag, blocksize);
> +		goto out;
> +	}
> +
>  	p = strtok(NULL, ":");
>  	if (p) {
>  		fprintf(stderr,
> @@ -211,13 +244,24 @@ report(
>  	int			ret;
>  
>  	ret = construct_records_per_block(tag, blocksize, records_per_block);
> -	if (ret) {
> -		printf(_("%s: Unable to determine records per block.\n"),
> -				tag);
> +	if (ret)
>  		return;
> -	}
>  
>  	if (report_what & REPORT_MAX) {
> +		if (records_per_block[0] < 2) {
> +			fprintf(stderr,
> +_("%s: cannot calculate best case scenario due to leaf geometry underflow.\n"),
> +				tag);
> +			return;
> +		}
> +
> +		if (records_per_block[1] < 4) {
> +			fprintf(stderr,
> +_("%s: cannot calculate best case scenario due to node geometry underflow.\n"),
> +				tag);
> +			return;
> +		}
> +
>  		printf(
>  _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
>  				tag, blocksize, records_per_block[0],
> @@ -230,6 +274,20 @@ _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
>  		records_per_block[0] /= 2;
>  		records_per_block[1] /= 2;
>  
> +		if (records_per_block[0] < 1) {
> +			fprintf(stderr,
> +_("%s: cannot calculate worst case scenario due to leaf geometry underflow.\n"),
> +				tag);
> +			return;
> +		}
> +
> +		if (records_per_block[1] < 2) {
> +			fprintf(stderr,
> +_("%s: cannot calculate worst case scenario due to node geometry underflow.\n"),
> +				tag);
> +			return;
> +		}
> +
>  		printf(
>  _("%s: worst case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
>  				tag, blocksize, records_per_block[0],
> @@ -284,8 +342,26 @@ btheight_f(
>  		}
>  	}
>  
> -	if (argc == optind || blocksize <= 0 || blocksize > INT_MAX ||
> -	    nr_records == 0) {
> +	if (nr_records == 0) {
> +		fprintf(stderr,
> +_("Number of records must be greater than zero.\n"));
> +		return 0;
> +	}
> +
> +	if (blocksize > INT_MAX) {
> +		fprintf(stderr,
> +_("The largest block size this command will consider is %u bytes.\n"),
> +			INT_MAX);
> +		return 0;
> +	}
> +
> +	if (blocksize < 128) {
> +		fprintf(stderr,
> +_("The smallest block size this command will consider is 128 bytes.\n"));
> +		return 0;
> +	}
> +
> +	if (argc == optind) {
>  		btheight_help();
>  		return 0;
>  	}
>
Darrick J. Wong Sept. 26, 2019, 5:38 p.m. UTC | #2 
On Thu, Sep 26, 2019 at 11:11:48AM +0200, Carlos Maiolino wrote:
> On Wed, Sep 25, 2019 at 02:40:53PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > The btheight command needs to check user-supplied geometry more
> > carefully so that we don't hit floating point exceptions.
> > 
> > Coverity-id: 1453661, 1453659
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  
> 
> Patch looks good, but.
> 
> 
> > +	if (record_size > blocksize) {
> > +		fprintf(stderr,
> > +			_("%s: record size must be less than %u bytes.\n"),
> 
> 	Couldn't this message maybe be better saying "less than current blocksize"?
> 	Saying it is less than X bytes sounds kind of meaningless, requiring a
> 	trip to the code to understand what exactly 'bytes' mean here.
> 
> 	Maybe something like:
> 
> 	_("%s: record size must be less than current block size (%u).\n"),

I think I'll change that to 'selected' from 'current' since the caller
can change the block size with -b, but otherwise I agree.

--D

> 
> Same for the next two.
> 
> > +			tag, blocksize);
> > +		goto out;
> > +	}
> > +
> > +	if (key_size > blocksize) {
> > +		fprintf(stderr,
> > +			_("%s: key size must be less than %u bytes.\n"),
> > +			tag, blocksize);
> > +		goto out;
> > +	}
> > +
> > +	if (ptr_size > blocksize) {
> > +		fprintf(stderr,
> > +			_("%s: pointer size must be less than %u bytes.\n"),
> > +			tag, blocksize);
> > +		goto out;
> > +	}
> > +
> >  	p = strtok(NULL, ":");
> >  	if (p) {
> >  		fprintf(stderr,
> > @@ -211,13 +244,24 @@ report(
> >  	int			ret;
> >  
> >  	ret = construct_records_per_block(tag, blocksize, records_per_block);
> > -	if (ret) {
> > -		printf(_("%s: Unable to determine records per block.\n"),
> > -				tag);
> > +	if (ret)
> >  		return;
> > -	}
> >  
> >  	if (report_what & REPORT_MAX) {
> > +		if (records_per_block[0] < 2) {
> > +			fprintf(stderr,
> > +_("%s: cannot calculate best case scenario due to leaf geometry underflow.\n"),
> > +				tag);
> > +			return;
> > +		}
> > +
> > +		if (records_per_block[1] < 4) {
> > +			fprintf(stderr,
> > +_("%s: cannot calculate best case scenario due to node geometry underflow.\n"),
> > +				tag);
> > +			return;
> > +		}
> > +
> >  		printf(
> >  _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
> >  				tag, blocksize, records_per_block[0],
> > @@ -230,6 +274,20 @@ _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
> >  		records_per_block[0] /= 2;
> >  		records_per_block[1] /= 2;
> >  
> > +		if (records_per_block[0] < 1) {
> > +			fprintf(stderr,
> > +_("%s: cannot calculate worst case scenario due to leaf geometry underflow.\n"),
> > +				tag);
> > +			return;
> > +		}
> > +
> > +		if (records_per_block[1] < 2) {
> > +			fprintf(stderr,
> > +_("%s: cannot calculate worst case scenario due to node geometry underflow.\n"),
> > +				tag);
> > +			return;
> > +		}
> > +
> >  		printf(
> >  _("%s: worst case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
> >  				tag, blocksize, records_per_block[0],
> > @@ -284,8 +342,26 @@ btheight_f(
> >  		}
> >  	}
> >  
> > -	if (argc == optind || blocksize <= 0 || blocksize > INT_MAX ||
> > -	    nr_records == 0) {
> > +	if (nr_records == 0) {
> > +		fprintf(stderr,
> > +_("Number of records must be greater than zero.\n"));
> > +		return 0;
> > +	}
> > +
> > +	if (blocksize > INT_MAX) {
> > +		fprintf(stderr,
> > +_("The largest block size this command will consider is %u bytes.\n"),
> > +			INT_MAX);
> > +		return 0;
> > +	}
> > +
> > +	if (blocksize < 128) {
> > +		fprintf(stderr,
> > +_("The smallest block size this command will consider is 128 bytes.\n"));
> > +		return 0;
> > +	}
> > +
> > +	if (argc == optind) {
> >  		btheight_help();
> >  		return 0;
> >  	}
> > 
> 
> -- 
> Carlos
>
Carlos Maiolino Sept. 27, 2019, 12:30 p.m. UTC | #3 
On Thu, Sep 26, 2019 at 10:38:43AM -0700, Darrick J. Wong wrote:
> On Thu, Sep 26, 2019 at 11:11:48AM +0200, Carlos Maiolino wrote:
> > On Wed, Sep 25, 2019 at 02:40:53PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > The btheight command needs to check user-supplied geometry more
> > > carefully so that we don't hit floating point exceptions.
> > > 
> > > Coverity-id: 1453661, 1453659
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  
> > 
> > Patch looks good, but.
> > 
> > 
> > > +	if (record_size > blocksize) {
> > > +		fprintf(stderr,
> > > +			_("%s: record size must be less than %u bytes.\n"),
> > 
> > 	Couldn't this message maybe be better saying "less than current blocksize"?
> > 	Saying it is less than X bytes sounds kind of meaningless, requiring a
> > 	trip to the code to understand what exactly 'bytes' mean here.
> > 
> > 	Maybe something like:
> > 
> > 	_("%s: record size must be less than current block size (%u).\n"),
> 
> I think I'll change that to 'selected' from 'current' since the caller
> can change the block size with -b, but otherwise I agree.

Sounds good o me too, thanks

> 
> --D
> 
> > 
> > Same for the next two.
> > 
> > > +			tag, blocksize);
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (key_size > blocksize) {
> > > +		fprintf(stderr,
> > > +			_("%s: key size must be less than %u bytes.\n"),
> > > +			tag, blocksize);
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (ptr_size > blocksize) {
> > > +		fprintf(stderr,
> > > +			_("%s: pointer size must be less than %u bytes.\n"),
> > > +			tag, blocksize);
> > > +		goto out;
> > > +	}
> > > +
> > >  	p = strtok(NULL, ":");
> > >  	if (p) {
> > >  		fprintf(stderr,
> > > @@ -211,13 +244,24 @@ report(
> > >  	int			ret;
> > >  
> > >  	ret = construct_records_per_block(tag, blocksize, records_per_block);
> > > -	if (ret) {
> > > -		printf(_("%s: Unable to determine records per block.\n"),
> > > -				tag);
> > > +	if (ret)
> > >  		return;
> > > -	}
> > >  
> > >  	if (report_what & REPORT_MAX) {
> > > +		if (records_per_block[0] < 2) {
> > > +			fprintf(stderr,
> > > +_("%s: cannot calculate best case scenario due to leaf geometry underflow.\n"),
> > > +				tag);
> > > +			return;
> > > +		}
> > > +
> > > +		if (records_per_block[1] < 4) {
> > > +			fprintf(stderr,
> > > +_("%s: cannot calculate best case scenario due to node geometry underflow.\n"),
> > > +				tag);
> > > +			return;
> > > +		}
> > > +
> > >  		printf(
> > >  _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
> > >  				tag, blocksize, records_per_block[0],
> > > @@ -230,6 +274,20 @@ _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
> > >  		records_per_block[0] /= 2;
> > >  		records_per_block[1] /= 2;
> > >  
> > > +		if (records_per_block[0] < 1) {
> > > +			fprintf(stderr,
> > > +_("%s: cannot calculate worst case scenario due to leaf geometry underflow.\n"),
> > > +				tag);
> > > +			return;
> > > +		}
> > > +
> > > +		if (records_per_block[1] < 2) {
> > > +			fprintf(stderr,
> > > +_("%s: cannot calculate worst case scenario due to node geometry underflow.\n"),
> > > +				tag);
> > > +			return;
> > > +		}
> > > +
> > >  		printf(
> > >  _("%s: worst case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
> > >  				tag, blocksize, records_per_block[0],
> > > @@ -284,8 +342,26 @@ btheight_f(
> > >  		}
> > >  	}
> > >  
> > > -	if (argc == optind || blocksize <= 0 || blocksize > INT_MAX ||
> > > -	    nr_records == 0) {
> > > +	if (nr_records == 0) {
> > > +		fprintf(stderr,
> > > +_("Number of records must be greater than zero.\n"));
> > > +		return 0;
> > > +	}
> > > +
> > > +	if (blocksize > INT_MAX) {
> > > +		fprintf(stderr,
> > > +_("The largest block size this command will consider is %u bytes.\n"),
> > > +			INT_MAX);
> > > +		return 0;
> > > +	}
> > > +
> > > +	if (blocksize < 128) {
> > > +		fprintf(stderr,
> > > +_("The smallest block size this command will consider is 128 bytes.\n"));
> > > +		return 0;
> > > +	}
> > > +
> > > +	if (argc == optind) {
> > >  		btheight_help();
> > >  		return 0;
> > >  	}
> > > 
> > 
> > -- 
> > Carlos
> >
Christoph Hellwig Sept. 30, 2019, 7:55 a.m. UTC | #4 
Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/db/btheight.c b/db/btheight.c
index 289e5d84..e2c9759f 100644
--- a/db/btheight.c
+++ b/db/btheight.c
@@ -138,6 +138,10 @@  construct_records_per_block(
 		perror(p);
 		goto out;
 	}
+	if (record_size == 0) {
+		fprintf(stderr, _("%s: record size cannot be zero.\n"), tag);
+		goto out;
+	}
 
 	p = strtok(NULL, ":");
 	if (!p) {
@@ -149,6 +153,10 @@  construct_records_per_block(
 		perror(p);
 		goto out;
 	}
+	if (key_size == 0) {
+		fprintf(stderr, _("%s: key size cannot be zero.\n"), tag);
+		goto out;
+	}
 
 	p = strtok(NULL, ":");
 	if (!p) {
@@ -160,6 +168,10 @@  construct_records_per_block(
 		perror(p);
 		goto out;
 	}
+	if (ptr_size == 0) {
+		fprintf(stderr, _("%s: pointer size cannot be zero.\n"), tag);
+		goto out;
+	}
 
 	p = strtok(NULL, ":");
 	if (!p) {
@@ -180,6 +192,27 @@  construct_records_per_block(
 		goto out;
 	}
 
+	if (record_size > blocksize) {
+		fprintf(stderr,
+			_("%s: record size must be less than %u bytes.\n"),
+			tag, blocksize);
+		goto out;
+	}
+
+	if (key_size > blocksize) {
+		fprintf(stderr,
+			_("%s: key size must be less than %u bytes.\n"),
+			tag, blocksize);
+		goto out;
+	}
+
+	if (ptr_size > blocksize) {
+		fprintf(stderr,
+			_("%s: pointer size must be less than %u bytes.\n"),
+			tag, blocksize);
+		goto out;
+	}
+
 	p = strtok(NULL, ":");
 	if (p) {
 		fprintf(stderr,
@@ -211,13 +244,24 @@  report(
 	int			ret;
 
 	ret = construct_records_per_block(tag, blocksize, records_per_block);
-	if (ret) {
-		printf(_("%s: Unable to determine records per block.\n"),
-				tag);
+	if (ret)
 		return;
-	}
 
 	if (report_what & REPORT_MAX) {
+		if (records_per_block[0] < 2) {
+			fprintf(stderr,
+_("%s: cannot calculate best case scenario due to leaf geometry underflow.\n"),
+				tag);
+			return;
+		}
+
+		if (records_per_block[1] < 4) {
+			fprintf(stderr,
+_("%s: cannot calculate best case scenario due to node geometry underflow.\n"),
+				tag);
+			return;
+		}
+
 		printf(
 _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
 				tag, blocksize, records_per_block[0],
@@ -230,6 +274,20 @@  _("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
 		records_per_block[0] /= 2;
 		records_per_block[1] /= 2;
 
+		if (records_per_block[0] < 1) {
+			fprintf(stderr,
+_("%s: cannot calculate worst case scenario due to leaf geometry underflow.\n"),
+				tag);
+			return;
+		}
+
+		if (records_per_block[1] < 2) {
+			fprintf(stderr,
+_("%s: cannot calculate worst case scenario due to node geometry underflow.\n"),
+				tag);
+			return;
+		}
+
 		printf(
 _("%s: worst case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
 				tag, blocksize, records_per_block[0],
@@ -284,8 +342,26 @@  btheight_f(
 		}
 	}
 
-	if (argc == optind || blocksize <= 0 || blocksize > INT_MAX ||
-	    nr_records == 0) {
+	if (nr_records == 0) {
+		fprintf(stderr,
+_("Number of records must be greater than zero.\n"));
+		return 0;
+	}
+
+	if (blocksize > INT_MAX) {
+		fprintf(stderr,
+_("The largest block size this command will consider is %u bytes.\n"),
+			INT_MAX);
+		return 0;
+	}
+
+	if (blocksize < 128) {
+		fprintf(stderr,
+_("The smallest block size this command will consider is 128 bytes.\n"));
+		return 0;
+	}
+
+	if (argc == optind) {
 		btheight_help();
 		return 0;
 	}