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

[Justin Pettit]

On Feb 1, 2010, at 9:34 AM, Ben Pfaff wrote:

> On Fri, Jan 29, 2010 at 04:48:29PM -0800, Justin Pettit wrote:
>> +    struct ofp_desc_stats *ods;
>> +
>> +    if (mfr_desc) {
>> +        if (strlen(mfr_desc) >= sizeof ods->mfr_desc) {
>> +            VLOG_WARN("truncating mfr_desc, must be less than %d characters",
>> +                    sizeof ods->mfr_desc);
>> +        }
> 
> This really seems like an exceedingly unlikely problem, but I guess it
> doesn't hurt to warn about it.

As a user, I'd definitely want to be warned about my strings being truncated--even if they are 255 or 31 character limits.

> %zu is the correct printf specifier for size_t.

Thanks.  Fixed.

>> +        free(p->mfr_desc);
>> +        p->mfr_desc = xstrdup(mfr_desc);
> 
> Might want to add an xstrndup() function to util.[ch] so that you could
> just copy the first n bytes with xstrndup(string, n);

Probably not a bad idea.

> (strndup actually made it into POSIX 2008:
> http://www.opengroup.org/onlinepubs/9699919799/)

Woo-hoo!  

>> @@ -2437,11 +2459,11 @@ handle_desc_stats_request(struct ofproto *p, struct ofconn *ofconn,
>> 
>>     msg = start_stats_reply(request, sizeof *ods);
>>     ods = append_stats_reply(sizeof *ods, ofconn, &msg);
>> -    strncpy(ods->mfr_desc, p->manufacturer, sizeof ods->mfr_desc);
>> -    strncpy(ods->hw_desc, p->hardware, sizeof ods->hw_desc);
>> -    strncpy(ods->sw_desc, p->software, sizeof ods->sw_desc);
>> -    strncpy(ods->serial_num, p->serial, sizeof ods->serial_num);
>> -    strncpy(ods->dp_desc, p->dp_desc, sizeof ods->dp_desc);
>> +    ovs_strlcpy(ods->mfr_desc, p->mfr_desc, sizeof ods->mfr_desc);
>> +    ovs_strlcpy(ods->hw_desc, p->hw_desc, sizeof ods->hw_desc);
>> +    ovs_strlcpy(ods->sw_desc, p->sw_desc, sizeof ods->sw_desc);
>> +    ovs_strlcpy(ods->serial_num, p->serial_desc, sizeof ods->serial_num);
>> +    ovs_strlcpy(ods->dp_desc, p->dp_desc, sizeof ods->dp_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.  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.

--Justin