linux-kernel - Re: [PATCH v4 3/4] KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yz8zYXvhp9WGH4Uz@google.com>
Date:   Thu, 6 Oct 2022 19:58:25 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Vipin Sharma <vipinsh@...gle.com>
Cc:     pbonzini@...hat.com, dmatlack@...gle.com, andrew.jones@...ux.dev,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/4] KVM: selftests: Add atoi_paranoid() to catch
 errors missed by atoi()

On Thu, Oct 06, 2022, Vipin Sharma wrote:
> +int atoi_paranoid(const char *num_str)
> +{
> +	int num;
> +	char *end_ptr;

Reverse fir-tree when it's convention:

	char *end_ptr;

> +
> +	errno = 0;
> +	num = (int)strtol(num_str, &end_ptr, 10);
> +	TEST_ASSERT(!errno, "strtol(\"%s\") failed", num_str);
> +	TEST_ASSERT(num_str != end_ptr,
> +		    "strtol(\"%s\") didn't find any valid number.\n", num_str);

s/number/integer ?  And should that be "a valid intenger", not "any valid integer"?
"any" implies that this helper will be happy if there's at least one integer,
whereas I believe the intent is to find _exactly_ one integer.

> +	TEST_ASSERT(
> +		*end_ptr == '\0',

Weird and unnecessary wrap+indentation.

> +		"strtol(\"%s\") failed to parse trailing characters \"%s\".\n",
> +		num_str, end_ptr);
> +
> +	return num;
> +}
> diff --git a/tools/testing/selftests/kvm/max_guest_memory_test.c b/tools/testing/selftests/kvm/max_guest_memory_test.c
> index 9a6e4f3ad6b5..1595b73dc09a 100644
> --- a/tools/testing/selftests/kvm/max_guest_memory_test.c
> +++ b/tools/testing/selftests/kvm/max_guest_memory_test.c
> @@ -193,15 +193,15 @@ int main(int argc, char *argv[])
>  	while ((opt = getopt(argc, argv, "c:h:m:s:H")) != -1) {
>  		switch (opt) {
>  		case 'c':
> -			nr_vcpus = atoi(optarg);
> +			nr_vcpus = atoi_paranoid(optarg);
>  			TEST_ASSERT(nr_vcpus > 0, "number of vcpus must be >0");

Many users require a positive and or non-negative value, maybe add wrappers in
a follow-up?

			nr_vcpus = atoi_positive(optarg);

and later down

			targs->tfirst = atoi_non_negative(optarg);

We'll lose custom error messages, but I don't think that's a big deal.  Definitely
not required, just a thought.

> diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
> index 0d55f508d595..c366949c8362 100644
> --- a/tools/testing/selftests/kvm/set_memory_region_test.c
> +++ b/tools/testing/selftests/kvm/set_memory_region_test.c
> @@ -407,7 +407,7 @@ int main(int argc, char *argv[])
>  
>  #ifdef __x86_64__
>  	if (argc > 1)
> -		loops = atoi(argv[1]);
> +		loops = atoi_paranoid(argv[1]);

This is a good candidate for atoi_positive().

>  	else
>  		loops = 10;
>  
> diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
> index 59ffe7fd354f..354b6902849c 100644
> --- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c
> @@ -241,10 +241,10 @@ int main(int argc, char **argv)
>  	while ((opt = getopt(argc, argv, "hp:t:r")) != -1) {
>  		switch (opt) {
>  		case 'p':
> -			reclaim_period_ms = atoi(optarg);
> +			reclaim_period_ms = atoi_paranoid(optarg);
>  			break;
>  		case 't':
> -			token = atoi(optarg);
> +			token = atoi_paranoid(optarg);
>  			break;
>  		case 'r':
>  			reboot_permissions = true;
> -- 
> 2.38.0.rc1.362.ged0d419d3c-goog
>