[ovs-dev] [PATCH 4/4] ofproto: Always terminate OpenFlow description strings

[Ben Pfaff]

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.