[ovs-dev] [overload 1/7] ofproto: Avoid wasting memory malloc()'ing empty action sets for subrules.
Ben Pfaff
blp at nicira.com
Wed Oct 6 21:32:56 UTC 2010
On Fri, Oct 01, 2010 at 03:19:24PM -0700, Justin Pettit wrote:
> This looks like a good improvement. I'm a bit concerned about code
> that may not be defensively written to protect against "rule->actions"
> being NULL. For example the following call in flow_stats_ds_cb():
>
> ofp_print_actions(results, &rule->actions->header, act_len);
>
> This isn't currently a problem due to a check earlier in the function
> for whether it's a subrule. However, the check isn't about whether
> "rule->n_actions" is 0, so it seems like it wouldn't be hard to
> accidentally introduce a segfault.
Thanks, that's a good point. I hadn't audited for related blunders
earlier, so I've done so now. I didn't see any actual likely real
problems, but I did see any number of petty violations of C standard
technicalities. I also saw another place where we could avoid unneeded
allocation. So here's what I've got now:
--8<--------------------------cut here-------------------------->8--
From: Ben Pfaff <blp at nicira.com>
Date: Wed, 6 Oct 2010 14:21:47 -0700
Subject: [PATCH] ofproto: Avoid wasting memory malloc()'ing empty action sets for subrules.
GNU libc treats malloc(0) as malloc(1). Subrules always have an n_actions
of 0, so this code was wasting time and memory for subrules. This commit
stops doing that.
Also audits and fixes some very pedantic potential problems with null
pointers; e.g. the C standard says that NULL may not be compared with the
< operator, even if both arguments are null, and it also says that a null
pointer may not be passed to memcpy() or memcmp(), even if the length is
zero.
---
lib/ofp-util.c | 9 +++++----
ofproto/ofproto.c | 18 ++++++++++++------
2 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 7a2e17c..6d04586 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -619,9 +619,10 @@ int
validate_actions(const union ofp_action *actions, size_t n_actions,
int max_ports)
{
- const union ofp_action *a;
+ size_t i;
- for (a = actions; a < &actions[n_actions]; ) {
+ for (i = 0; i < n_actions; ) {
+ const union ofp_action *a = &actions[i];
unsigned int len = ntohs(a->header.len);
unsigned int n_slots = len / ACTION_ALIGNMENT;
unsigned int slots_left = &actions[n_actions] - a;
@@ -645,7 +646,7 @@ validate_actions(const union ofp_action *actions, size_t n_actions,
if (error) {
return error;
}
- a += n_slots;
+ i += n_slots;
}
return 0;
}
@@ -679,7 +680,7 @@ actions_first(struct actions_iterator *iter,
const union ofp_action *
actions_next(struct actions_iterator *iter)
{
- if (iter->pos < iter->end) {
+ if (iter->pos != iter->end) {
const union ofp_action *a = iter->pos;
unsigned int len = ntohs(a->header.len);
iter->pos += len / ACTION_ALIGNMENT;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 3d2989a..9a5db34 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1871,8 +1871,10 @@ rule_create(struct ofproto *ofproto, struct rule *super,
} else {
list_init(&rule->list);
}
- rule->n_actions = n_actions;
- rule->actions = xmemdup(actions, n_actions * sizeof *actions);
+ if (n_actions > 0) {
+ rule->n_actions = n_actions;
+ rule->actions = xmemdup(actions, n_actions * sizeof *actions);
+ }
netflow_flow_clear(&rule->nf_flow);
netflow_flow_update_time(ofproto->netflow, &rule->nf_flow, rule->created);
@@ -3266,7 +3268,9 @@ flow_stats_cb(struct cls_rule *rule_, void *cbdata_)
memset(ofs->pad2, 0, sizeof ofs->pad2);
ofs->packet_count = htonll(packet_count);
ofs->byte_count = htonll(byte_count);
- memcpy(ofs->actions, rule->actions, act_len);
+ if (rule->n_actions > 0) {
+ memcpy(ofs->actions, rule->actions, act_len);
+ }
}
static int
@@ -3335,7 +3339,9 @@ flow_stats_ds_cb(struct cls_rule *rule_, void *cbdata_)
ds_put_format(results, "n_packets=%"PRIu64", ", packet_count);
ds_put_format(results, "n_bytes=%"PRIu64", ", byte_count);
ofp_print_match(results, &match, true);
- ofp_print_actions(results, &rule->actions->header, act_len);
+ if (act_len > 0) {
+ ofp_print_actions(results, &rule->actions->header, act_len);
+ }
ds_put_cstr(results, "\n");
}
@@ -3763,14 +3769,14 @@ modify_flow(struct ofproto *p, const struct ofp_flow_mod *ofm,
/* If the actions are the same, do nothing. */
if (n_actions == rule->n_actions
- && !memcmp(ofm->actions, rule->actions, actions_len))
+ && (!n_actions || !memcmp(ofm->actions, rule->actions, actions_len)))
{
return 0;
}
/* Replace actions. */
free(rule->actions);
- rule->actions = xmemdup(ofm->actions, actions_len);
+ rule->actions = n_actions ? xmemdup(ofm->actions, actions_len) : NULL;
rule->n_actions = n_actions;
/* Make sure that the datapath gets updated properly. */
--
1.7.1
More information about the dev
mailing list