diff mbox series

[v2,3/3] xfs_repair: catch strtol() errors

Message ID 20240417125937.917910-4-aalbersh@redhat.com (mailing list archive)
State Superseded, archived
Headers show
Series Refactoring and error handling from Coverity scan fixes | expand

Commit Message

Andrey Albershteyn April 17, 2024, 12:59 p.m. UTC 
strtol() sets errno if string parsing. Abort and tell user which
parameter is wrong.

Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 repair/xfs_repair.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

Comments

Darrick J. Wong April 17, 2024, 3:21 p.m. UTC | #1 
On Wed, Apr 17, 2024 at 02:59:37PM +0200, Andrey Albershteyn wrote:
> strtol() sets errno if string parsing. Abort and tell user which
> parameter is wrong.
> 
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  repair/xfs_repair.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index 2ceea87dc57d..2fc89dac345d 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -252,14 +252,22 @@ process_args(int argc, char **argv)
>  					if (!val)
>  						do_abort(
>  		_("-o bhash requires a parameter\n"));
> +					errno = 0;
>  					libxfs_bhash_size = (int)strtol(val, NULL, 0);
> +					if (errno)
> +						do_abort(
> +		_("-o bhash invalid parameter: %s\n"), strerror(errno));
>  					bhash_option_used = 1;
>  					break;
>  				case AG_STRIDE:
>  					if (!val)
>  						do_abort(
>  		_("-o ag_stride requires a parameter\n"));
> +					errno = 0;
>  					ag_stride = (int)strtol(val, NULL, 0);
> +					if (errno)
> +						do_abort(
> +		_("-o ag_stride invalid parameter: %s\n"), strerror(errno));
>  					break;
>  				case FORCE_GEO:
>  					if (val)
> @@ -272,19 +280,31 @@ process_args(int argc, char **argv)
>  					if (!val)
>  						do_abort(
>  		_("-o phase2_threads requires a parameter\n"));
> +					errno = 0;
>  					phase2_threads = (int)strtol(val, NULL, 0);
> +					if (errno)
> +						do_abort(
> +		_("-o phase2_threads invalid parameter: %s\n"), strerror(errno));
>  					break;
>  				case BLOAD_LEAF_SLACK:
>  					if (!val)
>  						do_abort(
>  		_("-o debug_bload_leaf_slack requires a parameter\n"));
> +					errno = 0;
>  					bload_leaf_slack = (int)strtol(val, NULL, 0);
> +					if (errno)
> +						do_abort(
> +		_("-o debug_bload_leaf_slack invalid parameter: %s\n"), strerror(errno));
>  					break;
>  				case BLOAD_NODE_SLACK:
>  					if (!val)
>  						do_abort(
>  		_("-o debug_bload_node_slack requires a parameter\n"));
> +					errno = 0;
>  					bload_node_slack = (int)strtol(val, NULL, 0);
> +					if (errno)
> +						do_abort(
> +		_("-o debug_bload_node_slack invalid parameter: %s\n"), strerror(errno));
>  					break;
>  				case NOQUOTA:
>  					quotacheck_skip();
> @@ -305,7 +325,11 @@ process_args(int argc, char **argv)
>  					if (!val)
>  						do_abort(
>  		_("-c lazycount requires a parameter\n"));
> +					errno = 0;
>  					lazy_count = (int)strtol(val, NULL, 0);
> +					if (errno)
> +						do_abort(
> +		_("-o lazycount invalid parameter: %s\n"), strerror(errno));
>  					convert_lazy_count = 1;
>  					break;
>  				case CONVERT_INOBTCOUNT:
> @@ -356,7 +380,11 @@ process_args(int argc, char **argv)
>  			if (bhash_option_used)
>  				do_abort(_("-m option cannot be used with "
>  						"-o bhash option\n"));
> +			errno = 0;
>  			max_mem_specified = strtol(optarg, NULL, 0);
> +			if (errno)
> +				do_abort(
> +		_("%s: invalid memory amount: %s\n"), optarg, strerror(errno));
>  			break;
>  		case 'L':
>  			zap_log = 1;
> @@ -377,7 +405,11 @@ process_args(int argc, char **argv)
>  			do_prefetch = 0;
>  			break;
>  		case 't':
> +			errno = 0;
>  			report_interval = strtol(optarg, NULL, 0);
> +			if (errno)
> +				do_abort(
> +		_("%s: invalid interval: %s\n"), optarg, strerror(errno));
>  			break;
>  		case 'e':
>  			report_corrected = true;
> @@ -397,8 +429,14 @@ process_args(int argc, char **argv)
>  		usage();
>  
>  	p = getenv("XFS_REPAIR_FAIL_AFTER_PHASE");
> -	if (p)
> +	if (p) {
> +		errno = 0;
>  		fail_after_phase = (int)strtol(p, NULL, 0);
> +		if (errno)
> +			do_abort(
> +		_("%s: invalid phase in XFS_REPAIR_FAIL_AFTER_PHASE: %s\n"),
> +				p, strerror(errno));
> +	}
>  }
>  
>  void __attribute__((noreturn))
> -- 
> 2.42.0
> 
>
Christoph Hellwig April 17, 2024, 3:22 p.m. UTC | #2 
Looks good:

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

Patch

diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 2ceea87dc57d..2fc89dac345d 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -252,14 +252,22 @@  process_args(int argc, char **argv)
 					if (!val)
 						do_abort(
 		_("-o bhash requires a parameter\n"));
+					errno = 0;
 					libxfs_bhash_size = (int)strtol(val, NULL, 0);
+					if (errno)
+						do_abort(
+		_("-o bhash invalid parameter: %s\n"), strerror(errno));
 					bhash_option_used = 1;
 					break;
 				case AG_STRIDE:
 					if (!val)
 						do_abort(
 		_("-o ag_stride requires a parameter\n"));
+					errno = 0;
 					ag_stride = (int)strtol(val, NULL, 0);
+					if (errno)
+						do_abort(
+		_("-o ag_stride invalid parameter: %s\n"), strerror(errno));
 					break;
 				case FORCE_GEO:
 					if (val)
@@ -272,19 +280,31 @@  process_args(int argc, char **argv)
 					if (!val)
 						do_abort(
 		_("-o phase2_threads requires a parameter\n"));
+					errno = 0;
 					phase2_threads = (int)strtol(val, NULL, 0);
+					if (errno)
+						do_abort(
+		_("-o phase2_threads invalid parameter: %s\n"), strerror(errno));
 					break;
 				case BLOAD_LEAF_SLACK:
 					if (!val)
 						do_abort(
 		_("-o debug_bload_leaf_slack requires a parameter\n"));
+					errno = 0;
 					bload_leaf_slack = (int)strtol(val, NULL, 0);
+					if (errno)
+						do_abort(
+		_("-o debug_bload_leaf_slack invalid parameter: %s\n"), strerror(errno));
 					break;
 				case BLOAD_NODE_SLACK:
 					if (!val)
 						do_abort(
 		_("-o debug_bload_node_slack requires a parameter\n"));
+					errno = 0;
 					bload_node_slack = (int)strtol(val, NULL, 0);
+					if (errno)
+						do_abort(
+		_("-o debug_bload_node_slack invalid parameter: %s\n"), strerror(errno));
 					break;
 				case NOQUOTA:
 					quotacheck_skip();
@@ -305,7 +325,11 @@  process_args(int argc, char **argv)
 					if (!val)
 						do_abort(
 		_("-c lazycount requires a parameter\n"));
+					errno = 0;
 					lazy_count = (int)strtol(val, NULL, 0);
+					if (errno)
+						do_abort(
+		_("-o lazycount invalid parameter: %s\n"), strerror(errno));
 					convert_lazy_count = 1;
 					break;
 				case CONVERT_INOBTCOUNT:
@@ -356,7 +380,11 @@  process_args(int argc, char **argv)
 			if (bhash_option_used)
 				do_abort(_("-m option cannot be used with "
 						"-o bhash option\n"));
+			errno = 0;
 			max_mem_specified = strtol(optarg, NULL, 0);
+			if (errno)
+				do_abort(
+		_("%s: invalid memory amount: %s\n"), optarg, strerror(errno));
 			break;
 		case 'L':
 			zap_log = 1;
@@ -377,7 +405,11 @@  process_args(int argc, char **argv)
 			do_prefetch = 0;
 			break;
 		case 't':
+			errno = 0;
 			report_interval = strtol(optarg, NULL, 0);
+			if (errno)
+				do_abort(
+		_("%s: invalid interval: %s\n"), optarg, strerror(errno));
 			break;
 		case 'e':
 			report_corrected = true;
@@ -397,8 +429,14 @@  process_args(int argc, char **argv)
 		usage();
 
 	p = getenv("XFS_REPAIR_FAIL_AFTER_PHASE");
-	if (p)
+	if (p) {
+		errno = 0;
 		fail_after_phase = (int)strtol(p, NULL, 0);
+		if (errno)
+			do_abort(
+		_("%s: invalid phase in XFS_REPAIR_FAIL_AFTER_PHASE: %s\n"),
+				p, strerror(errno));
+	}
 }
 
 void __attribute__((noreturn))