| Message ID | 20210420222255.45203-1-aroytman@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2] ovn-sbctl: Prevent core dump from ovn-sbctl lflow-list [datpath] 0xflow | expand |
Hi Alexey, nice find! I have a comment down below: On 4/20/21 6:22 PM, Alexey Roytman wrote: > From: Alexey Roytman <roytman@il.ibm.com> > > When ovn-sbctl lflow-list gets lflow argument with 0x prefix, e.g. 0x8131c8a8, > it prints correct output, but fails with coredump. > For example: > ovn-sbctl --uuid lflow-list sw1 0x8131c8a8 > > Datapath: "sw1" (4b1e53d8-9f0f-4768-b4a6-6cbc58a4bfda) Pipeline: egress > uuid=0x8131c8a8, table=10(ls_out_port_sec_l2 ), priority=100 , > match=(eth.mcast), action=(output;) > free(): invalid pointer > [2] 616553 abort (core dumped) ovn-sbctl --uuid dump-flows sw1 0x8131c8a8 > > This patch fixes it. > > Signed-off-by: Alexey Roytman <roytman@il.ibm.com> > --- > utilities/ovn-sbctl.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c > index e3aa7a68e..099d31f08 100644 > --- a/utilities/ovn-sbctl.c > +++ b/utilities/ovn-sbctl.c > @@ -764,6 +764,12 @@ sbctl_lflow_cmp(const void *a_, const void *b_) > return cmp ? cmp : strcmp(a->actions, b->actions); > } > > +static bool > +is_uuid_with_prefix(const char *uuid) > +{ > + return uuid[0] == '0' && (uuid[1] == 'x' || uuid[1] == 'X'); > +} > + > static char * > parse_partial_uuid(char *s) > { > @@ -774,8 +780,7 @@ parse_partial_uuid(char *s) > > /* Accept a full or partial UUID prefixed by 0x, since "ovs-ofctl > * dump-flows" prints cookies prefixed by 0x. */ > - if (s[0] == '0' && (s[1] == 'x' || s[1] == 'X') > - && uuid_is_partial_string(s + 2)) { > + if (is_uuid_with_prefix(s) && uuid_is_partial_string(s + 2)) { > return s + 2; > } > > @@ -799,8 +804,11 @@ is_partial_uuid_match(const struct uuid *uuid, const char *match) > * from UUIDs, and cookie values are printed without leading zeros because > * they're just numbers. */ > const char *s1 = strip_leading_zero(uuid_s); > - const char *s2 = strip_leading_zero(match); > - > + const char *s2 = match; > + if (is_uuid_with_prefix(s2)) { > + s2 = s2 + 2; > + } > + s2 = strip_leading_zero(s2); > return !strncmp(s1, s2, strlen(s2)); > } > > @@ -1139,7 +1147,6 @@ cmd_lflow_list(struct ctl_context *ctx) > ctl_fatal("%s is not a UUID or the beginning of a UUID", > ctx->argv[i]); > } > - ctx->argv[i] = s; Since you no longer assign s, this section could be re-written to get rid of s: if (!parse_partial_uuid(ctx->argv[i])) { ctl_fatal("%s is not a UUID or the beginning of a UUID", ctx->argv[i]; } This then snowballs to the parse_partial_uuid() function. The reason the crash was happening was because parse_partial_uuid() was returning a pointer to the middle of an allocated string. We could increase the safety of parse_partial_uuid() by having it return a boolean instead of a dangerous pointer. This way, there is no chance of misusing the string it returns since it no longer will return a string. > } > > struct vconn *vconn = sbctl_open_vconn(&ctx->options); >
diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c index e3aa7a68e..099d31f08 100644 --- a/utilities/ovn-sbctl.c +++ b/utilities/ovn-sbctl.c @@ -764,6 +764,12 @@ sbctl_lflow_cmp(const void *a_, const void *b_) return cmp ? cmp : strcmp(a->actions, b->actions); } +static bool +is_uuid_with_prefix(const char *uuid) +{ + return uuid[0] == '0' && (uuid[1] == 'x' || uuid[1] == 'X'); +} + static char * parse_partial_uuid(char *s) { @@ -774,8 +780,7 @@ parse_partial_uuid(char *s) /* Accept a full or partial UUID prefixed by 0x, since "ovs-ofctl * dump-flows" prints cookies prefixed by 0x. */ - if (s[0] == '0' && (s[1] == 'x' || s[1] == 'X') - && uuid_is_partial_string(s + 2)) { + if (is_uuid_with_prefix(s) && uuid_is_partial_string(s + 2)) { return s + 2; } @@ -799,8 +804,11 @@ is_partial_uuid_match(const struct uuid *uuid, const char *match) * from UUIDs, and cookie values are printed without leading zeros because * they're just numbers. */ const char *s1 = strip_leading_zero(uuid_s); - const char *s2 = strip_leading_zero(match); - + const char *s2 = match; + if (is_uuid_with_prefix(s2)) { + s2 = s2 + 2; + } + s2 = strip_leading_zero(s2); return !strncmp(s1, s2, strlen(s2)); } @@ -1139,7 +1147,6 @@ cmd_lflow_list(struct ctl_context *ctx) ctl_fatal("%s is not a UUID or the beginning of a UUID", ctx->argv[i]); } - ctx->argv[i] = s; } struct vconn *vconn = sbctl_open_vconn(&ctx->options);