The "uffd_delay" variable is unsigned so it's always going to be >= 0.
Fixes: 0119cb365c93 ("KVM: selftests: Add configurable demand paging delay") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com --- tools/testing/selftests/kvm/demand_paging_test.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c index 360cd3ea4cd67..4eb79621434e6 100644 --- a/tools/testing/selftests/kvm/demand_paging_test.c +++ b/tools/testing/selftests/kvm/demand_paging_test.c @@ -615,8 +615,6 @@ int main(int argc, char *argv[]) break; case 'd': uffd_delay = strtoul(optarg, NULL, 0); - TEST_ASSERT(uffd_delay >= 0, - "A negative UFFD delay is not supported."); break; case 'b': vcpu_memory_bytes = parse_size(optarg);
On 05/06/20 13:00, Dan Carpenter wrote:
The "uffd_delay" variable is unsigned so it's always going to be >= 0.
Fixes: 0119cb365c93 ("KVM: selftests: Add configurable demand paging delay") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
tools/testing/selftests/kvm/demand_paging_test.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c index 360cd3ea4cd67..4eb79621434e6 100644 --- a/tools/testing/selftests/kvm/demand_paging_test.c +++ b/tools/testing/selftests/kvm/demand_paging_test.c @@ -615,8 +615,6 @@ int main(int argc, char *argv[]) break; case 'd': uffd_delay = strtoul(optarg, NULL, 0);
TEST_ASSERT(uffd_delay >= 0,"A negative UFFD delay is not supported."); break;
The bug is that strtoul is "impossible" to use correctly. The right fix would be to have a replacement for strtoul.
Paolo
On Fri, Jun 05, 2020 at 01:16:59PM +0200, Paolo Bonzini wrote:
On 05/06/20 13:00, Dan Carpenter wrote:
The "uffd_delay" variable is unsigned so it's always going to be >= 0.
Fixes: 0119cb365c93 ("KVM: selftests: Add configurable demand paging delay") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
tools/testing/selftests/kvm/demand_paging_test.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c index 360cd3ea4cd67..4eb79621434e6 100644 --- a/tools/testing/selftests/kvm/demand_paging_test.c +++ b/tools/testing/selftests/kvm/demand_paging_test.c @@ -615,8 +615,6 @@ int main(int argc, char *argv[]) break; case 'd': uffd_delay = strtoul(optarg, NULL, 0);
TEST_ASSERT(uffd_delay >= 0,"A negative UFFD delay is not supported."); break;The bug is that strtoul is "impossible" to use correctly. The right fix would be to have a replacement for strtoul.
The test needs an upper limit. It obviously doesn't make sense to ever want a ULONG_MAX usec delay. What's the maximum number of usecs we should allow?
Thanks, drew
On Fri, Jun 05, 2020 at 01:53:16PM +0200, Andrew Jones wrote:
On Fri, Jun 05, 2020 at 01:16:59PM +0200, Paolo Bonzini wrote:
On 05/06/20 13:00, Dan Carpenter wrote:
The "uffd_delay" variable is unsigned so it's always going to be >= 0.
Fixes: 0119cb365c93 ("KVM: selftests: Add configurable demand paging delay") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
tools/testing/selftests/kvm/demand_paging_test.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c index 360cd3ea4cd67..4eb79621434e6 100644 --- a/tools/testing/selftests/kvm/demand_paging_test.c +++ b/tools/testing/selftests/kvm/demand_paging_test.c @@ -615,8 +615,6 @@ int main(int argc, char *argv[]) break; case 'd': uffd_delay = strtoul(optarg, NULL, 0);
TEST_ASSERT(uffd_delay >= 0,"A negative UFFD delay is not supported."); break;The bug is that strtoul is "impossible" to use correctly.
Could I ask why?
The right fix would be to have a replacement for strtoul.
The test needs an upper limit. It obviously doesn't make sense to ever want a ULONG_MAX usec delay. What's the maximum number of usecs we should allow?
Maybe this test can also be used to emulate a hang-forever kvm mmu fault due to some reason we wanted, by specifying an extremely large value here? From that POV, seems still ok to even keep it unbound as a test...
Thanks,
On 05/06/20 14:48, Peter Xu wrote:
The bug is that strtoul is "impossible" to use correctly.
Could I ask why?
To see see how annoying the situation is, check out utils/cutils.c in QEMU; basically, it is very hard to do error handling. From the man page:
Since strtoul() can legitimately return 0 or ULONG_MAX (ULLONG_MAX for strtoull()) on both success and failure, the calling program should set errno to 0 before the call, and then determine if an error occurred by checking whether errno has a nonzero value after the call.
and of course no one wants to write code for that every time they have to parse a number.
In addition, if the string is empty it returns 0, and of endptr is NULL it will accept something like "123abc" and return 123.
So it is not literally impossible, but it is a poorly-designed interface which is a major source of bugs. On Rusty's API design levels[1][2], I would put it at 3 if I'm feeling generous ("Read the documentation and you'll get it right"), and at -4 to -7 ("The obvious use is wrong") if it's been a bad day.
Therefore it's quite common to have a wrapper like
int my_strtoul(char *p, char **endptr, unsigned long *result);
The wrapper will:
- check that the string is not empty
- always return 0 or -1 because of the by-reference output argument "result"
- take care of checking that the entire input string was parsed, for example by rejecting partial parsing of the string if endptr == NULL.
This version gets a solid 7 ("The obvious use is probably the correct one"); possibly even 8 ("The compiler will warn if you get it wrong") because the output argument gives you better protection against overflow.
Regarding overflow, there is a strtol but not a strtoi, so you need to have a temporary long and do range checking manually. Again, you will most likely make mistakes if you use strtol, while my_strtol will merely make it annoying but it should be obvious that you're getting it wrong.
Paolo
[1] https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html [2] https://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html
On Fri, Jun 05, 2020 at 03:26:39PM +0200, Paolo Bonzini wrote:
On 05/06/20 14:48, Peter Xu wrote:
The bug is that strtoul is "impossible" to use correctly.
Could I ask why?
To see see how annoying the situation is, check out utils/cutils.c in QEMU; basically, it is very hard to do error handling. From the man page:
Since strtoul() can legitimately return 0 or ULONG_MAX (ULLONG_MAX for strtoull()) on both success and failure, the calling program should set errno to 0 before the call, and then determine if an error occurred by checking whether errno has a nonzero value after the call.and of course no one wants to write code for that every time they have to parse a number.
In addition, if the string is empty it returns 0, and of endptr is NULL it will accept something like "123abc" and return 123.
So it is not literally impossible, but it is a poorly-designed interface which is a major source of bugs. On Rusty's API design levels[1][2], I would put it at 3 if I'm feeling generous ("Read the documentation and you'll get it right"), and at -4 to -7 ("The obvious use is wrong") if it's been a bad day.
Therefore it's quite common to have a wrapper like
int my_strtoul(char *p, char **endptr, unsigned long *result);The wrapper will:
check that the string is not empty
always return 0 or -1 because of the by-reference output argument "result"
take care of checking that the entire input string was parsed, for
example by rejecting partial parsing of the string if endptr == NULL.
This version gets a solid 7 ("The obvious use is probably the correct one"); possibly even 8 ("The compiler will warn if you get it wrong") because the output argument gives you better protection against overflow.
Regarding overflow, there is a strtol but not a strtoi, so you need to have a temporary long and do range checking manually. Again, you will most likely make mistakes if you use strtol, while my_strtol will merely make it annoying but it should be obvious that you're getting it wrong.
Paolo
[1] https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html [2] https://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html
Fair enough, and a good reading material. :)
Thanks!
linux-kselftest-mirror@lists.linaro.org
-
Andrew Jones -
Dan Carpenter -
Paolo Bonzini -
Peter Xu