[ovs-dev] [PATCH 4/4] ofproto: Always terminate OpenFlow description strings
blp at nicira.com
Tue Feb 2 15:04:04 PST 2010
On Tue, Feb 02, 2010 at 02:43:51PM -0800, Justin Pettit wrote:
> On Feb 1, 2010, at 9:34 AM, Ben Pfaff wrote:
> > On Fri, Jan 29, 2010 at 04:48:29PM -0800, Justin Pettit wrote:
> >> - strncpy(ods->mfr_desc, p->manufacturer, sizeof ods->mfr_desc);
> >> + ovs_strlcpy(ods->mfr_desc, p->mfr_desc, sizeof ods->mfr_desc);
> > I don't think this is an improvement. ovs_strlcpy() fails to pad out on
> > the right with null bytes and it will truncate the last byte of a value
> > that fills the entire field.
> > strncpy() is really the right choice here in my opinion.
> As we discussed in person, the problem seems to be a possible
> ambiguity in the OpenFlow spec. I was trying to fix the issue of
> putting in a non-null-terminated string. I think the intention of the
> spec (and I believe I defined it) is that these descriptions are
> supposed to be null-terminated.
OK. I defer to you, then.
> There is a spot where it could be interpreted otherwise. In any case,
> I'll seek consensus on the openflow-spec mailing list. If they are
> supposed to be null-terminated, do you still object to my
> implementation? The start_stats_reply() function should zero out the
> unused portion of the message, so it's still padded to the right with
> null bytes.
I don't see where start_stats_reply() zeroes out the unused portion of
the message. It calls make_stats_reply(), which calls
put_openflow_xid() with sizeof(struct ofp_stats_reply), which just
zeroes out that much of the buffer. And append_stats_reply() just uses
ofpbuf_put_uninit(), so I don't see it happening there.
If you make sure that the buffer is zeroed out one way or another, then
ovs_strlcpy() is fine by me.
More information about the dev