[PATCH] fixes for apr_vformatter and apr_snprintf

[Nuutti Kotivuori <na...@iki.fi>]

I went through apr_snprintf trying to find whether the value returne
was including or not including a null-terminator. On the way, I
stumbled upon some nastyness. This is what you get when you don't just
write a test program and see how it behaves.

There are a few fixes I made, outlined below.

* apr_vformatter behaved incorrectly when the output filled the buffer
exactly. It still tried to flush, which caused it to return -1.

* apr_snprintf behaved incorrectly when the output was truncated. It
returned the length that was passed in, but that is including the
null-terminator.

* apr_vsnprintf did the same.

* added testcases for the proper behaviour.

Then an additional nitpick would be that it should be mentioned
somewhere that apr_vformatter never touches vbuff.endpos, that it is
never written to. It is not clear from it's documentation.

That's it, hope these get in soon. And hope no-one depends on broken
behaviour.

-- Naked

Patch:
diff --exclude=CVS -Nur apr.old/strings/apr_snprintf.c apr/strings/apr_snprintf.c
--- apr.old/strings/apr_snprintf.c	Tue May  7 08:20:55 2002
+++ apr/strings/apr_snprintf.c	Wed Jul 10 23:37:02 2002
@@ -1197,10 +1197,6 @@
         fmt++;
     }
     vbuff->curpos = sp;
-    if (sp >= bep) {
-     if (flush_func(vbuff))
-         return -1;
-    }
     return cc;
 }
 
@@ -1231,7 +1227,7 @@
     cc = apr_vformatter(snprintf_flush, &vbuff, format, ap);
     va_end(ap);
     *vbuff.curpos = '\0';
-    return (cc == -1) ? (int)len : cc;
+    return (cc == -1) ? (int)len - 1 : cc;
 }
 
 
@@ -1249,5 +1245,5 @@
     vbuff.endpos = buf + len - 1;
     cc = apr_vformatter(snprintf_flush, &vbuff, format, ap);
     *vbuff.curpos = '\0';
-    return (cc == -1) ? (int)len : cc;
+    return (cc == -1) ? (int)len - 1 : cc;
 }
diff --exclude=CVS -Nur apr.old/test/Makefile.in apr/test/Makefile.in
--- apr.old/test/Makefile.in	Fri Jul  5 11:49:55 2002
+++ apr/test/Makefile.in	Wed Jul 10 23:37:29 2002
@@ -46,7 +46,8 @@
 	testatomic@EXEEXT@ \
 	testpools@EXEEXT@ \
 	testmutexscope@EXEEXT@ \
-	testtable@EXEEXT@
+	testtable@EXEEXT@ \
+	testsnprintf@EXEEXT@
 
 
 TARGETS = $(PROGRAMS) $(NONPORTABLE)
@@ -203,5 +204,8 @@
 
 testtable@EXEEXT@: testtable.lo $(LOCAL_LIBS)
 	$(LINK) testtable.lo $(LOCAL_LIBS) $(ALL_LIBS)
+
+testsnprintf@EXEEXT@: testsnprintf.lo $(LOCAL_LIBS)
+	$(LINK) testsnprintf.lo $(LOCAL_LIBS) $(ALL_LIBS)
 
 # DO NOT REMOVE
