| Message ID | 1444669827-5198-1-git-send-email-blp@nicira.com |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Oct 12, 2015 at 10:10:27AM -0700, Ben Pfaff wrote: > Signed-off-by: Ben Pfaff <blp@nicira.com> > Reported-by: Manpreet Singh <er.manpreet25@gmail.com> > Reported-at: http://openvswitch.org/pipermail/discuss/2015-October/019048.html Reviewed-by: Simon Horman <simon.horman@netronome.com>
On Mon, Oct 12, 2015 at 10:10:27AM -0700, Ben Pfaff wrote: > Signed-off-by: Ben Pfaff <blp@nicira.com> > Reported-by: Manpreet Singh <er.manpreet25@gmail.com> > Reported-at: http://openvswitch.org/pipermail/discuss/2015-October/019048.html > --- > AUTHORS | 1 + > lib/ofp-util.c | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/AUTHORS b/AUTHORS > index 99bcf60..8123f43 100644 > --- a/AUTHORS > +++ b/AUTHORS > @@ -310,6 +310,7 @@ Len Gao leng@vmware.com > Logan Rosen logatronico@gmail.com > Luca Falavigna dktrkranz@debian.org > Luiz Henrique Ozaki luiz.ozaki@gmail.com > +Manpreet Singh er.manpreet25@gmail.com > Marco d'Itri md@Linux.IT > Martin Vizvary vizvary@ics.muni.cz > Marvin Pascual marvin@pascual.com.ph > diff --git a/lib/ofp-util.c b/lib/ofp-util.c > index b9dbcda..f0f6319 100644 > --- a/lib/ofp-util.c > +++ b/lib/ofp-util.c > @@ -8679,7 +8679,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh, > case OFPGT11_FF: > break; > default: > - OVS_NOT_REACHED(); > + return OFPERR_OFPGMFC_BAD_TYPE; This looks correct. > } > > switch (gm->command) { > @@ -8694,7 +8694,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh, > } > break; > default: > - OVS_NOT_REACHED(); > + return OFPERR_OFPGMFC_BAD_COMMAND; This too. > } > > LIST_FOR_EACH (bucket, list_node, &gm->buckets) { But then it continues iterating over the buckets checking the gm->type: ... default: OVS_NOT_REACHED(); } Shouldn't that also returns OFPERR_OFPGMFC_BAD_TYPE? Thanks, fbl
On Thu, Nov 26, 2015 at 05:20:54PM -0200, Flavio Leitner wrote: > On Mon, Oct 12, 2015 at 10:10:27AM -0700, Ben Pfaff wrote: > > Signed-off-by: Ben Pfaff <blp@nicira.com> > > Reported-by: Manpreet Singh <er.manpreet25@gmail.com> > > Reported-at: http://openvswitch.org/pipermail/discuss/2015-October/019048.html > > --- > > AUTHORS | 1 + > > lib/ofp-util.c | 4 ++-- > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/AUTHORS b/AUTHORS > > index 99bcf60..8123f43 100644 > > --- a/AUTHORS > > +++ b/AUTHORS > > @@ -310,6 +310,7 @@ Len Gao leng@vmware.com > > Logan Rosen logatronico@gmail.com > > Luca Falavigna dktrkranz@debian.org > > Luiz Henrique Ozaki luiz.ozaki@gmail.com > > +Manpreet Singh er.manpreet25@gmail.com > > Marco d'Itri md@Linux.IT > > Martin Vizvary vizvary@ics.muni.cz > > Marvin Pascual marvin@pascual.com.ph > > diff --git a/lib/ofp-util.c b/lib/ofp-util.c > > index b9dbcda..f0f6319 100644 > > --- a/lib/ofp-util.c > > +++ b/lib/ofp-util.c > > @@ -8679,7 +8679,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh, > > case OFPGT11_FF: > > break; > > default: > > - OVS_NOT_REACHED(); > > + return OFPERR_OFPGMFC_BAD_TYPE; > > This looks correct. > > > > } > > > > switch (gm->command) { > > @@ -8694,7 +8694,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh, > > } > > break; > > default: > > - OVS_NOT_REACHED(); > > + return OFPERR_OFPGMFC_BAD_COMMAND; > > This too. > > } > > > > LIST_FOR_EACH (bucket, list_node, &gm->buckets) { > > But then it continues iterating over the buckets checking the > gm->type: > ... > default: > OVS_NOT_REACHED(); > } > Shouldn't that also returns OFPERR_OFPGMFC_BAD_TYPE? Here we genuinely can't get any invalid types because the first "switch" statement in the function has verified gm->type.
On Wed, Nov 25, 2015 at 05:31:33PM +0900, Simon Horman wrote: > On Mon, Oct 12, 2015 at 10:10:27AM -0700, Ben Pfaff wrote: > > Signed-off-by: Ben Pfaff <blp@nicira.com> > > Reported-by: Manpreet Singh <er.manpreet25@gmail.com> > > Reported-at: http://openvswitch.org/pipermail/discuss/2015-October/019048.html > > Reviewed-by: Simon Horman <simon.horman@netronome.com> Thanks Simon, I applied this to master.
On Sun, Nov 29, 2015 at 11:02:02AM -0800, Ben Pfaff wrote: > On Thu, Nov 26, 2015 at 05:20:54PM -0200, Flavio Leitner wrote: > > On Mon, Oct 12, 2015 at 10:10:27AM -0700, Ben Pfaff wrote: > > > Signed-off-by: Ben Pfaff <blp@nicira.com> > > > Reported-by: Manpreet Singh <er.manpreet25@gmail.com> > > > Reported-at: http://openvswitch.org/pipermail/discuss/2015-October/019048.html > > > --- > > > AUTHORS | 1 + > > > lib/ofp-util.c | 4 ++-- > > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/AUTHORS b/AUTHORS > > > index 99bcf60..8123f43 100644 > > > --- a/AUTHORS > > > +++ b/AUTHORS > > > @@ -310,6 +310,7 @@ Len Gao leng@vmware.com > > > Logan Rosen logatronico@gmail.com > > > Luca Falavigna dktrkranz@debian.org > > > Luiz Henrique Ozaki luiz.ozaki@gmail.com > > > +Manpreet Singh er.manpreet25@gmail.com > > > Marco d'Itri md@Linux.IT > > > Martin Vizvary vizvary@ics.muni.cz > > > Marvin Pascual marvin@pascual.com.ph > > > diff --git a/lib/ofp-util.c b/lib/ofp-util.c > > > index b9dbcda..f0f6319 100644 > > > --- a/lib/ofp-util.c > > > +++ b/lib/ofp-util.c > > > @@ -8679,7 +8679,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh, > > > case OFPGT11_FF: > > > break; > > > default: > > > - OVS_NOT_REACHED(); > > > + return OFPERR_OFPGMFC_BAD_TYPE; > > > > This looks correct. > > > > > > > } > > > > > > switch (gm->command) { > > > @@ -8694,7 +8694,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh, > > > } > > > break; > > > default: > > > - OVS_NOT_REACHED(); > > > + return OFPERR_OFPGMFC_BAD_COMMAND; > > > > This too. > > > } > > > > > > LIST_FOR_EACH (bucket, list_node, &gm->buckets) { > > > > But then it continues iterating over the buckets checking the > > gm->type: > > ... > > default: > > OVS_NOT_REACHED(); > > } > > Shouldn't that also returns OFPERR_OFPGMFC_BAD_TYPE? > > Here we genuinely can't get any invalid types because the first "switch" > statement in the function has verified gm->type. Indeed, that is the case now. However, if we copy&paste that piece of code somewhere else or change the function in the future, then the issue might get reintroduced. fbl
On Mon, Nov 30, 2015 at 09:05:54PM -0200, Flavio Leitner wrote: > On Sun, Nov 29, 2015 at 11:02:02AM -0800, Ben Pfaff wrote: > > On Thu, Nov 26, 2015 at 05:20:54PM -0200, Flavio Leitner wrote: > > > On Mon, Oct 12, 2015 at 10:10:27AM -0700, Ben Pfaff wrote: > > > > Signed-off-by: Ben Pfaff <blp@nicira.com> > > > > Reported-by: Manpreet Singh <er.manpreet25@gmail.com> > > > > Reported-at: http://openvswitch.org/pipermail/discuss/2015-October/019048.html > > > > --- > > > > AUTHORS | 1 + > > > > lib/ofp-util.c | 4 ++-- > > > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/AUTHORS b/AUTHORS > > > > index 99bcf60..8123f43 100644 > > > > --- a/AUTHORS > > > > +++ b/AUTHORS > > > > @@ -310,6 +310,7 @@ Len Gao leng@vmware.com > > > > Logan Rosen logatronico@gmail.com > > > > Luca Falavigna dktrkranz@debian.org > > > > Luiz Henrique Ozaki luiz.ozaki@gmail.com > > > > +Manpreet Singh er.manpreet25@gmail.com > > > > Marco d'Itri md@Linux.IT > > > > Martin Vizvary vizvary@ics.muni.cz > > > > Marvin Pascual marvin@pascual.com.ph > > > > diff --git a/lib/ofp-util.c b/lib/ofp-util.c > > > > index b9dbcda..f0f6319 100644 > > > > --- a/lib/ofp-util.c > > > > +++ b/lib/ofp-util.c > > > > @@ -8679,7 +8679,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh, > > > > case OFPGT11_FF: > > > > break; > > > > default: > > > > - OVS_NOT_REACHED(); > > > > + return OFPERR_OFPGMFC_BAD_TYPE; > > > > > > This looks correct. > > > > > > > > > > } > > > > > > > > switch (gm->command) { > > > > @@ -8694,7 +8694,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh, > > > > } > > > > break; > > > > default: > > > > - OVS_NOT_REACHED(); > > > > + return OFPERR_OFPGMFC_BAD_COMMAND; > > > > > > This too. > > > > } > > > > > > > > LIST_FOR_EACH (bucket, list_node, &gm->buckets) { > > > > > > But then it continues iterating over the buckets checking the > > > gm->type: > > > ... > > > default: > > > OVS_NOT_REACHED(); > > > } > > > Shouldn't that also returns OFPERR_OFPGMFC_BAD_TYPE? > > > > Here we genuinely can't get any invalid types because the first "switch" > > statement in the function has verified gm->type. > > Indeed, that is the case now. However, if we copy&paste that piece of > code somewhere else or change the function in the future, then the issue > might get reintroduced. OK, I sent out a patch, CCing you.
diff --git a/AUTHORS b/AUTHORS index 99bcf60..8123f43 100644 --- a/AUTHORS +++ b/AUTHORS @@ -310,6 +310,7 @@ Len Gao leng@vmware.com Logan Rosen logatronico@gmail.com Luca Falavigna dktrkranz@debian.org Luiz Henrique Ozaki luiz.ozaki@gmail.com +Manpreet Singh er.manpreet25@gmail.com Marco d'Itri md@Linux.IT Martin Vizvary vizvary@ics.muni.cz Marvin Pascual marvin@pascual.com.ph diff --git a/lib/ofp-util.c b/lib/ofp-util.c index b9dbcda..f0f6319 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -8679,7 +8679,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh, case OFPGT11_FF: break; default: - OVS_NOT_REACHED(); + return OFPERR_OFPGMFC_BAD_TYPE; } switch (gm->command) { @@ -8694,7 +8694,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh, } break; default: - OVS_NOT_REACHED(); + return OFPERR_OFPGMFC_BAD_COMMAND; } LIST_FOR_EACH (bucket, list_node, &gm->buckets) {
Signed-off-by: Ben Pfaff <blp@nicira.com> Reported-by: Manpreet Singh <er.manpreet25@gmail.com> Reported-at: http://openvswitch.org/pipermail/discuss/2015-October/019048.html --- AUTHORS | 1 + lib/ofp-util.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-)