Fixed xstrndup() documentation, callers. Disclosed implementation bugs.

xstrndup() does not work like strndup(3), and some callers got confused:

1. When n is the str length or less, standard strndup(str,n) copies all
   n bytes but our xstrndup(str,n) drops the last one. Thus, all callers
   must add one to the desired result length when calling xstrndup().
   Most already do, but it is often hard to see due to low code quality
   (e.g., one must remember that MAX_URL is not the maximum URL length).

2. xstrndup() also assumes that the source string is 0-terminated. This
   dangerous assumption does not contradict many official strndup(3)
   descriptions, but that lack of contradiction is actually a recently
   fixed POSIX documentation bug (i.e., correct implementations must not
   assume 0-termination): http://austingroupbugs.net/view.php?id=1019

The OutOfBoundsException bug led to truncated exception messages.

The ESI bug led to truncated 'literal strings', but I do not know what
that means in terms of user impact. That ESI fix is untested.

cachemgr.cc bug was masked by the fact that the buffer ends with \n
that is unused and stripped by the custom xstrtok() implementation.

TODO. Fix xstrndup() implementation (and rename the function so that
fixed callers do not misbehave if carelessly ported to older Squids).

diff --git a/compat/xstring.h b/compat/xstring.h
index d5283928d5c762d5b4d2fe7c4b3044875d896fee..831f2be0d3709574aec970f3b7b2b891544f3e10 100644 (file)
--- a/compat/xstring.h
+++ b/compat/xstring.h
@@ -41,7 +41,10 @@ char *xstrdup(const char *s);
 char *xstrncpy(char *dst, const char *src, size_t n);
 
 /**
- * xstrndup() - same as strndup(3).  Used for portability.
+ * xstrndup() - Somewhat similar(XXX) to strndup(3): Allocates up to n bytes,
+ * while strndup(3) copies up to n bytes and allocates up to n+1 bytes
+ * to fit the terminating character. Assumes s is 0-terminated (another XXX).
+ *
  * Never returns NULL; fatal on error.
  *
  * Sets errno to EINVAL if a NULL pointer or negative

diff --git a/src/esi/Expression.cc b/src/esi/Expression.cc
index 1254066f3fe715f2854dd8d1c595b751ea4aeef6..8d5049b72d351485566034376792660bac2cc2e3 100644 (file)
--- a/src/esi/Expression.cc
+++ b/src/esi/Expression.cc
@@ -743,7 +743,7 @@ getsymbol(const char *s, char const **endptr)
             /* Special case for zero length strings */
 
             if (t - s - 1)
-                rv.value.string = xstrndup(s + 1, t - s - 1);
+                rv.value.string = xstrndup(s + 1, t - (s + 1) + 1);
             else
                 rv.value.string = static_cast<char *>(xcalloc(1,1));
 

diff --git a/src/sbuf/Exceptions.cc b/src/sbuf/Exceptions.cc
index 5b595d5132c7cd059f9a420f89e7f538325e0b38..abb4016008cdf8c1ec57ab64ab990094f3ebcc67 100644 (file)
--- a/src/sbuf/Exceptions.cc
+++ b/src/sbuf/Exceptions.cc
@@ -25,9 +25,7 @@ OutOfBoundsException::OutOfBoundsException(const SBuf &throwingBuf,
         explanatoryText.appendf(" in file %s", aFileName);
     explanatoryText.appendf(" while accessing position %d in a SBuf long %d",
                             pos, throwingBuf.length());
-    // we can safely alias c_str as both are local to the object
-    //  and will not further manipulated.
-    message = xstrndup(explanatoryText.c_str(),explanatoryText.length());
+    message = xstrdup(explanatoryText.c_str());
 }
 
 OutOfBoundsException::~OutOfBoundsException() throw()

diff --git a/tools/cachemgr.cc b/tools/cachemgr.cc
index e16ce1bb5786917d50ee7667cc78a383ea8d91d4..bcb64d97daa0870fc9bd7b732695beb843a25926 100644 (file)
--- a/tools/cachemgr.cc
+++ b/tools/cachemgr.cc
@@ -440,7 +440,7 @@ munge_menu_line(MemBuf &out, const char *buf, cachemgr_request * req)
         return;
     }
 
-    buf_copy = x = xstrndup(buf, bufLen);
+    buf_copy = x = xstrndup(buf, bufLen+1);
 
     a = xstrtok(&x, '\t');