| Message ID | 20240820135549.1726748-7-mkp@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | Address clang analyze warnings. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 20 Aug 2024, at 15:55, Mike Pattrick wrote: > Currently the error conditions in vconn_dump_flows() don't handle > freeing memory in a consistent fashion. This can make it possible to > reference memory after it's freed. > > This patch attempts to handle errors consistently. Error conditions will > always cause memory to be freed and then that memory will never be > referenced. > > Fixes: d444a914fdbd ("ovn-trace: New --ovs option to also print OpenFlow flows.") > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > lib/vconn.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/lib/vconn.c b/lib/vconn.c > index e9603432d..dcede1656 100644 > --- a/lib/vconn.c > +++ b/lib/vconn.c > @@ -1017,6 +1017,7 @@ recv_flow_stats_reply(struct vconn *vconn, ovs_be32 send_xid, > VLOG_WARN_RL(&rl, "received bad reply: %s", > ofp_to_string(reply->data, reply->size, > NULL, NULL, 1)); > + ofpbuf_delete(reply); > return EPROTO; > } > } > @@ -1041,6 +1042,7 @@ recv_flow_stats_reply(struct vconn *vconn, ovs_be32 send_xid, > default: > VLOG_WARN_RL(&rl, "parse error in reply (%s)", > ofperr_to_string(retval)); > + ofpbuf_delete(reply); If we delete the buffer, we should also set the replyp back to NULL, for consistency, this will also remove the additional check below for ofpbuf_delete(). > return EPROTO; > } > } > @@ -1079,16 +1081,19 @@ vconn_dump_flows(struct vconn *vconn, If we set the replyp to NULL above, i.e. relative to the state it’s in, we do not need the changes below. > struct ofputil_flow_stats *fs = &fses[n_fses]; > error = recv_flow_stats_reply(vconn, send_xid, &reply, fs, &ofpacts); > if (error) { > - if (error == EOF) { > - error = 0; > - } > break; > } > fs->ofpacts = xmemdup(fs->ofpacts, fs->ofpacts_len); > n_fses++; > } > + > ofpbuf_uninit(&ofpacts); > - ofpbuf_delete(reply); > + > + if (!error) { > + ofpbuf_delete(reply); > + } else if (error == EOF) { > + error = 0; > + } > > if (error) { > for (size_t i = 0; i < n_fses; i++) { > -- > 2.43.5 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/vconn.c b/lib/vconn.c index e9603432d..dcede1656 100644 --- a/lib/vconn.c +++ b/lib/vconn.c @@ -1017,6 +1017,7 @@ recv_flow_stats_reply(struct vconn *vconn, ovs_be32 send_xid, VLOG_WARN_RL(&rl, "received bad reply: %s", ofp_to_string(reply->data, reply->size, NULL, NULL, 1)); + ofpbuf_delete(reply); return EPROTO; } } @@ -1041,6 +1042,7 @@ recv_flow_stats_reply(struct vconn *vconn, ovs_be32 send_xid, default: VLOG_WARN_RL(&rl, "parse error in reply (%s)", ofperr_to_string(retval)); + ofpbuf_delete(reply); return EPROTO; } } @@ -1079,16 +1081,19 @@ vconn_dump_flows(struct vconn *vconn, struct ofputil_flow_stats *fs = &fses[n_fses]; error = recv_flow_stats_reply(vconn, send_xid, &reply, fs, &ofpacts); if (error) { - if (error == EOF) { - error = 0; - } break; } fs->ofpacts = xmemdup(fs->ofpacts, fs->ofpacts_len); n_fses++; } + ofpbuf_uninit(&ofpacts); - ofpbuf_delete(reply); + + if (!error) { + ofpbuf_delete(reply); + } else if (error == EOF) { + error = 0; + } if (error) { for (size_t i = 0; i < n_fses; i++) {
Currently the error conditions in vconn_dump_flows() don't handle freeing memory in a consistent fashion. This can make it possible to reference memory after it's freed. This patch attempts to handle errors consistently. Error conditions will always cause memory to be freed and then that memory will never be referenced. Fixes: d444a914fdbd ("ovn-trace: New --ovs option to also print OpenFlow flows.") Signed-off-by: Mike Pattrick <mkp@redhat.com> --- lib/vconn.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)