[ovs-dev] [bug4462 2/3] ofp-parse: Don't segfault when an OpenFlow action's argument is missing.

Ben Pfaff blp at nicira.com
Wed Feb 23 17:43:09 UTC 2011


Thanks.  I added that to the log message and I'll push this series soon.

On Tue, Feb 22, 2011 at 11:03:33PM -0800, Justin Pettit wrote:
> Looks good.  Thanks for fixing Coverity issue #10712, too.  :-)
> 
> --Justin
> 
> 
> On Feb 22, 2011, at 4:28 PM, Ben Pfaff wrote:
> 
> > Some actions checked that 'arg' was nonnull before attempting to parse it
> > but a lot of them didn't.  This commit avoids the segfault by substituting
> > an empty string when no argument is given.  It also updates a few of the
> > action implementations to correspond.
> > 
> > Reported-by: Reid Price <reid at nicira.com>
> > Bug #4462.
> > ---
> > lib/ofp-parse.c |   14 +++++++++-----
> > 1 files changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> > index 3fac474..73c3ebc 100644
> > --- a/lib/ofp-parse.c
> > +++ b/lib/ofp-parse.c
> > @@ -43,7 +43,7 @@ str_to_u32(const char *str)
> >     char *tail;
> >     uint32_t value;
> > 
> > -    if (!str) {
> > +    if (!str[0]) {
> >         ovs_fatal(0, "missing required numeric argument");
> >     }
> > 
> > @@ -61,6 +61,10 @@ str_to_u64(const char *str)
> >     char *tail;
> >     uint64_t value;
> > 
> > +    if (!str[0]) {
> > +        ovs_fatal(0, "missing required numeric argument");
> > +    }
> > +
> >     errno = 0;
> >     value = strtoull(str, &tail, 0);
> >     if (errno == EINVAL || errno == ERANGE || *tail) {
> > @@ -271,6 +275,7 @@ str_to_action(char *str, struct ofpbuf *b)
> >     pos = str;
> >     n_actions = 0;
> >     for (;;) {
> > +        char empty_string[] = "";
> >         char *act, *arg;
> >         size_t actlen;
> >         uint16_t port;
> > @@ -320,7 +325,7 @@ str_to_action(char *str, struct ofpbuf *b)
> >             pos = arg + arglen;
> >         } else {
> >             /* There might be no argument at all. */
> > -            arg = NULL;
> > +            arg = empty_string;
> >             pos = act + actlen + (act[actlen] != '\0');
> >         }
> >         act[actlen] = '\0';
> > @@ -410,7 +415,7 @@ str_to_action(char *str, struct ofpbuf *b)
> >             nan->subtype = htons(NXAST_NOTE);
> > 
> >             b->size -= sizeof nan->note;
> > -            while (arg && *arg != '\0') {
> > +            while (*arg != '\0') {
> >                 uint8_t byte;
> >                 bool ok;
> > 
> > @@ -472,7 +477,7 @@ str_to_action(char *str, struct ofpbuf *b)
> > 
> >             /* Unless a numeric argument is specified, we send the whole
> >              * packet to the controller. */
> > -            if (arg && (strspn(arg, "0123456789") == strlen(arg))) {
> > +            if (arg[0] && (strspn(arg, "0123456789") == strlen(arg))) {
> >                oao->max_len = htons(str_to_u32(arg));
> >             } else {
> >                 oao->max_len = htons(UINT16_MAX);
> > @@ -909,4 +914,3 @@ parse_ofp_flow_stats_request_str(struct flow_stats_request *fsr,
> >     fsr->out_port = fm.out_port;
> >     fsr->table_id = table_id;
> > }
> > -
> > -- 
> > 1.7.1
> > 
> > 
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
> 




More information about the dev mailing list