[PATCH] fix snprintf return value misuse (and some related off-by-1/etc)

[Yuriy M. Kaminskiy]

On 01.09.2016 22:31, Julien ÉLIE wrote:

[skip]

> --- lib/timer.c    (revision 9984)
> +++ lib/timer.c (working copy)
> @@ -369,14 +387,36 @@
>
> -    off += snprintf(buf + off, len - off, "time %lu ", TMRgettime(true));
> +    rc = snprintf(buf + off, len - off, "time %lu ", TMRgettime(true));
> +    if (rc < 0) {
> +        /* do nothing */
> +    } else if ((size_t)rc >= len) {
> +        off = len;
> +    } else {
> +        off += rc;
> +    }
>
> Hmm, are you sure the check is "if ((size_t)rc >= len)"
> and not "if ((size_t)rc >= len - off)"?

Ugrh. Sorry. You are right, and your variant is correct.

> I think "len - off" is really needed because otherwise the returned
> value could be larger than the supplied buffer size of "len - off".
>
>
>
> --- lib/timer.c    (revision 9984)
> +++ lib/timer.c (working copy)
> @@ -359,6 +376,7 @@
>
>       for (i = 0; i < timer_count; i++)
> -        if (timers[i] != NULL)
> -            off += TMRsumone(labels, timers[i], buf + off, len - off);
> +        if (timers[i] != NULL) {
> +            rc = TMRsumone(labels, timers[i], buf + off, len - off);
> +            if (rc < 0) {
> +                /* do nothing */
> +            } else if ((size_t)rc >= len) {

(FWIW, same bug here, it should've been
   +            } else if ((size_t)rc >= len - off) {
)

> +                off = len;
> +            } else {
> +                off += rc;
> +            }
> +        }
>
> Is that change really necessary, as TMRsumone() can no longer return
> a negative value or a value greater than the supplied buffer size?

Well, yes, this change is redundant with changed TMRsumone().
(I tried to be consistent with snprintf()-calling code, but now that I 
looked at it again, it is not consistent with other TMRsumone call-sites 
anyway; so, this chunk should be dropped).

Thanks for review.

> I would just not change anything there.
>
> Thanks again for your work,