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

[Ben Pfaff]

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.

%zu is the correct printf specifier for size_t.

> +        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);

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

>      }
> -    if (hardware) {
> -        free(p->hardware);
> -        p->hardware = xstrdup(hardware);
> +    if (hw_desc) {
> +        if (strlen(hw_desc) >= sizeof ods->hw_desc) {
> +            VLOG_WARN("truncating hw_desc, must be less than %d characters",
> +                    sizeof ods->hw_desc);
> +        }
> +        free(p->hw_desc);
> +        p->hw_desc = xstrdup(hw_desc);

See, if you'd used a helper function you only would have had to change
one place :-)

> @@ -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.