diff options
author | Ben Pfaff <blp@ovn.org> | 2018-08-24 14:50:14 -0700 |
---|---|---|
committer | Ben Pfaff <blp@ovn.org> | 2018-08-30 13:53:45 -0700 |
commit | 3ee9b38d5cd6c7b5dc29a5854432ffb77aa4f7db (patch) | |
tree | 69ca7de21ca9114789c1d4a6f3a2e50bb8ef30e4 /lib | |
parent | fc253483d6c874295c1a9eb7a86f44d2fe4f57ee (diff) | |
download | openvswitch-3ee9b38d5cd6c7b5dc29a5854432ffb77aa4f7db.tar.gz |
ofp-actions: Re-fix error path for parsing OpenFlow actions.
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 <blp@ovn.org>
Tested-by: Yifeng Sun <pkusunyifeng@gmail.com>
Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
Diffstat (limited to 'lib')
-rw-r--r-- | lib/ofp-actions.c | 19 |
1 files changed, 7 insertions, 12 deletions
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 6adb55d23..a80a4a308 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. |