diff mbox series

[4/5] xfsprogs: fix sliently borken option parsing

Message ID 20200324001928.17894-5-david@fromorbit.com (mailing list archive)
State New, archived
Headers show
Series xfsprogs: miscellenaous patches | expand

Commit Message

Dave Chinner March 24, 2020, 12:19 a.m. UTC 
From: Dave Chinner <dchinner@redhat.com>

When getopt() is passed an option string like "-m -n" and the
parameter m is defined as "m:", getopt returns a special error
to indication that the optstring started with a "-". Any getopt()
caller that is just catching the "?" error character will not
not catch this special error, so it silently eats the parameter
following -m.

Lots of getopt loops in xfsprogs have this issue. Convert them all
to just use a "default:" to catch anything unexpected.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 copy/xfs_copy.c      | 2 +-
 db/freesp.c          | 2 +-
 db/init.c            | 7 ++-----
 growfs/xfs_growfs.c  | 1 -
 io/copy_file_range.c | 2 ++
 logprint/logprint.c  | 2 +-
 mkfs/xfs_mkfs.c      | 2 +-
 repair/xfs_repair.c  | 2 +-
 scrub/xfs_scrub.c    | 2 --
 spaceman/freesp.c    | 1 -
 spaceman/prealloc.c  | 1 -
 11 files changed, 9 insertions(+), 15 deletions(-)

Comments

