| Message ID | 1457143246-41495-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On 4 March 2016 at 18:00, William Tu <u9012063@gmail.com> wrote: > Address pointed by bundle could be obsolete/free'd when > realloc, called from ofpbuf_put_zero(), returns new address. > Reported by Valgrind 367: ovs-ofctl parse-flows (NXM) > > Invalid write of size 4 > bundle_parse__ (bundle.c:200) > bundle_parse_load (bundle.c:272) > parse_bundle_load (ofp-actions.c:1324) > ofpacts_parse__ (ofp-actions.c:7484) > ofpacts_parse (ofp-actions.c:7540) > ofpacts_parse_copy (ofp-actions.c:7558) > parse_ofp_str__ (ofp-parse.c:491) > parse_ofp_str (ofp-parse.c:544) > parse_ofp_flow_mod_str (ofp-parse.c:870) > > Address 0x7a4e96c is 12 bytes inside a block of size 64 free'd > free (vg_replace_malloc.c:530) > ofpbuf_resize__ (ofpbuf.c:246) (purposely add to force using new buf) > ofpbuf_put_zeros (ofpbuf.c:375) > bundle_parse__ (bundle.c:181) > bundle_parse_load (bundle.c:272) > parse_bundle_load (ofp-actions.c:1324) > ofpacts_parse__ (ofp-actions.c:7484) > ofpacts_parse (ofp-actions.c:7540) > ofpacts_parse_copy (ofp-actions.c:7558) > > Signed-off-by: William Tu <u9012063@gmail.com> > --- > lib/bundle.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/lib/bundle.c b/lib/bundle.c > index 871a724..1451e92 100644 > --- a/lib/bundle.c > +++ b/lib/bundle.c > @@ -180,6 +180,7 @@ bundle_parse__(const char *s, char **save_ptr, > } > ofpact_finish(ofpacts, &bundle->ofpact); > > + bundle = ofpacts->header; > bundle->basis = atoi(basis); I don't understand how this would trigger (or how this would fix the issue); the same line is also done directly after ofpbuf_put() inside the loop above: https://github.com/openvswitch/ovs/blob/34abaa3deaa430ca0b50453865d2e042a5132165/lib/bundle.c#L178 Can you explain it?
Hi Joe,
Thanks for your reply. The reason is that line 181:
ofpact_finish(ofpacts, &bundle->ofpact);
https://github.com/openvswitch/ovs/blob/34abaa3deaa430ca0b50453865d2e042a5132165/lib/bundle.c#L181
Could also call into ofpbuf_put_zero, which frees the memory pointed by
bundle.
Regards,
William
On Fri, Mar 4, 2016 at 6:12 PM, Joe Stringer <joe@ovn.org> wrote:
> On 4 March 2016 at 18:00, William Tu <u9012063@gmail.com> wrote:
> > Address pointed by bundle could be obsolete/free'd when
> > realloc, called from ofpbuf_put_zero(), returns new address.
> > Reported by Valgrind 367: ovs-ofctl parse-flows (NXM)
> >
> > Invalid write of size 4
> > bundle_parse__ (bundle.c:200)
> > bundle_parse_load (bundle.c:272)
> > parse_bundle_load (ofp-actions.c:1324)
> > ofpacts_parse__ (ofp-actions.c:7484)
> > ofpacts_parse (ofp-actions.c:7540)
> > ofpacts_parse_copy (ofp-actions.c:7558)
> > parse_ofp_str__ (ofp-parse.c:491)
> > parse_ofp_str (ofp-parse.c:544)
> > parse_ofp_flow_mod_str (ofp-parse.c:870)
> >
> > Address 0x7a4e96c is 12 bytes inside a block of size 64 free'd
> > free (vg_replace_malloc.c:530)
> > ofpbuf_resize__ (ofpbuf.c:246) (purposely add to force using new buf)
> > ofpbuf_put_zeros (ofpbuf.c:375)
> > bundle_parse__ (bundle.c:181)
> > bundle_parse_load (bundle.c:272)
> > parse_bundle_load (ofp-actions.c:1324)
> > ofpacts_parse__ (ofp-actions.c:7484)
> > ofpacts_parse (ofp-actions.c:7540)
> > ofpacts_parse_copy (ofp-actions.c:7558)
> >
> > Signed-off-by: William Tu <u9012063@gmail.com>
> > ---
> > lib/bundle.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/bundle.c b/lib/bundle.c
> > index 871a724..1451e92 100644
> > --- a/lib/bundle.c
> > +++ b/lib/bundle.c
> > @@ -180,6 +180,7 @@ bundle_parse__(const char *s, char **save_ptr,
> > }
> > ofpact_finish(ofpacts, &bundle->ofpact);
> >
> > + bundle = ofpacts->header;
> > bundle->basis = atoi(basis);
>
> I don't understand how this would trigger (or how this would fix the
> issue); the same line is also done directly after ofpbuf_put() inside
> the loop above:
>
> https://github.com/openvswitch/ovs/blob/34abaa3deaa430ca0b50453865d2e042a5132165/lib/bundle.c#L178
>
> Can you explain it?
>
Oh! I've been looking mostly at v2.5 lately and didn't notice how commit 2bd318dec2428ae6c0febbf79453982676ccb672 changed the "update_len" (now ofpact_finish) function. Thanks, I applied this to master. As a separate thing, I was wondering about whether it's worthwhile to do something additional to try to avoid this kind of bug in future. A couple of ideas: * Rearrange the parse/decode functions so that ofpact_finish() is the final call within the decode_FOO()/parse_FOO() functions * Amend ofpact_finish() to have an additional 'void *localptr' parameter, so that the caller has to explicitly consider whether the pointer needs to be updated. Maybe the former is enough, perhaps + amend the comment above ofpact_finish() to make it more explicit that it may reallocate the buffer (and therefore invalidate local pointers in the caller context). On 4 March 2016 at 21:27, William Tu <u9012063@gmail.com> wrote: > Hi Joe, > > Thanks for your reply. The reason is that line 181: > ofpact_finish(ofpacts, &bundle->ofpact); > https://github.com/openvswitch/ovs/blob/34abaa3deaa430ca0b50453865d2e042a5132165/lib/bundle.c#L181 > > Could also call into ofpbuf_put_zero, which frees the memory pointed by > bundle. > > Regards, > William > > On Fri, Mar 4, 2016 at 6:12 PM, Joe Stringer <joe@ovn.org> wrote: >> >> On 4 March 2016 at 18:00, William Tu <u9012063@gmail.com> wrote: >> > Address pointed by bundle could be obsolete/free'd when >> > realloc, called from ofpbuf_put_zero(), returns new address. >> > Reported by Valgrind 367: ovs-ofctl parse-flows (NXM) >> > >> > Invalid write of size 4 >> > bundle_parse__ (bundle.c:200) >> > bundle_parse_load (bundle.c:272) >> > parse_bundle_load (ofp-actions.c:1324) >> > ofpacts_parse__ (ofp-actions.c:7484) >> > ofpacts_parse (ofp-actions.c:7540) >> > ofpacts_parse_copy (ofp-actions.c:7558) >> > parse_ofp_str__ (ofp-parse.c:491) >> > parse_ofp_str (ofp-parse.c:544) >> > parse_ofp_flow_mod_str (ofp-parse.c:870) >> > >> > Address 0x7a4e96c is 12 bytes inside a block of size 64 free'd >> > free (vg_replace_malloc.c:530) >> > ofpbuf_resize__ (ofpbuf.c:246) (purposely add to force using new >> > buf) >> > ofpbuf_put_zeros (ofpbuf.c:375) >> > bundle_parse__ (bundle.c:181) >> > bundle_parse_load (bundle.c:272) >> > parse_bundle_load (ofp-actions.c:1324) >> > ofpacts_parse__ (ofp-actions.c:7484) >> > ofpacts_parse (ofp-actions.c:7540) >> > ofpacts_parse_copy (ofp-actions.c:7558) >> > >> > Signed-off-by: William Tu <u9012063@gmail.com> >> > --- >> > lib/bundle.c | 1 + >> > 1 file changed, 1 insertion(+) >> > >> > diff --git a/lib/bundle.c b/lib/bundle.c >> > index 871a724..1451e92 100644 >> > --- a/lib/bundle.c >> > +++ b/lib/bundle.c >> > @@ -180,6 +180,7 @@ bundle_parse__(const char *s, char **save_ptr, >> > } >> > ofpact_finish(ofpacts, &bundle->ofpact); >> > >> > + bundle = ofpacts->header; >> > bundle->basis = atoi(basis); >> >> I don't understand how this would trigger (or how this would fix the >> issue); the same line is also done directly after ofpbuf_put() inside >> the loop above: >> >> https://github.com/openvswitch/ovs/blob/34abaa3deaa430ca0b50453865d2e042a5132165/lib/bundle.c#L178 >> >> Can you explain it? > >
On Mon, Mar 07, 2016 at 11:12:40AM -0800, Joe Stringer wrote: > As a separate thing, I was wondering about whether it's worthwhile to > do something additional to try to avoid this kind of bug in future. A > couple of ideas: > * Rearrange the parse/decode functions so that ofpact_finish() is the > final call within the decode_FOO()/parse_FOO() functions > * Amend ofpact_finish() to have an additional 'void *localptr' > parameter, so that the caller has to explicitly consider whether the > pointer needs to be updated. > > Maybe the former is enough, perhaps + amend the comment above > ofpact_finish() to make it more explicit that it may reallocate the > buffer (and therefore invalidate local pointers in the caller > context). I came up with an idea: http://openvswitch.org/pipermail/dev/2016-April/069508.html
It reminds me another similar issue: http://openvswitch.org/pipermail/dev/2016-March/067313.html Is there some way to avoid or make it harder to forgetting to update the pointer once ofpbuf_put_uninit reallocate to newly allocated memory? Regards, William On Tue, Apr 12, 2016 at 10:00 PM, Ben Pfaff <blp@ovn.org> wrote: > On Mon, Mar 07, 2016 at 11:12:40AM -0800, Joe Stringer wrote: > > As a separate thing, I was wondering about whether it's worthwhile to > > do something additional to try to avoid this kind of bug in future. A > > couple of ideas: > > * Rearrange the parse/decode functions so that ofpact_finish() is the > > final call within the decode_FOO()/parse_FOO() functions > > * Amend ofpact_finish() to have an additional 'void *localptr' > > parameter, so that the caller has to explicitly consider whether the > > pointer needs to be updated. > > > > Maybe the former is enough, perhaps + amend the comment above > > ofpact_finish() to make it more explicit that it may reallocate the > > buffer (and therefore invalidate local pointers in the caller > > context). > > I came up with an idea: > http://openvswitch.org/pipermail/dev/2016-April/069508.html >
diff --git a/lib/bundle.c b/lib/bundle.c index 871a724..1451e92 100644 --- a/lib/bundle.c +++ b/lib/bundle.c @@ -180,6 +180,7 @@ bundle_parse__(const char *s, char **save_ptr, } ofpact_finish(ofpacts, &bundle->ofpact); + bundle = ofpacts->header; bundle->basis = atoi(basis); if (!strcasecmp(fields, "eth_src")) {
Address pointed by bundle could be obsolete/free'd when realloc, called from ofpbuf_put_zero(), returns new address. Reported by Valgrind 367: ovs-ofctl parse-flows (NXM) Invalid write of size 4 bundle_parse__ (bundle.c:200) bundle_parse_load (bundle.c:272) parse_bundle_load (ofp-actions.c:1324) ofpacts_parse__ (ofp-actions.c:7484) ofpacts_parse (ofp-actions.c:7540) ofpacts_parse_copy (ofp-actions.c:7558) parse_ofp_str__ (ofp-parse.c:491) parse_ofp_str (ofp-parse.c:544) parse_ofp_flow_mod_str (ofp-parse.c:870) Address 0x7a4e96c is 12 bytes inside a block of size 64 free'd free (vg_replace_malloc.c:530) ofpbuf_resize__ (ofpbuf.c:246) (purposely add to force using new buf) ofpbuf_put_zeros (ofpbuf.c:375) bundle_parse__ (bundle.c:181) bundle_parse_load (bundle.c:272) parse_bundle_load (ofp-actions.c:1324) ofpacts_parse__ (ofp-actions.c:7484) ofpacts_parse (ofp-actions.c:7540) ofpacts_parse_copy (ofp-actions.c:7558) Signed-off-by: William Tu <u9012063@gmail.com> --- lib/bundle.c | 1 + 1 file changed, 1 insertion(+)