| Message ID | 1457141746-13557-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
It might be super slow, but how about running the test suite with valgrind and ofpbuf code changed so that each put reallocates the memory? That way we would not have to be lucky about the timing/placement of reallocations to find these bugs? Jarno > On Mar 4, 2016, at 5:35 PM, William Tu <u9012063@gmail.com> wrote: > > Address pointed by header_ptr might be free'd due to realloc > happened at ofpbuf_put_uninit() and ofpbuf_put_hex(). Reported > by valgrind 379: check TCP flags expression in OXM and NXM. > > Invalid write of size 4 > nx_match_from_string_raw (nx-match.c:1510) > nx_match_from_string (nx-match.c:1538) > ofctl_parse_nxm__ (ovs-ofctl.c:3325) > ovs_cmdl_run_command (command-line.c:121) > main (ovs-ofctl.c:137) > > Address 0x7a2cc40 is 0 bytes inside a block of size 64 free'd > free (vg_replace_malloc.c:530) > ofpbuf_resize__ (ofpbuf.c:246) > ofpbuf_put (ofpbuf.c:386) > ofpbuf_put_hex (ofpbuf.c:414) > nx_match_from_string_raw (nx-match.c:1488) > nx_match_from_string (nx-match.c:1538) > ofctl_parse_nxm__ (ovs-ofctl.c:3325) > > Signed-off-by: William Tu <u9012063@gmail.com> > --- > lib/nx-match.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/lib/nx-match.c b/lib/nx-match.c > index 4999b1a..2615f8c 100644 > --- a/lib/nx-match.c > +++ b/lib/nx-match.c > @@ -1470,6 +1470,7 @@ nx_match_from_string_raw(const char *s, struct ofpbuf *b) > ovs_be64 *header_ptr; > int name_len; > size_t n; > + ptrdiff_t header_ptr_offset; > > name = s; > name_len = strcspn(s, "("); > @@ -1485,6 +1486,7 @@ nx_match_from_string_raw(const char *s, struct ofpbuf *b) > s += name_len + 1; > > header_ptr = ofpbuf_put_uninit(b, nxm_header_len(header)); > + header_ptr_offset = (char *)header_ptr - (char *)b->data; > s = ofpbuf_put_hex(b, s, &n); > if (n != nxm_field_bytes(header)) { > const struct mf_field *field = mf_from_oxm_header(header); > @@ -1507,6 +1509,10 @@ nx_match_from_string_raw(const char *s, struct ofpbuf *b) > } > } > nw_header = htonll(header); > + > + /* header_ptr might be free'd due to > + * ofpbuf_put_uninit() and ofpbuf_put_hex(). */ > + header_ptr = (ovs_be64 *)((char *)b->data + header_ptr_offset); > memcpy(header_ptr, &nw_header, nxm_header_len(header)); > > if (nxm_hasmask(header)) { > -- > 2.5.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On 4 March 2016 at 17:35, William Tu <u9012063@gmail.com> wrote: > Address pointed by header_ptr might be free'd due to realloc > happened at ofpbuf_put_uninit() and ofpbuf_put_hex(). Reported > by valgrind 379: check TCP flags expression in OXM and NXM. > > Invalid write of size 4 > nx_match_from_string_raw (nx-match.c:1510) > nx_match_from_string (nx-match.c:1538) > ofctl_parse_nxm__ (ovs-ofctl.c:3325) > ovs_cmdl_run_command (command-line.c:121) > main (ovs-ofctl.c:137) > > Address 0x7a2cc40 is 0 bytes inside a block of size 64 free'd > free (vg_replace_malloc.c:530) > ofpbuf_resize__ (ofpbuf.c:246) > ofpbuf_put (ofpbuf.c:386) > ofpbuf_put_hex (ofpbuf.c:414) > nx_match_from_string_raw (nx-match.c:1488) > nx_match_from_string (nx-match.c:1538) > ofctl_parse_nxm__ (ovs-ofctl.c:3325) > > Signed-off-by: William Tu <u9012063@gmail.com> As a general policy, I think it's better to avoid adding pointer arithmetic throughout the codebase where possible. I've proposed a slightly different fix here making use of ofpbuf->header, although I'm not 100% on whether it's fine for this code to overwrite it: http://openvswitch.org/pipermail/dev/2016-March/067313.html
diff --git a/lib/nx-match.c b/lib/nx-match.c index 4999b1a..2615f8c 100644 --- a/lib/nx-match.c +++ b/lib/nx-match.c @@ -1470,6 +1470,7 @@ nx_match_from_string_raw(const char *s, struct ofpbuf *b) ovs_be64 *header_ptr; int name_len; size_t n; + ptrdiff_t header_ptr_offset; name = s; name_len = strcspn(s, "("); @@ -1485,6 +1486,7 @@ nx_match_from_string_raw(const char *s, struct ofpbuf *b) s += name_len + 1; header_ptr = ofpbuf_put_uninit(b, nxm_header_len(header)); + header_ptr_offset = (char *)header_ptr - (char *)b->data; s = ofpbuf_put_hex(b, s, &n); if (n != nxm_field_bytes(header)) { const struct mf_field *field = mf_from_oxm_header(header); @@ -1507,6 +1509,10 @@ nx_match_from_string_raw(const char *s, struct ofpbuf *b) } } nw_header = htonll(header); + + /* header_ptr might be free'd due to + * ofpbuf_put_uninit() and ofpbuf_put_hex(). */ + header_ptr = (ovs_be64 *)((char *)b->data + header_ptr_offset); memcpy(header_ptr, &nw_header, nxm_header_len(header)); if (nxm_hasmask(header)) {
Address pointed by header_ptr might be free'd due to realloc happened at ofpbuf_put_uninit() and ofpbuf_put_hex(). Reported by valgrind 379: check TCP flags expression in OXM and NXM. Invalid write of size 4 nx_match_from_string_raw (nx-match.c:1510) nx_match_from_string (nx-match.c:1538) ofctl_parse_nxm__ (ovs-ofctl.c:3325) ovs_cmdl_run_command (command-line.c:121) main (ovs-ofctl.c:137) Address 0x7a2cc40 is 0 bytes inside a block of size 64 free'd free (vg_replace_malloc.c:530) ofpbuf_resize__ (ofpbuf.c:246) ofpbuf_put (ofpbuf.c:386) ofpbuf_put_hex (ofpbuf.c:414) nx_match_from_string_raw (nx-match.c:1488) nx_match_from_string (nx-match.c:1538) ofctl_parse_nxm__ (ovs-ofctl.c:3325) Signed-off-by: William Tu <u9012063@gmail.com> --- lib/nx-match.c | 6 ++++++ 1 file changed, 6 insertions(+)