diff --exclude=CVS -Nur apr.old/test/testsnprintf.c apr/test/testsnprintf.c
--- apr.old/test/testsnprintf.c	Thu Jan  1 02:00:00 1970
+++ apr/test/testsnprintf.c	Wed Jul 10 23:35:37 2002
@@ -0,0 +1,241 @@
+/* ====================================================================
+ * The Apache Software License, Version 1.1
+ *
+ * Copyright (c) 2000-2002 The Apache Software Foundation.  All rights
+ * reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in
+ *    the documentation and/or other materials provided with the
+ *    distribution.
+ *
+ * 3. The end-user documentation included with the redistribution,
+ *    if any, must include the following acknowledgment:
+ *       "This product includes software developed by the
+ *        Apache Software Foundation (http://www.apache.org/)."
+ *    Alternately, this acknowledgment may appear in the software itself,
+ *    if and wherever such third-party acknowledgments normally appear.
+ *
+ * 4. The names "Apache" and "Apache Software Foundation" must
+ *    not be used to endorse or promote products derived from this
+ *    software without prior written permission. For written
+ *    permission, please contact apache@apache.org.
+ *
+ * 5. Products derived from this software may not be called "Apache",
+ *    nor may "Apache" appear in their name, without prior written
+ *    permission of the Apache Software Foundation.
+ *
+ * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESSED OR IMPLIED
+ * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED.  IN NO EVENT SHALL THE APACHE SOFTWARE FOUNDATION OR
+ * ITS CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation.  For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ */
+
+#include <assert.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "apr_general.h"
+#include "apr_strings.h"
+#include "apr_lib.h"
+
+static int test_flush(apr_vformatter_buff_t *vbuff)
+{
+    return -1;
+}
+
+static int call_vformatter(apr_pool_t *p, apr_vformatter_buff_t *vbuff, const char *format, ...)
+{
+  int cc;
+  va_list ap;
+
+  va_start(ap, format);
+  cc = apr_vformatter(test_flush, vbuff, format, ap);
+  va_end(ap);
+  return cc;
+}
+
+/*
+ * After a call to vformatter, curpos should be one past the last
+ * character written and return value should be the amount of
+ * characters written.
+ */
+static void test_vformatter1(apr_pool_t *p)
+{
+  apr_vformatter_buff_t vbuff;
+  char *testbuf;
+  int cc;
+
+  /* Make room for 8 character */
+  testbuf = apr_palloc(p, 8);
+  /* But endpos is never written, so we only have 7 to use. */
+  vbuff.curpos = testbuf;
+  vbuff.endpos = testbuf + 7;
+  /* Generate 5 characters. */
+  cc = call_vformatter(p, &vbuff, "x%dx", 100);
+  
+  /* Did we write five characters? */
+  assert(cc == 5);
+  /* Is the endpos still the same? */
+  assert(vbuff.endpos == testbuf + 7);
+  /* Is the curpos one past last character written? */
+  assert(vbuff.curpos == testbuf + 5);
+}
+
+/*
+ * If we overshoot the buffer, curpos should be equal to endpos and
+ * return value should be -1.
+ */
+static void test_vformatter2(apr_pool_t *p)
+{
+  apr_vformatter_buff_t vbuff;
+  char *testbuf;
+  int cc;
+
+  /* Make room for 8 character */
+  testbuf = apr_palloc(p, 8);
+  /* But endpos is never written, so we only have 7 to use. */
+  vbuff.curpos = testbuf;
+  vbuff.endpos = testbuf + 7;
+  /* Generate 10 characters. */
+  cc = call_vformatter(p, &vbuff, "x%dx67890", 100);
+  
+  /* Did we overshoot the buffer? */
+  assert(cc == -1);
+  /* Is the endpos still the same? */
+  assert(vbuff.endpos == testbuf + 7);
+  /* Is the curpos the same as endpos? */
+  assert(vbuff.curpos == vbuff.endpos);
+}
+
+/*
+ * If we just fill the buffer, curpos should be equal to endpos and
+ * return value should be the amount of characters written.
+ */
+static void test_vformatter3(apr_pool_t *p)
+{
+  apr_vformatter_buff_t vbuff;
+  char *testbuf;
+  int cc;
+
+  /* Make room for 8 character */
+  testbuf = apr_palloc(p, 8);
+  /* But endpos is never written, so we only have 7 to use. */
+  vbuff.curpos = testbuf;
+  vbuff.endpos = testbuf + 7;
+  /* Generate 7 characters. */
+  cc = call_vformatter(p, &vbuff, "x%dx67", 100);
+  
+  /* Did we write seven characters? */
+  assert(cc == 7);
+  /* Is the endpos still the same? */
+  assert(vbuff.endpos == testbuf + 7);
+  /* Is the curpos the same as endpos? */
+  assert(vbuff.curpos == vbuff.endpos);
+}
+
+/*
+ * After snprintf, the buffer should be null terminated and the return
+ * value should be the amount of characters written, not including the
+ * null-terminator.
+ */
+static void test_snprintf1(apr_pool_t *p)
+{
+  char *testbuf;
+  int cc;
+
+  /* Make room for 8 character */
+  testbuf = apr_palloc(p, 8);
+  /* Generate 5 characters. */
+  cc = apr_snprintf(testbuf, 8, "x%dx", 100);
+  
+  /* Did we write five characters? */
+  assert(cc == 5);
+  /* Is the final character right? */
+  assert(testbuf[4] == 'x');
+  /* Is the buffer null terminated? */
+  assert(testbuf[5] == '\0');
+}
+
+/*
+ * If we get truncated, the buffer should be null terminated and the
+ * return value should be one less than len.
+ */
+static void test_snprintf2(apr_pool_t *p)
+{
+  char *testbuf;
+  int cc;
+
+  /* Make room for 8 character */
+  testbuf = apr_palloc(p, 8);
+  /* Generate 10 characters. */
+  cc = apr_snprintf(testbuf, 8, "x%dx67890", 100);
+  
+  /* Did we write seven characters? */
+  assert(cc == 7);
+  /* Is the final character right? */
+  assert(testbuf[6] == '7');
+  /* Is the buffer null terminated? */
+  assert(testbuf[7] == '\0');
+}
+
+/*
+ * If we just fill the buffer, the buffer should be null terminated
+ * and the return value should be one less than len.
+ */
+static void test_snprintf3(apr_pool_t *p)
+{
+  char *testbuf;
+  int cc;
+
+  /* Make room for 8 character */
+  testbuf = apr_palloc(p, 8);
+  /* Generate 7 characters. */
+  cc = apr_snprintf(testbuf, 8, "x%dx67", 100);
+  
+  /* Did we write seven characters? */
+  assert(cc == 7);
+  /* Is the final character right? */
+  assert(testbuf[6] == '7');
+  /* Is the buffer null terminated? */
+  assert(testbuf[7] == '\0');
+}
+
+int main(int argc, const char * const argv[])
+{
+    apr_pool_t *p;
+
+    apr_initialize();
+    atexit(apr_terminate);
+    apr_pool_create(&p, NULL);
+
+    test_vformatter1(p);
+    test_vformatter2(p);
+    test_vformatter3(p);
+    test_snprintf1(p);
+    test_snprintf2(p);
+    test_snprintf3(p);
+
+    return 0;
+}