Christoph Hellwig March 24, 2020, 8:44 a.m. UTC | #1 
On Tue, Mar 24, 2020 at 11:19:27AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When getopt() is passed an option string like "-m -n" and the
> parameter m is defined as "m:", getopt returns a special error
> to indication that the optstring started with a "-". Any getopt()
> caller that is just catching the "?" error character will not
> not catch this special error, so it silently eats the parameter
> following -m.
> 
> Lots of getopt loops in xfsprogs have this issue. Convert them all
> to just use a "default:" to catch anything unexpected.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong March 24, 2020, 8:44 p.m. UTC | #2 
On Tue, Mar 24, 2020 at 11:19:27AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When getopt() is passed an option string like "-m -n" and the
> parameter m is defined as "m:", getopt returns a special error
> to indication that the optstring started with a "-". Any getopt()
> caller that is just catching the "?" error character will not
> not catch this special error, so it silently eats the parameter
> following -m.
> 
> Lots of getopt loops in xfsprogs have this issue. Convert them all
> to just use a "default:" to catch anything unexpected.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Ooops....
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  copy/xfs_copy.c      | 2 +-
>  db/freesp.c          | 2 +-
>  db/init.c            | 7 ++-----
>  growfs/xfs_growfs.c  | 1 -
>  io/copy_file_range.c | 2 ++
>  logprint/logprint.c  | 2 +-
>  mkfs/xfs_mkfs.c      | 2 +-
>  repair/xfs_repair.c  | 2 +-
>  scrub/xfs_scrub.c    | 2 --
>  spaceman/freesp.c    | 1 -
>  spaceman/prealloc.c  | 1 -
>  11 files changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
> index 91c2ae01683b..c4f9f34981ca 100644
> --- a/copy/xfs_copy.c
> +++ b/copy/xfs_copy.c
> @@ -584,7 +584,7 @@ main(int argc, char **argv)
>  		case 'V':
>  			printf(_("%s version %s\n"), progname, VERSION);
>  			exit(0);
> -		case '?':
> +		default:
>  			usage();
>  		}
>  	}
> diff --git a/db/freesp.c b/db/freesp.c
> index 903c60d7380a..6f2346665847 100644
> --- a/db/freesp.c
> +++ b/db/freesp.c
> @@ -177,7 +177,7 @@ init(
>  		case 's':
>  			summaryflag = 1;
>  			break;
> -		case '?':
> +		default:
>  			return usage();
>  		}
>  	}
> diff --git a/db/init.c b/db/init.c
> index 61eea111f017..ac649fbddbb9 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -84,15 +84,12 @@ init(
>  		case 'V':
>  			printf(_("%s version %s\n"), progname, VERSION);
>  			exit(0);
> -		case '?':
> +		default:
>  			usage();
> -			/*NOTREACHED*/
>  		}
>  	}
> -	if (optind + 1 != argc) {
> +	if (optind + 1 != argc)
>  		usage();
> -		/*NOTREACHED*/
> -	}
>  
>  	fsdevice = argv[optind];
>  	if (!x.disfile)
> diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
> index d27e3b94e0c4..a68b515de40d 100644
> --- a/growfs/xfs_growfs.c
> +++ b/growfs/xfs_growfs.c
> @@ -120,7 +120,6 @@ main(int argc, char **argv)
>  		case 'V':
>  			printf(_("%s version %s\n"), progname, VERSION);
>  			exit(0);
> -		case '?':
>  		default:
>  			usage();
>  		}
> diff --git a/io/copy_file_range.c b/io/copy_file_range.c
> index fb5702e1faad..4c4332c6e5ec 100644
> --- a/io/copy_file_range.c
> +++ b/io/copy_file_range.c
> @@ -127,6 +127,8 @@ copy_range_f(int argc, char **argv)
>  			/* Expect no src_path arg */
>  			src_path_arg = 0;
>  			break;
> +		default:
> +			return command_usage(&copy_range_cmd);
>  		}
>  	}
>  
> diff --git a/logprint/logprint.c b/logprint/logprint.c
> index 511a32aca726..e882c5d44397 100644
> --- a/logprint/logprint.c
> +++ b/logprint/logprint.c
> @@ -193,7 +193,7 @@ main(int argc, char **argv)
>  			case 'V':
>  				printf(_("%s version %s\n"), progname, VERSION);
>  				exit(0);
> -			case '?':
> +			default:
>  				usage();
>  		}
>  	}
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index f14ce8db5a74..039b1dcc5afa 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3679,7 +3679,7 @@ main(
>  		case 'V':
>  			printf(_("%s version %s\n"), progname, VERSION);
>  			exit(0);
> -		case '?':
> +		default:
>  			unknown(optopt, "");
>  		}
>  	}
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index 4d37ddc64906..e509fdeb66fe 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -326,7 +326,7 @@ process_args(int argc, char **argv)
>  		case 'e':
>  			report_corrected = true;
>  			break;
> -		case '?':
> +		default:
>  			usage();
>  		}
>  	}
> diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
> index 014c54dd76b2..33b876f2147a 100644
> --- a/scrub/xfs_scrub.c
> +++ b/scrub/xfs_scrub.c
> @@ -671,8 +671,6 @@ main(
>  		case 'x':
>  			scrub_data = true;
>  			break;
> -		case '?':
> -			/* fall through */
>  		default:
>  			usage();
>  		}
> diff --git a/spaceman/freesp.c b/spaceman/freesp.c
> index 92cdb7439427..de301c195fb3 100644
> --- a/spaceman/freesp.c
> +++ b/spaceman/freesp.c
> @@ -310,7 +310,6 @@ init(
>  		case 's':
>  			summaryflag = 1;
>  			break;
> -		case '?':
>  		default:
>  			return command_usage(&freesp_cmd);
>  		}
> diff --git a/spaceman/prealloc.c b/spaceman/prealloc.c
> index e5d857bdd334..6fcbb461125b 100644
> --- a/spaceman/prealloc.c
> +++ b/spaceman/prealloc.c
> @@ -56,7 +56,6 @@ prealloc_f(
>  			eofb.eof_min_file_size = cvtnum(fsgeom->blocksize,
>  					fsgeom->sectsize, optarg);
>  			break;
> -		case '?':
>  		default:
>  			return command_usage(&prealloc_cmd);
>  		}
> -- 
> 2.26.0.rc2
>
diff mbox series

Patch

diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
index 91c2ae01683b..c4f9f34981ca 100644
--- a/copy/xfs_copy.c
+++ b/copy/xfs_copy.c
@@ -584,7 +584,7 @@  main(int argc, char **argv)
 		case 'V':
 			printf(_("%s version %s\n"), progname, VERSION);
 			exit(0);
