| Message ID | 1470603965-73273-2-git-send-email-bhanuprakash.bodireddy@intel.com |
|---|---|
| State | Rejected |
| Headers | show |
> >ovs-appctl can crash with missing timeout argument. > # ovs-appctl --timeout= dpif-netdev/pmd-stats-show > >Fix by using strtol and validating the timeout value. > >Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> >--- > utilities/ovs-appctl.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > >diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c >index 8f87cc4..2543ee9 100644 >--- a/utilities/ovs-appctl.c >+++ b/utilities/ovs-appctl.c >@@ -127,6 +127,7 @@ parse_command_line(int argc, char *argv[]) > char *short_options_ = ovs_cmdl_long_options_to_short_options(long_options); > char *short_options = xasprintf("+%s", short_options_); > const char *target; >+ int timeout; > int e_options; > > target = NULL; >@@ -165,7 +166,13 @@ parse_command_line(int argc, char *argv[]) > exit(EXIT_SUCCESS); > > case 'T': >- time_alarm(atoi(optarg)); >+ timeout = strtol(optarg, NULL, 10); Hi Bhanu, To ensure that the user has supplied a valid numeric timeout value, you should provide a non-NULL 'endptr' parameter, and perform the usual checks on it, as described in the strtol man page: " If endptr is not NULL, strtol() stores the address of the first invalid character in *endptr. If there were no digits at all, strtol() stores the original value of nptr in *endptr (and returns 0). In particular, if *nptr is not '\0' but **endptr is '\0' on return, the entire string is valid." Cheers, Mark >+ if (timeout <= 0) { >+ ovs_fatal(0, "timeout value %s on -t or --timeout is invalid", >+ optarg); >+ } else { >+ time_alarm(timeout); >+ } > break; > > case 'V': >-- >2.4.11 > >_______________________________________________ >dev mailing list >dev@openvswitch.org >http://openvswitch.org/mailman/listinfo/dev
>-----Original Message----- >From: Kavanagh, Mark B >Sent: Monday, August 8, 2016 9:15 AM >To: Bodireddy, Bhanuprakash <bhanuprakash.bodireddy@intel.com>; >dev@openvswitch.org >Subject: RE: [ovs-dev] [PATCH 2/2] ovs-appctl: Fix potential crash with timeout >argument > >> >>ovs-appctl can crash with missing timeout argument. >> # ovs-appctl --timeout= dpif-netdev/pmd-stats-show >> >>Fix by using strtol and validating the timeout value. >> >>Signed-off-by: Bhanuprakash Bodireddy >><bhanuprakash.bodireddy@intel.com> >>--- >> utilities/ovs-appctl.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >>diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c index >>8f87cc4..2543ee9 100644 >>--- a/utilities/ovs-appctl.c >>+++ b/utilities/ovs-appctl.c >>@@ -127,6 +127,7 @@ parse_command_line(int argc, char *argv[]) >> char *short_options_ = >ovs_cmdl_long_options_to_short_options(long_options); >> char *short_options = xasprintf("+%s", short_options_); >> const char *target; >>+ int timeout; >> int e_options; >> >> target = NULL; >>@@ -165,7 +166,13 @@ parse_command_line(int argc, char *argv[]) >> exit(EXIT_SUCCESS); >> >> case 'T': >>- time_alarm(atoi(optarg)); >>+ timeout = strtol(optarg, NULL, 10); > >Hi Bhanu, > >To ensure that the user has supplied a valid numeric timeout value, you >should provide a non-NULL 'endptr' parameter, and perform the usual checks >on it, as described in the strtol man page: > " If endptr is not NULL, strtol() stores the address of the first > invalid character in *endptr. If there were no digits at all, > strtol() stores the original value of nptr in *endptr (and returns > 0). In particular, if *nptr is not '\0' but **endptr is '\0' on > return, the entire string is valid." > Thanks for reviewing the patch Mark, you have a point and I am sending out another version of the patch. I have verified all the below conditions with the v2 and found to be working good. ovs-appctl --timeout= dpif-netdev/pmd-stats-show (no timeout specified) ovs-appctl --timeout=0 dpif-netdev/pmd-stats-show (timeout 0, invalid) ovs-appctl --timeout=12345675345212151252543523524524 dpif-netdev/pmd-stats-show (overflow case) ovs-appctl --timeout=123abc dpif-netdev/pmd-stats-show (invalid input) ovs-appctl --timeout=1 dpif-netdev/pmd-stats-show (valid input) Regards, Bhanu Prakash. > > >>+ if (timeout <= 0) { >>+ ovs_fatal(0, "timeout value %s on -t or --timeout is invalid", >>+ optarg); >>+ } else { >>+ time_alarm(timeout); >>+ } >> break; >> >> case 'V': >>-- >>2.4.11 >> >>_______________________________________________ >>dev mailing list >>dev@openvswitch.org >>http://openvswitch.org/mailman/listinfo/dev
On Sun, Aug 07, 2016 at 10:06:05PM +0100, Bhanuprakash Bodireddy wrote: > ovs-appctl can crash with missing timeout argument. > # ovs-appctl --timeout= dpif-netdev/pmd-stats-show I don't see the crash when I run this. How is it caused?
diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c index 8f87cc4..2543ee9 100644 --- a/utilities/ovs-appctl.c +++ b/utilities/ovs-appctl.c @@ -127,6 +127,7 @@ parse_command_line(int argc, char *argv[]) char *short_options_ = ovs_cmdl_long_options_to_short_options(long_options); char *short_options = xasprintf("+%s", short_options_); const char *target; + int timeout; int e_options; target = NULL; @@ -165,7 +166,13 @@ parse_command_line(int argc, char *argv[]) exit(EXIT_SUCCESS); case 'T': - time_alarm(atoi(optarg)); + timeout = strtol(optarg, NULL, 10); + if (timeout <= 0) { + ovs_fatal(0, "timeout value %s on -t or --timeout is invalid", + optarg); + } else { + time_alarm(timeout); + } break; case 'V':
ovs-appctl can crash with missing timeout argument. # ovs-appctl --timeout= dpif-netdev/pmd-stats-show Fix by using strtol and validating the timeout value. Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> --- utilities/ovs-appctl.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)