[1/2] hackbench: Fix negativity opt checking

It was easy to escape the checking. For example, run

  ./hackbench --datasize 4096 -g -1 -l -1

This patch fixes the checking.

Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com>
---
 src/hackbench/hackbench.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

On Thu, 28 Apr 2022, Yihao Wu wrote:

> It was easy to escape the checking. For example, run
> 
>   ./hackbench --datasize 4096 -g -1 -l -1
> 
> This patch fixes the checking.
> 
> Signed-off-by: Yihao Wu <wuyihao@linux.alibaba.com>
> ---
>  src/hackbench/hackbench.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/src/hackbench/hackbench.c b/src/hackbench/hackbench.c
> index 268c232..b9b0af6 100644
> --- a/src/hackbench/hackbench.c
> +++ b/src/hackbench/hackbench.c
> @@ -31,10 +31,10 @@
>  #include <setjmp.h>
>  #include <sched.h>
>  
> -static unsigned int datasize = 100;
> -static unsigned int loops = 100;
> -static unsigned int num_groups = 10;
> -static unsigned int num_fds = 20;
> +static int datasize = 100;
> +static int loops = 100;
> +static int num_groups = 10;
> +static int num_fds = 20;
>  static unsigned int fifo = 0;
>  
>  /*
> @@ -377,7 +377,7 @@ static void process_options(int argc, char *argv[])
>  		}
>  		switch (c) {
>  		case 'f':
> -			if (!(argv[optind] && (num_fds = atoi(optarg)) > 0)) {
> +			if ((num_fds = atoi(optarg)) <= 0) {
>  				fprintf(stderr, "%s: --fds|-f requires an integer > 0\n", argv[0]);
>  				print_usage_exit(1);
>  			}
> @@ -386,7 +386,7 @@ static void process_options(int argc, char *argv[])
>  			fifo = 1;
>  			break;
>  		case 'g':
> -			if (!(argv[optind] && (num_groups = atoi(optarg)) > 0)) {
> +			if ((num_groups = atoi(optarg)) <= 0) {
>  				fprintf(stderr, "%s: --groups|-g requires an integer > 0\n", argv[0]);
>  				print_usage_exit(1);
>  			}
> @@ -394,7 +394,7 @@ static void process_options(int argc, char *argv[])
>  		case 'h':
>  			print_usage_exit(0);
>  		case 'l':
> -			if (!(argv[optind] && (loops = atoi(optarg)) > 0)) {
> +			if ((loops = atoi(optarg)) <= 0) {
>  				fprintf(stderr, "%s: --loops|-l requires an integer > 0\n", argv[0]);
>  				print_usage_exit(1);
>  			}
> @@ -403,7 +403,7 @@ static void process_options(int argc, char *argv[])
>  			use_pipes = 1;
>  			break;
>  		case 's':
> -			if (!(argv[optind] && (datasize = atoi(optarg)) > 0)) {
> +			if ((datasize = atoi(optarg)) <= 0) {
>  				fprintf(stderr, "%s: --datasize|-s requires an integer > 0\n", argv[0]);
>  				print_usage_exit(1);
>  			}
> -- 
> 2.18.2
> 
> 

Well, the code doesn't work the way the author intended, that's for sure.
As your patch indicates, you realize the problem is that an unsigned 
variable can never be less than zero, so the check is meaningless. 
However, is changing the data type the right solution, just to make a 
check against something stupid a user might input the correct thing to do? 
It decreases the size of valid input. Maybe that doesn't matter, but then 
could it introduce a new bug somewhere? You've dropped the argv[optind] 
too which also doesn't work the way the author intended, but it was 
probably meant as a check of whether the user provided an argument or not.

What I would rather see if you want to fix this, is a check against the 
user input that doesn't change the datatype. For example, if you treat the 
input as a char, if (optarg[0] == '-') then we can tell the difference 
between a really big number or user input that is stupid or malicious.
If you look in cyclictest, (there could be bugs lurking there too), there 
is an attempt to parse this kind of thing and to check whether there are 
arguments too.

Thanks

John

I'm sorry! John. In the commit log, I described the problem wrong. It 
should be more trivially reproduced like this,

./hackbench --datasize 4096 -g 1 -l 1

This command gives an error "--datasize|-s requires an integer > 0". But 
if you append any argument (or nonsense) after this command, like,

./hackbench --datasize 4096 -g 1 -l 1 whatever

This command runs without any error message.

My original intention of this patch was to remove "!(argv[optind]". I 
suppose this condition leads to this bug. (Pardon me for I don't know 
what's it for. I'd admit I'm not familiar with getopt)

This negative bugfix should only be a piggypack fix. But as you've 
pointed out, it merely hides misusage. So it's a bad bugfix.

Thanks,

Yihao

On 2022/5/10 02:59, John Kacur wrote:
> Well, the code doesn't work the way the author intended, that's for sure.
> As your patch indicates, you realize the problem is that an unsigned
> variable can never be less than zero, so the check is meaningless.
> However, is changing the data type the right solution, just to make a
> check against something stupid a user might input the correct thing to do?
> It decreases the size of valid input. Maybe that doesn't matter, but then
> could it introduce a new bug somewhere? You've dropped the argv[optind]
> too which also doesn't work the way the author intended, but it was
> probably meant as a check of whether the user provided an argument or not.
> 
> What I would rather see if you want to fix this, is a check against the
> user input that doesn't change the datatype. For example, if you treat the
> input as a char, if (optarg[0] == '-') then we can tell the difference
> between a really big number or user input that is stupid or malicious.
> If you look in cyclictest, (there could be bugs lurking there too), there
> is an attempt to parse this kind of thing and to check whether there are
> arguments too.
> 
> Thanks
> 
> John