-		case '?':
+		default:
 			usage();
 		}
 	}
diff --git a/db/freesp.c b/db/freesp.c
index 903c60d7380a..6f2346665847 100644
--- a/db/freesp.c
+++ b/db/freesp.c
@@ -177,7 +177,7 @@  init(
 		case 's':
 			summaryflag = 1;
 			break;
-		case '?':
+		default:
 			return usage();
 		}
 	}
diff --git a/db/init.c b/db/init.c
index 61eea111f017..ac649fbddbb9 100644
--- a/db/init.c
+++ b/db/init.c
@@ -84,15 +84,12 @@  init(
 		case 'V':
 			printf(_("%s version %s\n"), progname, VERSION);
 			exit(0);
-		case '?':
+		default:
 			usage();
-			/*NOTREACHED*/
 		}
 	}
-	if (optind + 1 != argc) {
+	if (optind + 1 != argc)
 		usage();
-		/*NOTREACHED*/
-	}
 
 	fsdevice = argv[optind];
 	if (!x.disfile)
diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
index d27e3b94e0c4..a68b515de40d 100644
--- a/growfs/xfs_growfs.c
+++ b/growfs/xfs_growfs.c
@@ -120,7 +120,6 @@  main(int argc, char **argv)
 		case 'V':
 			printf(_("%s version %s\n"), progname, VERSION);
 			exit(0);
-		case '?':
 		default:
 			usage();
 		}
diff --git a/io/copy_file_range.c b/io/copy_file_range.c
index fb5702e1faad..4c4332c6e5ec 100644
--- a/io/copy_file_range.c
+++ b/io/copy_file_range.c
@@ -127,6 +127,8 @@  copy_range_f(int argc, char **argv)
 			/* Expect no src_path arg */
 			src_path_arg = 0;
 			break;
+		default:
+			return command_usage(&copy_range_cmd);
 		}
 	}
 
diff --git a/logprint/logprint.c b/logprint/logprint.c
index 511a32aca726..e882c5d44397 100644
--- a/logprint/logprint.c
+++ b/logprint/logprint.c
@@ -193,7 +193,7 @@  main(int argc, char **argv)
 			case 'V':
 				printf(_("%s version %s\n"), progname, VERSION);
 				exit(0);
-			case '?':
+			default:
 				usage();
 		}
 	}
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index f14ce8db5a74..039b1dcc5afa 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3679,7 +3679,7 @@  main(
 		case 'V':
 			printf(_("%s version %s\n"), progname, VERSION);
 			exit(0);
-		case '?':
+		default:
 			unknown(optopt, "");
 		}
 	}
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 4d37ddc64906..e509fdeb66fe 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -326,7 +326,7 @@  process_args(int argc, char **argv)
 		case 'e':
 			report_corrected = true;
 			break;
-		case '?':
+		default:
 			usage();
 		}
 	}
diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
index 014c54dd76b2..33b876f2147a 100644
--- a/scrub/xfs_scrub.c
+++ b/scrub/xfs_scrub.c
@@ -671,8 +671,6 @@  main(
 		case 'x':
 			scrub_data = true;
 			break;
-		case '?':
-			/* fall through */
 		default:
 			usage();
 		}
diff --git a/spaceman/freesp.c b/spaceman/freesp.c
index 92cdb7439427..de301c195fb3 100644
--- a/spaceman/freesp.c
+++ b/spaceman/freesp.c
@@ -310,7 +310,6 @@  init(
 		case 's':
 			summaryflag = 1;
 			break;
-		case '?':
 		default:
 			return command_usage(&freesp_cmd);
 		}
diff --git a/spaceman/prealloc.c b/spaceman/prealloc.c
index e5d857bdd334..6fcbb461125b 100644
--- a/spaceman/prealloc.c
+++ b/spaceman/prealloc.c
@@ -56,7 +56,6 @@  prealloc_f(
 			eofb.eof_min_file_size = cvtnum(fsgeom->blocksize,
 					fsgeom->sectsize, optarg);
 			break;
-		case '?':
 		default:
 			return command_usage(&prealloc_cmd);
 		}