A previous commit attempted to fix the error path when the actions nested within clone provoked an error. However, this commit just introduced a new problem in another case, since it made ofpacts_pull_openflow_actions__() restore a previously valid pointer to data that might have been reallocated.
This commit takes another approach. Instead of trying to restore anything at all, it just defines ofpacts_pull_openflow_actions__() to clear the output buffer when there's an error. It seems that this is less error prone. Most of the callers don't care; this commit fixes up the ones that do. Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=9975 Fixes: 20cdd1dbd546 ("ofp-actions: Avoid assertion failure for clone(ct(...bad actions...)).") Signed-off-by: Ben Pfaff <b...@ovn.org> --- lib/ofp-actions.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 6adb55d23b02..a80a4a308dba 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -5888,6 +5888,9 @@ decode_NXAST_RAW_CLONE(const struct ext_action_header *eah, ofp_version, 1u << OVSINST_OFPIT11_APPLY_ACTIONS, out, 0, vl_mff_map, tlv_bitmap); + if (error) { + return error; + } clone = ofpbuf_push_uninit(out, sizeof *clone); out->header = &clone->ofpact; ofpact_finish_CLONE(out, &clone); @@ -6479,7 +6482,7 @@ decode_NXAST_RAW_CT(const struct nx_action_conntrack *nac, out, OFPACT_CT, vl_mff_map, tlv_bitmap); if (error) { - goto out; + return error; } conntrack = ofpbuf_push_uninit(out, sizeof(*conntrack)); @@ -7500,8 +7503,6 @@ ofpacts_pull_openflow_actions__(struct ofpbuf *openflow, uint64_t *ofpacts_tlv_bitmap) { const struct ofp_action_header *actions; - void *orig_data = ofpacts->data; - size_t orig_size = ofpacts->size; enum ofperr error; if (actions_len % OFP_ACTION_ALIGN != 0) { @@ -7525,21 +7526,15 @@ ofpacts_pull_openflow_actions__(struct ofpbuf *openflow, outer_action); } if (error) { - /* Back out changes to 'ofpacts'. (Normally, decoding would only - * append to 'ofpacts', so that one would expect only to need to - * restore 'ofpacts->size', but some action parsing temporarily pulls - * off data from the start of 'ofpacts' and may not properly re-push it - * on error paths.) */ - ofpacts->data = orig_data; - ofpacts->size = orig_size; + ofpbuf_clear(ofpacts); } return error; } /* Attempts to convert 'actions_len' bytes of OpenFlow actions from the front * of 'openflow' into ofpacts. On success, appends the converted actions to - * 'ofpacts'; on failure, 'ofpacts' is unchanged (but might be reallocated) . - * Returns 0 if successful, otherwise an OpenFlow error. + * 'ofpacts'; on failure, clears 'ofpacts'. Returns 0 if successful, otherwise + * an OpenFlow error. * * Actions are processed according to their OpenFlow version which * is provided in the 'version' parameter. -- 2.16.1 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev