ofp-actions: Prevent integer overflow in decode.
[cascardo/ovs.git] / lib / ofp-actions.c
index 2a71e7a..905469b 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
+ * Copyright (c) 2008-2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -30,6 +30,7 @@
 #include "nx-match.h"
 #include "odp-netlink.h"
 #include "ofp-parse.h"
+#include "ofp-prop.h"
 #include "ofp-util.h"
 #include "ofpbuf.h"
 #include "unaligned.h"
@@ -273,6 +274,8 @@ enum ofp_raw_action_type {
 
     /* NX1.0+(20): struct nx_action_controller. */
     NXAST_RAW_CONTROLLER,
+    /* NX1.0+(37): struct nx_action_controller2, ... */
+    NXAST_RAW_CONTROLLER2,
 
     /* NX1.0+(22): struct nx_action_write_metadata. */
     NXAST_RAW_WRITE_METADATA,
@@ -364,6 +367,72 @@ static char * OVS_WARN_UNUSED_RESULT ofpacts_parse_copy(
     enum ofputil_protocol *usable_protocols,
     bool allow_instructions, enum ofpact_type outer_action);
 
+/* Returns the ofpact following 'ofpact', except that if 'ofpact' contains
+ * nested ofpacts it returns the first one. */
+struct ofpact *
+ofpact_next_flattened(const struct ofpact *ofpact)
+{
+    switch (ofpact->type) {
+    case OFPACT_OUTPUT:
+    case OFPACT_GROUP:
+    case OFPACT_CONTROLLER:
+    case OFPACT_ENQUEUE:
+    case OFPACT_OUTPUT_REG:
+    case OFPACT_BUNDLE:
+    case OFPACT_SET_FIELD:
+    case OFPACT_SET_VLAN_VID:
+    case OFPACT_SET_VLAN_PCP:
+    case OFPACT_STRIP_VLAN:
+    case OFPACT_PUSH_VLAN:
+    case OFPACT_SET_ETH_SRC:
+    case OFPACT_SET_ETH_DST:
+    case OFPACT_SET_IPV4_SRC:
+    case OFPACT_SET_IPV4_DST:
+    case OFPACT_SET_IP_DSCP:
+    case OFPACT_SET_IP_ECN:
+    case OFPACT_SET_IP_TTL:
+    case OFPACT_SET_L4_SRC_PORT:
+    case OFPACT_SET_L4_DST_PORT:
+    case OFPACT_REG_MOVE:
+    case OFPACT_STACK_PUSH:
+    case OFPACT_STACK_POP:
+    case OFPACT_DEC_TTL:
+    case OFPACT_SET_MPLS_LABEL:
+    case OFPACT_SET_MPLS_TC:
+    case OFPACT_SET_MPLS_TTL:
+    case OFPACT_DEC_MPLS_TTL:
+    case OFPACT_PUSH_MPLS:
+    case OFPACT_POP_MPLS:
+    case OFPACT_SET_TUNNEL:
+    case OFPACT_SET_QUEUE:
+    case OFPACT_POP_QUEUE:
+    case OFPACT_FIN_TIMEOUT:
+    case OFPACT_RESUBMIT:
+    case OFPACT_LEARN:
+    case OFPACT_CONJUNCTION:
+    case OFPACT_MULTIPATH:
+    case OFPACT_NOTE:
+    case OFPACT_EXIT:
+    case OFPACT_SAMPLE:
+    case OFPACT_UNROLL_XLATE:
+    case OFPACT_DEBUG_RECIRC:
+    case OFPACT_METER:
+    case OFPACT_CLEAR_ACTIONS:
+    case OFPACT_WRITE_METADATA:
+    case OFPACT_GOTO_TABLE:
+    case OFPACT_NAT:
+        return ofpact_next(ofpact);
+
+    case OFPACT_CT:
+        return ofpact_get_CT(ofpact)->actions;
+
+    case OFPACT_WRITE_ACTIONS:
+        return ofpact_get_WRITE_ACTIONS(ofpact)->actions;
+    }
+
+    OVS_NOT_REACHED();
+}
+
 /* Pull off existing actions or instructions. Used by nesting actions to keep
  * ofpacts_parse() oblivious of actions nesting.
  *
@@ -378,7 +447,6 @@ ofpacts_pull(struct ofpbuf *ofpacts)
 {
     size_t ofs;
 
-    ofpact_pad(ofpacts);
     ofs = ofpacts->size;
     ofpbuf_pull(ofpacts, ofs);
 
@@ -560,6 +628,31 @@ struct nx_action_controller {
 };
 OFP_ASSERT(sizeof(struct nx_action_controller) == 16);
 
+/* Properties for NXAST_CONTROLLER2.
+ *
+ * For more information on the effect of NXAC2PT_PAUSE, see the large comment
+ * on NXT_PACKET_IN2 in nicira-ext.h */
+enum nx_action_controller2_prop_type {
+    NXAC2PT_MAX_LEN,            /* ovs_be16 max bytes to send (default all). */
+    NXAC2PT_CONTROLLER_ID,      /* ovs_be16 dest controller ID (default 0). */
+    NXAC2PT_REASON,             /* uint8_t reason (OFPR_*), default 0. */
+    NXAC2PT_USERDATA,           /* Data to copy into NXPINT_USERDATA. */
+    NXAC2PT_PAUSE,              /* Flag to pause pipeline to resume later. */
+};
+
+/* Action structure for NXAST_CONTROLLER2.
+ *
+ * This replacement for NXAST_CONTROLLER makes it extensible via properties. */
+struct nx_action_controller2 {
+    ovs_be16 type;                  /* OFPAT_VENDOR. */
+    ovs_be16 len;                   /* Length is 16 or more. */
+    ovs_be32 vendor;                /* NX_VENDOR_ID. */
+    ovs_be16 subtype;               /* NXAST_CONTROLLER2. */
+    uint8_t zeros[6];               /* Must be zero. */
+    /* Followed by NXAC2PT_* properties. */
+};
+OFP_ASSERT(sizeof(struct nx_action_controller2) == 16);
+
 static enum ofperr
 decode_NXAST_RAW_CONTROLLER(const struct nx_action_controller *nac,
                             enum ofp_version ofp_version OVS_UNUSED,
@@ -568,9 +661,81 @@ decode_NXAST_RAW_CONTROLLER(const struct nx_action_controller *nac,
     struct ofpact_controller *oc;
 
     oc = ofpact_put_CONTROLLER(out);
+    oc->ofpact.raw = NXAST_RAW_CONTROLLER;
     oc->max_len = ntohs(nac->max_len);
     oc->controller_id = ntohs(nac->controller_id);
     oc->reason = nac->reason;
+    ofpact_finish(out, &oc->ofpact);
+
+    return 0;
+}
+
+static enum ofperr
+decode_NXAST_RAW_CONTROLLER2(const struct nx_action_controller2 *nac2,
+                             enum ofp_version ofp_version OVS_UNUSED,
+                             struct ofpbuf *out)
+{
+    if (!is_all_zeros(nac2->zeros, sizeof nac2->zeros)) {
+        return OFPERR_NXBRC_MUST_BE_ZERO;
+    }
+
+    size_t start_ofs = out->size;
+    struct ofpact_controller *oc = ofpact_put_CONTROLLER(out);
+    oc->ofpact.raw = NXAST_RAW_CONTROLLER2;
+    oc->max_len = UINT16_MAX;
+    oc->reason = OFPR_ACTION;
+
+    struct ofpbuf properties;
+    ofpbuf_use_const(&properties, nac2, ntohs(nac2->len));
+    ofpbuf_pull(&properties, sizeof *nac2);
+
+    while (properties.size > 0) {
+        struct ofpbuf payload;
+        uint64_t type;
+
+        enum ofperr error = ofpprop_pull(&properties, &payload, &type);
+        if (error) {
+            return error;
+        }
+
+        switch (type) {
+        case NXAC2PT_MAX_LEN:
+            error = ofpprop_parse_u16(&payload, &oc->max_len);
+            break;
+
+        case NXAC2PT_CONTROLLER_ID:
+            error = ofpprop_parse_u16(&payload, &oc->controller_id);
+            break;
+
+        case NXAC2PT_REASON: {
+            uint8_t u8;
+            error = ofpprop_parse_u8(&payload, &u8);
+            oc->reason = u8;
+            break;
+        }
+
+        case NXAC2PT_USERDATA:
+            out->size = start_ofs + OFPACT_CONTROLLER_SIZE;
+            ofpbuf_put(out, payload.msg, ofpbuf_msgsize(&payload));
+            oc = ofpbuf_at_assert(out, start_ofs, sizeof *oc);
+            oc->userdata_len = ofpbuf_msgsize(&payload);
+            break;
+
+        case NXAC2PT_PAUSE:
+            oc->pause = true;
+            break;
+
+        default:
+            error = OFPPROP_UNKNOWN(false, "NXAST_RAW_CONTROLLER2", type);
+            break;
+        }
+        if (error) {
+            return error;
+        }
+    }
+
+    ofpact_finish(out, &oc->ofpact);
+
     return 0;
 }
 
@@ -579,12 +744,37 @@ encode_CONTROLLER(const struct ofpact_controller *controller,
                   enum ofp_version ofp_version OVS_UNUSED,
                   struct ofpbuf *out)
 {
-    struct nx_action_controller *nac;
+    if (controller->userdata_len
+        || controller->pause
+        || controller->ofpact.raw == NXAST_RAW_CONTROLLER2) {
+        size_t start_ofs = out->size;
+        put_NXAST_CONTROLLER2(out);
+        if (controller->max_len != UINT16_MAX) {
+            ofpprop_put_u16(out, NXAC2PT_MAX_LEN, controller->max_len);
+        }
+        if (controller->controller_id != 0) {
+            ofpprop_put_u16(out, NXAC2PT_CONTROLLER_ID,
+                            controller->controller_id);
+        }
+        if (controller->reason != OFPR_ACTION) {
+            ofpprop_put_u8(out, NXAC2PT_REASON, controller->reason);
+        }
+        if (controller->userdata_len != 0) {
+            ofpprop_put(out, NXAC2PT_USERDATA, controller->userdata,
+                        controller->userdata_len);
+        }
+        if (controller->pause) {
+            ofpprop_put_flag(out, NXAC2PT_PAUSE);
+        }
+        pad_ofpat(out, start_ofs);
+    } else {
+        struct nx_action_controller *nac;
 
-    nac = put_NXAST_CONTROLLER(out);
-    nac->max_len = htons(controller->max_len);
-    nac->controller_id = htons(controller->controller_id);
-    nac->reason = controller->reason;
+        nac = put_NXAST_CONTROLLER(out);
+        nac->max_len = htons(controller->max_len);
+        nac->controller_id = htons(controller->controller_id);
+        nac->reason = controller->reason;
+    }
 }
 
 static char * OVS_WARN_UNUSED_RESULT
@@ -594,6 +784,8 @@ parse_CONTROLLER(char *arg, struct ofpbuf *ofpacts,
     enum ofp_packet_in_reason reason = OFPR_ACTION;
     uint16_t controller_id = 0;
     uint16_t max_len = UINT16_MAX;
+    const char *userdata = NULL;
+    bool pause = false;
 
     if (!arg[0]) {
         /* Use defaults. */
@@ -620,6 +812,10 @@ parse_CONTROLLER(char *arg, struct ofpbuf *ofpacts,
                 if (error) {
                     return error;
                 }
+            } else if (!strcmp(name, "userdata")) {
+                userdata = value;
+            } else if (!strcmp(name, "pause")) {
+                pause = true;
             } else {
                 return xasprintf("unknown key \"%s\" parsing controller "
                                  "action", name);
@@ -627,7 +823,7 @@ parse_CONTROLLER(char *arg, struct ofpbuf *ofpacts,
         }
     }
 
-    if (reason == OFPR_ACTION && controller_id == 0) {
+    if (reason == OFPR_ACTION && controller_id == 0 && !userdata && !pause) {
         struct ofpact_output *output;
 
         output = ofpact_put_OUTPUT(ofpacts);
@@ -640,15 +836,41 @@ parse_CONTROLLER(char *arg, struct ofpbuf *ofpacts,
         controller->max_len = max_len;
         controller->reason = reason;
         controller->controller_id = controller_id;
+        controller->pause = pause;
+
+        if (userdata) {
+            size_t start_ofs = ofpacts->size;
+            const char *end = ofpbuf_put_hex(ofpacts, userdata, NULL);
+            if (*end) {
+                return xstrdup("bad hex digit in `controller' "
+                               "action `userdata'");
+            }
+            size_t userdata_len = ofpacts->size - start_ofs;
+            controller = ofpacts->header;
+            controller->userdata_len = userdata_len;
+        }
+        ofpact_finish(ofpacts, &controller->ofpact);
     }
 
     return NULL;
 }
 
+static void
+format_hex_arg(struct ds *s, const uint8_t *data, size_t len)
+{
+    for (size_t i = 0; i < len; i++) {
+        if (i) {
+            ds_put_char(s, '.');
+        }
+        ds_put_format(s, "%02"PRIx8, data[i]);
+    }
+}
+
 static void
 format_CONTROLLER(const struct ofpact_controller *a, struct ds *s)
 {
-    if (a->reason == OFPR_ACTION && a->controller_id == 0) {
+    if (a->reason == OFPR_ACTION && !a->controller_id && !a->userdata_len
+        && !a->pause) {
         ds_put_format(s, "CONTROLLER:%"PRIu16, a->max_len);
     } else {
         enum ofp_packet_in_reason reason = a->reason;
@@ -667,6 +889,14 @@ format_CONTROLLER(const struct ofpact_controller *a, struct ds *s)
         if (a->controller_id != 0) {
             ds_put_format(s, "id=%"PRIu16",", a->controller_id);
         }
+        if (a->userdata_len) {
+            ds_put_cstr(s, "userdata=");
+            format_hex_arg(s, a->userdata, a->userdata_len);
+            ds_put_char(s, ',');
+        }
+        if (a->pause) {
+            ds_put_cstr(s, "pause,");
+        }
         ds_chomp(s, ',');
         ds_put_char(s, ')');
     }
@@ -824,18 +1054,16 @@ decode_NXAST_RAW_OUTPUT_REG2(const struct nx_action_output_reg2 *naor,
                              struct ofpbuf *out)
 {
     struct ofpact_output_reg *output_reg;
-    enum ofperr error;
-    struct ofpbuf b;
-
     output_reg = ofpact_put_OUTPUT_REG(out);
     output_reg->ofpact.raw = NXAST_RAW_OUTPUT_REG2;
     output_reg->src.ofs = nxm_decode_ofs(naor->ofs_nbits);
     output_reg->src.n_bits = nxm_decode_n_bits(naor->ofs_nbits);
     output_reg->max_len = ntohs(naor->max_len);
 
-    ofpbuf_use_const(&b, naor, ntohs(naor->len));
+    struct ofpbuf b = ofpbuf_const_initializer(naor, ntohs(naor->len));
     ofpbuf_pull(&b, OBJECT_OFFSETOF(naor, pad));
-    error = nx_pull_header(&b, &output_reg->src.field, NULL);
+
+    enum ofperr error = nx_pull_header(&b, &output_reg->src.field, NULL);
     if (error) {
         return error;
     }
@@ -1025,10 +1253,10 @@ decode_bundle(bool load, const struct nx_action_bundle *nab,
     for (i = 0; i < bundle->n_slaves; i++) {
         uint16_t ofp_port = ntohs(((ovs_be16 *)(nab + 1))[i]);
         ofpbuf_put(ofpacts, &ofp_port, sizeof ofp_port);
+        bundle = ofpacts->header;
     }
 
-    bundle = ofpacts->header;
-    ofpact_update_len(ofpacts, &bundle->ofpact);
+    ofpact_finish(ofpacts, &bundle->ofpact);
 
     if (!error) {
         error = bundle_check(bundle, OFPP_MAX, NULL);
@@ -1946,20 +2174,17 @@ decode_copy_field__(ovs_be16 src_offset, ovs_be16 dst_offset, ovs_be16 n_bits,
                     const void *action, ovs_be16 action_len, size_t oxm_offset,
                     struct ofpbuf *ofpacts)
 {
-    struct ofpact_reg_move *move;
-    enum ofperr error;
-    struct ofpbuf b;
-
-    move = ofpact_put_REG_MOVE(ofpacts);
+    struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts);
     move->ofpact.raw = ONFACT_RAW13_COPY_FIELD;
     move->src.ofs = ntohs(src_offset);
     move->src.n_bits = ntohs(n_bits);
     move->dst.ofs = ntohs(dst_offset);
     move->dst.n_bits = ntohs(n_bits);
 
-    ofpbuf_use_const(&b, action, ntohs(action_len));
+    struct ofpbuf b = ofpbuf_const_initializer(action, ntohs(action_len));
     ofpbuf_pull(&b, oxm_offset);
-    error = nx_pull_header(&b, &move->src.field, NULL);
+
+    enum ofperr error = nx_pull_header(&b, &move->src.field, NULL);
     if (error) {
         return error;
     }
@@ -2000,20 +2225,17 @@ decode_NXAST_RAW_REG_MOVE(const struct nx_action_reg_move *narm,
                           enum ofp_version ofp_version OVS_UNUSED,
                           struct ofpbuf *ofpacts)
 {
-    struct ofpact_reg_move *move;
-    enum ofperr error;
-    struct ofpbuf b;
-
-    move = ofpact_put_REG_MOVE(ofpacts);
+    struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts);
     move->ofpact.raw = NXAST_RAW_REG_MOVE;
     move->src.ofs = ntohs(narm->src_ofs);
     move->src.n_bits = ntohs(narm->n_bits);
     move->dst.ofs = ntohs(narm->dst_ofs);
     move->dst.n_bits = ntohs(narm->n_bits);
 
-    ofpbuf_use_const(&b, narm, ntohs(narm->len));
+    struct ofpbuf b = ofpbuf_const_initializer(narm, ntohs(narm->len));
     ofpbuf_pull(&b, sizeof *narm);
-    error = nx_pull_header(&b, &move->src.field, NULL);
+
+    enum ofperr error = nx_pull_header(&b, &move->src.field, NULL);
     if (error) {
         return error;
     }
@@ -2177,16 +2399,12 @@ static enum ofperr
 decode_ofpat_set_field(const struct ofp12_action_set_field *oasf,
                        bool may_mask, struct ofpbuf *ofpacts)
 {
-    struct ofpact_set_field *sf;
-    enum ofperr error;
-    struct ofpbuf b;
-
-    sf = ofpact_put_SET_FIELD(ofpacts);
-
-    ofpbuf_use_const(&b, oasf, ntohs(oasf->len));
+    struct ofpbuf b = ofpbuf_const_initializer(oasf, ntohs(oasf->len));
     ofpbuf_pull(&b, OBJECT_OFFSETOF(oasf, pad));
-    error = nx_pull_entry(&b, &sf->field, &sf->value,
-                          may_mask ? &sf->mask : NULL);
+
+    struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts);
+    enum ofperr error = nx_pull_entry(&b, &sf->field, &sf->value,
+                                      may_mask ? &sf->mask : NULL);
     if (error) {
         return (error == OFPERR_OFPBMC_BAD_MASK
                 ? OFPERR_OFPBAC_BAD_SET_MASK
@@ -2288,16 +2506,13 @@ decode_NXAST_RAW_REG_LOAD2(const struct nx_action_reg_load2 *narl,
                            enum ofp_version ofp_version OVS_UNUSED,
                            struct ofpbuf *out)
 {
-    struct ofpact_set_field *sf;
-    enum ofperr error;
-    struct ofpbuf b;
-
-    sf = ofpact_put_SET_FIELD(out);
+    struct ofpact_set_field *sf = ofpact_put_SET_FIELD(out);
     sf->ofpact.raw = NXAST_RAW_REG_LOAD2;
 
-    ofpbuf_use_const(&b, narl, ntohs(narl->len));
+    struct ofpbuf b = ofpbuf_const_initializer(narl, ntohs(narl->len));
     ofpbuf_pull(&b, OBJECT_OFFSETOF(narl, pad));
-    error = nx_pull_entry(&b, &sf->field, &sf->value, &sf->mask);
+
+    enum ofperr error = nx_pull_entry(&b, &sf->field, &sf->value, &sf->mask);
     if (error) {
         return error;
     }
@@ -2734,14 +2949,12 @@ static enum ofperr
 decode_stack_action(const struct nx_action_stack *nasp,
                     struct ofpact_stack *stack_action)
 {
-    enum ofperr error;
-    struct ofpbuf b;
-
     stack_action->subfield.ofs = ntohs(nasp->offset);
 
-    ofpbuf_use_const(&b, nasp, sizeof *nasp);
+    struct ofpbuf b = ofpbuf_const_initializer(nasp, sizeof *nasp);
     ofpbuf_pull(&b, OBJECT_OFFSETOF(nasp, pad));
-    error = nx_pull_header(&b, &stack_action->subfield.field, NULL);
+    enum ofperr error = nx_pull_header(&b, &stack_action->subfield.field,
+                                       NULL);
     if (error) {
         return error;
     }
@@ -2868,7 +3081,7 @@ decode_OFPAT_RAW_DEC_NW_TTL(struct ofpbuf *out)
     ids->n_controllers = 1;
     ofpbuf_put(out, &id, sizeof id);
     ids = out->header;
-    ofpact_update_len(out, &ids->ofpact);
+    ofpact_finish(out, &ids->ofpact);
     return error;
 }
 
@@ -2905,7 +3118,7 @@ decode_NXAST_RAW_DEC_TTL_CNT_IDS(const struct nx_action_cnt_ids *nac_ids,
         ids = out->header;
     }
 
-    ofpact_update_len(out, &ids->ofpact);
+    ofpact_finish(out, &ids->ofpact);
 
     return 0;
 }
@@ -2944,7 +3157,7 @@ parse_noargs_dec_ttl(struct ofpbuf *ofpacts)
     ofpbuf_put(ofpacts, &id, sizeof id);
     ids = ofpacts->header;
     ids->n_controllers++;
-    ofpact_update_len(ofpacts, &ids->ofpact);
+    ofpact_finish(ofpacts, &ids->ofpact);
 }
 
 static char * OVS_WARN_UNUSED_RESULT
@@ -2971,7 +3184,7 @@ parse_DEC_TTL(char *arg, struct ofpbuf *ofpacts,
             return xstrdup("dec_ttl_cnt_ids: expected at least one controller "
                            "id.");
         }
-        ofpact_update_len(ofpacts, &ids->ofpact);
+        ofpact_finish(ofpacts, &ids->ofpact);
     }
     return NULL;
 }
@@ -3989,7 +4202,7 @@ decode_NXAST_RAW_LEARN(const struct nx_action_learn *nal,
             get_subfield(spec->n_bits, &p, &spec->dst);
         }
     }
-    ofpact_update_len(ofpacts, &learn->ofpact);
+    ofpact_finish(ofpacts, &learn->ofpact);
 
     if (!is_all_zeros(p, (char *) end - (char *) p)) {
         return OFPERR_OFPBAC_BAD_ARGUMENT;
@@ -4310,10 +4523,10 @@ decode_NXAST_RAW_NOTE(const struct nx_action_note *nan,
     unsigned int length;
 
     length = ntohs(nan->len) - offsetof(struct nx_action_note, note);
-    note = ofpact_put(out, OFPACT_NOTE,
-                      offsetof(struct ofpact_note, data) + length);
+    note = ofpact_put_NOTE(out);
     note->length = length;
-    memcpy(note->data, nan->note, length);
+    ofpbuf_put(out, nan->note, length);
+    ofpact_finish(out, out->header);
 
     return 0;
 }
@@ -4336,47 +4549,24 @@ static char * OVS_WARN_UNUSED_RESULT
 parse_NOTE(const char *arg, struct ofpbuf *ofpacts,
            enum ofputil_protocol *usable_protocols OVS_UNUSED)
 {
-    struct ofpact_note *note;
-
-    note = ofpact_put_NOTE(ofpacts);
-    while (*arg != '\0') {
-        uint8_t byte;
-        bool ok;
-
-        if (*arg == '.') {
-            arg++;
-        }
-        if (*arg == '\0') {
-            break;
-        }
-
-        byte = hexits_value(arg, 2, &ok);
-        if (!ok) {
-            return xstrdup("bad hex digit in `note' argument");
-        }
-        ofpbuf_put(ofpacts, &byte, 1);
-
-        note = ofpacts->header;
-        note->length++;
-
-        arg += 2;
+    size_t start_ofs = ofpacts->size;
+    ofpact_put_NOTE(ofpacts);
+    arg = ofpbuf_put_hex(ofpacts, arg, NULL);
+    if (arg[0]) {
+        return xstrdup("bad hex digit in `note' argument");
     }
-    ofpact_update_len(ofpacts, &note->ofpact);
+    struct ofpact_note *note = ofpbuf_at_assert(ofpacts, start_ofs,
+                                                sizeof *note);
+    note->length = ofpacts->size - (start_ofs + sizeof *note);
+    ofpact_finish(ofpacts, &note->ofpact);
     return NULL;
 }
 
 static void
 format_NOTE(const struct ofpact_note *a, struct ds *s)
 {
-    size_t i;
-
     ds_put_cstr(s, "note:");
-    for (i = 0; i < a->length; i++) {
-        if (i) {
-            ds_put_char(s, '.');
-        }
-        ds_put_format(s, "%02"PRIx8, a->data[i]);
-    }
+    format_hex_arg(s, a->data, a->length);
 }
 \f
 /* Exit action. */
@@ -4428,10 +4618,10 @@ parse_UNROLL_XLATE(char *arg OVS_UNUSED, struct ofpbuf *ofpacts OVS_UNUSED,
 }
 
 static void
-format_UNROLL_XLATE(const struct ofpact_unroll_xlate *a OVS_UNUSED,
-                    struct ds *s)
+format_UNROLL_XLATE(const struct ofpact_unroll_xlate *a, struct ds *s)
 {
-    ds_put_cstr(s, "unroll_xlate");
+    ds_put_format(s, "unroll_xlate(table=%"PRIu8", cookie=%"PRIu64")",
+                  a->rule_table_id, ntohll(a->rule_cookie));
 }
 \f
 /* Action structure for NXAST_SAMPLE.
@@ -4742,13 +4932,10 @@ decode_NXAST_RAW_CT(const struct nx_action_conntrack *nac,
                     enum ofp_version ofp_version, struct ofpbuf *out)
 {
     const size_t ct_offset = ofpacts_pull(out);
-    struct ofpact_conntrack *conntrack;
-    struct ofpbuf openflow;
-    int error = 0;
-
-    conntrack = ofpact_put_CT(out);
+    struct ofpact_conntrack *conntrack = ofpact_put_CT(out);
     conntrack->flags = ntohs(nac->flags);
-    error = decode_ct_zone(nac, conntrack);
+
+    int error = decode_ct_zone(nac, conntrack);
     if (error) {
         goto out;
     }
@@ -4757,7 +4944,8 @@ decode_NXAST_RAW_CT(const struct nx_action_conntrack *nac,
 
     ofpbuf_pull(out, sizeof(*conntrack));
 
-    ofpbuf_use_const(&openflow, nac + 1, ntohs(nac->len) - sizeof(*nac));
+    struct ofpbuf openflow = ofpbuf_const_initializer(
+        nac + 1, ntohs(nac->len) - sizeof(*nac));
     error = ofpacts_pull_openflow_actions__(&openflow, openflow.size,
                                             ofp_version,
                                             1u << OVSINST_OFPIT11_APPLY_ACTIONS,
@@ -4768,7 +4956,7 @@ decode_NXAST_RAW_CT(const struct nx_action_conntrack *nac,
 
     conntrack = ofpbuf_push_uninit(out, sizeof(*conntrack));
     out->header = &conntrack->ofpact;
-    ofpact_update_len(out, &conntrack->ofpact);
+    ofpact_finish(out, &conntrack->ofpact);
 
     if (conntrack->ofpact.len > sizeof(*conntrack)
         && !(conntrack->flags & NX_CT_F_COMMIT)) {
@@ -4864,7 +5052,6 @@ parse_CT(char *arg, struct ofpbuf *ofpacts,
             const size_t nat_offset = ofpacts_pull(ofpacts);
 
             error = parse_NAT(value, ofpacts, usable_protocols);
-            ofpact_pad(ofpacts);
             /* Update CT action pointer and length. */
             ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
             oc = ofpacts->header;
@@ -4880,7 +5067,6 @@ parse_CT(char *arg, struct ofpbuf *ofpacts,
             error = ofpacts_parse_copy(value, ofpacts, &usable_protocols2,
                                        false, OFPACT_CT);
             *usable_protocols &= usable_protocols2;
-            ofpact_pad(ofpacts);
             ofpacts->header = ofpbuf_push_uninit(ofpacts, exec_offset);
             oc = ofpacts->header;
         } else {
@@ -4891,7 +5077,7 @@ parse_CT(char *arg, struct ofpbuf *ofpacts,
         }
     }
 
-    ofpact_update_len(ofpacts, &oc->ofpact);
+    ofpact_finish(ofpacts, &oc->ofpact);
     ofpbuf_push_uninit(ofpacts, ct_offset);
     return error;
 }
@@ -5537,9 +5723,7 @@ static enum ofperr
 ofpacts_decode(const void *actions, size_t actions_len,
                enum ofp_version ofp_version, struct ofpbuf *ofpacts)
 {
-    struct ofpbuf openflow;
-
-    ofpbuf_use_const(&openflow, actions, actions_len);
+    struct ofpbuf openflow = ofpbuf_const_initializer(actions, actions_len);
     while (openflow.size) {
         const struct ofp_action_header *action = openflow.data;
         enum ofp_raw_action_type raw;
@@ -5556,8 +5740,6 @@ ofpacts_decode(const void *actions, size_t actions_len,
             return error;
         }
     }
-
-    ofpact_pad(ofpacts);
     return 0;
 }
 
@@ -5570,12 +5752,9 @@ ofpacts_pull_openflow_actions__(struct ofpbuf *openflow,
                                 enum ofpact_type outer_action)
 {
     const struct ofp_action_header *actions;
+    size_t orig_size = ofpacts->size;
     enum ofperr error;
 
-    if (!outer_action) {
-        ofpbuf_clear(ofpacts);
-    }
-
     if (actions_len % OFP_ACTION_ALIGN != 0) {
         VLOG_WARN_RL(&rl, "OpenFlow message actions length %u is not a "
                      "multiple of %d", actions_len, OFP_ACTION_ALIGN);
@@ -5592,21 +5771,21 @@ ofpacts_pull_openflow_actions__(struct ofpbuf *openflow,
 
     error = ofpacts_decode(actions, actions_len, version, ofpacts);
     if (error) {
-        ofpbuf_clear(ofpacts);
+        ofpacts->size = orig_size;
         return error;
     }
 
     error = ofpacts_verify(ofpacts->data, ofpacts->size, allowed_ovsinsts,
                            outer_action);
     if (error) {
-        ofpbuf_clear(ofpacts);
+        ofpacts->size = orig_size;
     }
     return error;
 }
 
-/* Attempts to convert 'actions_len' bytes of OpenFlow actions from the
- * front of 'openflow' into ofpacts.  On success, replaces any existing content
- * in 'ofpacts' by the converted ofpacts; on failure, clears 'ofpacts'.
+/* 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.
  *
  * Actions are processed according to their OpenFlow version which
@@ -6168,6 +6347,7 @@ ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
     const struct ofp11_instruction *insts[N_OVS_INSTRUCTIONS];
     enum ofperr error;
 
+    ofpbuf_clear(ofpacts);
     if (version == OFP10_VERSION) {
         return ofpacts_pull_openflow_actions__(openflow, instructions_len,
                                                version,
@@ -6175,8 +6355,6 @@ ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
                                                ofpacts, 0);
     }
 
-    ofpbuf_clear(ofpacts);
-
     if (instructions_len % OFP11_INSTRUCTION_ALIGN != 0) {
         VLOG_WARN_RL(&rl, "OpenFlow message instructions length %u is not a "
                      "multiple of %d",
@@ -6231,10 +6409,7 @@ ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
         struct ofpact_nest *on;
         const struct ofp_action_header *actions;
         size_t actions_len;
-        size_t start;
-
-        ofpact_pad(ofpacts);
-        start = ofpacts->size;
+        size_t start = ofpacts->size;
         ofpact_put(ofpacts, OFPACT_WRITE_ACTIONS,
                    offsetof(struct ofpact_nest, actions));
         get_actions_from_instruction(insts[OVSINST_OFPIT11_WRITE_ACTIONS],
@@ -6664,16 +6839,20 @@ ofpacts_check_consistency(struct ofpact ofpacts[], size_t ofpacts_len,
             : 0);
 }
 
-static const struct mf_field *
-ofpact_get_mf_field(enum ofpact_type type, const void *ofpact)
+/* Returns the destination field that 'ofpact' would write to, or NULL
+ * if the action would not write to an mf_field. */
+const struct mf_field *
+ofpact_get_mf_dst(const struct ofpact *ofpact)
 {
-    if (type == OFPACT_SET_FIELD) {
-        const struct ofpact_set_field *orl = ofpact;
+    if (ofpact->type == OFPACT_SET_FIELD) {
+        const struct ofpact_set_field *orl;
 
+        orl = CONTAINER_OF(ofpact, struct ofpact_set_field, ofpact);
         return orl->field;
-    } else if (type == OFPACT_REG_MOVE) {
-        const struct ofpact_reg_move *orm = ofpact;
+    } else if (ofpact->type == OFPACT_REG_MOVE) {
+        const struct ofpact_reg_move *orm;
 
+        orm = CONTAINER_OF(ofpact, struct ofpact_reg_move, ofpact);
         return orm->dst.field;
     }
 
@@ -6698,7 +6877,7 @@ field_requires_ct(enum mf_field_id field)
 static enum ofperr
 ofpacts_verify_nested(const struct ofpact *a, enum ofpact_type outer_action)
 {
-    const struct mf_field *field = ofpact_get_mf_field(a->type, a);
+    const struct mf_field *field = ofpact_get_mf_dst(a);
 
     if (field && field_requires_ct(field->id) && outer_action != OFPACT_CT) {
         VLOG_WARN("cannot set CT fields outside of ct action");
@@ -7096,7 +7275,7 @@ ofpacts_output_to_port(const struct ofpact *ofpacts, size_t ofpacts_len,
 {
     const struct ofpact *a;
 
-    OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
+    OFPACT_FOR_EACH_FLATTENED (a, ofpacts, ofpacts_len) {
         if (ofpact_outputs_to_port(a, port)) {
             return true;
         }
@@ -7113,7 +7292,7 @@ ofpacts_output_to_group(const struct ofpact *ofpacts, size_t ofpacts_len,
 {
     const struct ofpact *a;
 
-    OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
+    OFPACT_FOR_EACH_FLATTENED (a, ofpacts, ofpacts_len) {
         if (a->type == OFPACT_GROUP
             && ofpact_get_GROUP(a)->group_id == group_id) {
             return true;
@@ -7200,7 +7379,6 @@ ofpact_put(struct ofpbuf *ofpacts, enum ofpact_type type, size_t len)
 {
     struct ofpact *ofpact;
 
-    ofpact_pad(ofpacts);
     ofpacts->header = ofpbuf_put_uninit(ofpacts, len);
     ofpact = ofpacts->header;
     ofpact_init(ofpact, type, len);
@@ -7216,41 +7394,22 @@ ofpact_init(struct ofpact *ofpact, enum ofpact_type type, size_t len)
     ofpact->len = len;
 }
 \f
-/* Updates 'ofpact->len' to the number of bytes in the tail of 'ofpacts'
- * starting at 'ofpact'.
- *
- * This is the correct way to update a variable-length ofpact's length after
- * adding the variable-length part of the payload.  (See the large comment
- * near the end of ofp-actions.h for more information.) */
+/* Finishes composing a variable-length action (begun using
+ * ofpact_put_<NAME>()), by padding the action to a multiple of OFPACT_ALIGNTO
+ * bytes and updating its embedded length field.  See the large comment near
+ * the end of ofp-actions.h for more information. */
 void
-ofpact_update_len(struct ofpbuf *ofpacts, struct ofpact *ofpact)
+ofpact_finish(struct ofpbuf *ofpacts, struct ofpact *ofpact)
 {
-    ovs_assert(ofpact == ofpacts->header);
-    ofpact->len = (char *) ofpbuf_tail(ofpacts) - (char *) ofpact;
-}
+    ptrdiff_t len;
 
-/* Pads out 'ofpacts' to a multiple of OFPACT_ALIGNTO bytes in length.  Each
- * ofpact_put_<ENUM>() calls this function automatically beforehand, but the
- * client must call this itself after adding the final ofpact to an array of
- * them.
- *
- * (The consequences of failing to call this function are probably not dire.
- * OFPACT_FOR_EACH will calculate a pointer beyond the end of the ofpacts, but
- * not dereference it.  That's undefined behavior, technically, but it will not
- * cause a real problem on common systems.  Still, it seems better to call
- * it.) */
-void
-ofpact_pad(struct ofpbuf *ofpacts)
-{
-    unsigned int pad = PAD_SIZE(ofpacts->size, OFPACT_ALIGNTO);
-    if (pad) {
-        ofpbuf_put_zeros(ofpacts, pad);
-    }
+    ovs_assert(ofpact == ofpacts->header);
+    len = (char *) ofpbuf_tail(ofpacts) - (char *) ofpact;
+    ovs_assert(len <= UINT16_MAX);
+    ofpact->len = len;
+    ofpbuf_padto(ofpacts, OFPACT_ALIGN(ofpacts->size));
 }
 \f
-
-
-
 static char * OVS_WARN_UNUSED_RESULT
 ofpact_parse(enum ofpact_type type, char *value, struct ofpbuf *ofpacts,
              enum ofputil_protocol *usable_protocols)
@@ -7354,7 +7513,6 @@ ofpacts_parse__(char *str, struct ofpbuf *ofpacts,
         }
         prev_inst = inst;
     }
-    ofpact_pad(ofpacts);
 
     if (drop && ofpacts->size) {
         return xstrdup("\"drop\" must not be accompanied by any other action "