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

[Julien ÉLIE]

Hi Yuriy,

> snprintf() return -1 on error, and *value larger than supplied buffer
> size* if formatted string will not fit in supplied buffer.
> If you add/subtract snprintf() return value without validating its
> range, this will lead up to disaster.
> As I don't use inn, patch is compile-tested only, passes `make check`.

Thanks a lot for your patches, that I did not forget and marked
to be integrated to the next release.  I've at last taken the time
to do that work, and they will show up in INN 2.6.1.


--- lib/timer.c    (revision 9984)
+++ lib/timer.c (working copy)

     if (off == len) {
         warn("timer log too long while processing %s",
              TMRlabel(labels, timer->id));
-        return 0;
+        return off;
     }

You even find a genuine bug, fixed with the above patch you submitted.
Thanks for that catch!
 


--- 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)"?
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) {
+                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?
I would just not change anything there.

Thanks again for your work,

-- 
Julien ÉLIE

« Petite annonce : Abeille épouserait frelon. Lune de miel
  assurée. »