com: replace BSD strlcpy and strlcat

Compared to BSD strlcpy and strlcat, dp_strlcpy and dp_strlcat are
faster, never crash, and have a more useful return (DP didn't use the
strlcpy return at all), see included docs.

Adds dp_stpecpy() for efficient chain copying and dp_ustr2stp() for
copying measured byte sequences (unterminated strings) to strings.

Replaces the only use of the strlcat() return with dp_stpecpy().

Updates the forbidden string functions list.

Signed-off-by: bones_was_here <bones_was_here@xonotic.au>

diff --git a/common.c b/common.c
index 0f7ddd2b8bc11fc31d45c340ee9a3e8f085c7c7e..05bbd7c64086a6aa4b31d5203973c53449f782ed 100644 (file)
--- a/common.c
+++ b/common.c
@@ -1305,80 +1305,87 @@ COM_StringDecolorize(const char *in, size_t size_in, char *out, size_t size_out,
 #undef APPEND
 }
 
-//========================================================
-// strlcat and strlcpy, from OpenBSD
 
 /*
- * Copyright (c) 1998, 2015 Todd C. Miller <millert@openbsd.org>
- *
- * Permission to use, copy, modify, and distribute this software for any
- * purpose with or without fee is hereby granted, provided that the above
- * copyright notice and this permission notice appear in all copies.
- *
- * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
- * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
- * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
- * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
- * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
- * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
- * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+============
+String Copying
+
+The glibc implementation of memccpy is several times faster than old functions that
+copy one byte at a time (even at -O3) and its advantage increases with string length.
+============
+*/
+#ifdef WIN32
+       // memccpy() is standard in POSIX.1-2001, POSIX.1-2008, SVr4, 4.3BSD, C23.
+       // Microsoft supports it, but apparently complains if we use it.
+       #pragma warning(disable : 4996)
+#endif
+
+/** Chain-copies a string with truncation and efficiency (compared to strlcat()).
+ * The destination ends at an absolute pointer instead of a relative offset
+ * and a pointer to the \0 terminator is returned on success.
+ * Truncates, warns, and returns the end pointer on overflow or unterminated source.
+ * Guarantees \0 termination.    end = dst + sizeof(src[])
  */
+char *dp_stpecpy(char *dst, char *end, const char *src)
+{
+       char *p = (char *)memccpy(dst, src, '\0', end - dst);
 
-/*     $OpenBSD: strlcat.c,v 1.19 2019/01/25 00:19:25 millert Exp $    */
-/*     $OpenBSD: strlcpy.c,v 1.16 2019/01/25 00:19:25 millert Exp $    */
+       if (p)
+               return p - 1;
+       end[-1] = '\0';
+       Con_Printf(CON_WARN "%s: src string unterminated or truncated to %zu bytes: \"%s\"\n", __func__, dst == end ? 0 : (end - dst) - 1, dst);
+       return end;
+}
 
-size_t dp_strlcat(char *dst, const char *src, size_t dsize)
+/** Copies a measured byte sequence (unterminated string) to a null-terminated string.
+ * Returns a pointer to the \0 terminator. Guarantees \0 termination.
+ * Compared to ustr2stp(): truncates and warns on overflow.
+ */
+char *dp_ustr2stp(char *dst, size_t dsize, const char *src, size_t ssize)
 {
-       const char *odst = dst;
-       const char *osrc = src;
-       size_t n = dsize;
-       size_t dlen;
-
-       /* Find the end of dst and adjust bytes left but don't go past end. */
-       while (n-- != 0 && *dst != '\0')
-               dst++;
-       dlen = dst - odst;
-       n = dsize - dlen;
-
-       if (n-- == 0)
-               return(dlen + strlen(src));
-       while (*src != '\0') {
-               if (n != 0) {
-                       *dst++ = *src;
-                       n--;
-               }
-               src++;
+       if (ssize >= dsize)
+       {
+               ssize = dsize - 1;
+               Con_Printf(CON_WARN "%s: src string truncated to %zu bytes: \"%.*s\"\n", __func__, ssize, (int)ssize, src);
        }
-       *dst = '\0';
-
-       return(dlen + (src - osrc));    /* count does not include NUL */
+       memcpy(dst, src, ssize);
+       dst[ssize] = '\0';
+       return &dst[ssize];
 }
 
-size_t dp_strlcpy(char *dst, const char *src, size_t dsize)
+/** Copies a string, like strlcpy() but with a better return: the number of bytes copied
+ * excluding the \0 terminator. Truncates and warns on overflow or unterminated source,
+ * whereas strlcpy() truncates silently and overreads (possibly segfaulting).
+ * Guarantees \0 termination.
+ * See also: dp_stpecpy() and dp_ustr2stp().
+ */
+size_t dp__strlcpy(char *dst, const char *src, size_t dsize, const char *func, unsigned line)
 {
-       const char *osrc = src;
-       size_t nleft = dsize;
+       char *p = (char *)memccpy(dst, src, '\0', dsize);
 
-       /* Copy as many bytes as will fit. */
-       if (nleft != 0) {
-               while (--nleft != 0) {
-                       if ((*dst++ = *src++) == '\0')
-                               break;
-               }
-       }
+       if (p)
+               return (p - 1) - dst;
+       dst[dsize - 1] = '\0';
+       Con_Printf(CON_WARN "%s:%u: src string unterminated or truncated to %zu bytes: \"%s\"\n", func, line, dsize - 1, dst);
+       return dsize - 1;
+}
 
-       /* Not enough room in dst, add NUL and traverse rest of src. */
-       if (nleft == 0) {
-               if (dsize != 0)
-                       *dst = '\0';            /* NUL-terminate dst */
-               while (*src++)
-                       ;
-       }
+/** Catenates a string, like strlcat() but with a better return: the number of bytes copied
+ * excluding the \0 terminator. Truncates and warns on overflow or unterminated source,
+ * whereas strlcat() truncates silently and overreads (possibly segfaulting).
+ * Guarantees \0 termination.
+ * Inefficient like any strcat(), please use memcpy(), dp_stpecpy() or dp_strlcpy() instead.
+ */
+size_t dp__strlcat(char *dst, const char *src, size_t dsize, const char *func, unsigned line)
+{
+       char *p = (char *)memchr(dst, '\0', dsize) ?: dst;
+       size_t offset = p - dst;
 
-       return(src - osrc - 1); /* count does not include NUL */
+       return dp__strlcpy(p, src, dsize - offset, func, line) + offset;
 }
 
 
+
 void FindFraction(double val, int *num, int *denom, int denomMax)
 {
        int i;

diff --git a/common.h b/common.h
index 677219d621fc8e3579d5016d8255f5d295822325..ff088a790546d420c6205236329cfcfec2ef7873 100644 (file)
--- a/common.h
+++ b/common.h
@@ -225,13 +225,9 @@ char *va(char *buf, size_t buflen, const char *format, ...) DP_FUNC_PRINTF(3);
 #endif
 
 // snprintf and vsnprintf are NOT portable. Use their DP counterparts instead
-#ifdef snprintf
-# undef snprintf
-#endif
+#undef snprintf
 #define snprintf DP_STATIC_ASSERT(0, "snprintf is forbidden for portability reasons. Use dpsnprintf instead.")
-#ifdef vsnprintf
-# undef vsnprintf
-#endif
+#undef vsnprintf
 #define vsnprintf DP_STATIC_ASSERT(0, "vsnprintf is forbidden for portability reasons. Use dpvsnprintf instead.")
 
 // dpsnprintf and dpvsnprintf
@@ -244,16 +240,27 @@ extern int dpvsnprintf (char *buffer, size_t buffersize, const char *format, va_
 // A bunch of functions are forbidden for security reasons (and also to please MSVS 2005, for some of them)
 // LadyHavoc: added #undef lines here to avoid warnings in Linux
 #undef strcat
-#define strcat DP_STATIC_ASSERT(0, "strcat is forbidden for security reasons. Use strlcat or memcpy instead.")
+#define strcat DP_STATIC_ASSERT(0, "strcat is forbidden for security reasons. Use dp_strlcat or memcpy instead.")
 #undef strncat
-#define strncat DP_STATIC_ASSERT(0, "strncat is forbidden for security reasons. Use strlcat or memcpy instead.")
+#define strncat DP_STATIC_ASSERT(0, "strncat is forbidden for security reasons. Use dp_strlcat or memcpy instead.")
 #undef strcpy
-#define strcpy DP_STATIC_ASSERT(0, "strcpy is forbidden for security reasons. Use strlcpy or memcpy instead.")
+#define strcpy DP_STATIC_ASSERT(0, "strcpy is forbidden for security reasons. Use dp_strlcpy or memcpy instead.")
 #undef strncpy
-#define strncpy DP_STATIC_ASSERT(0, "strncpy is forbidden for security reasons. Use strlcpy or memcpy instead.")
+#define strncpy DP_STATIC_ASSERT(0, "strncpy is forbidden for security reasons. Use dp_strlcpy or memcpy instead.")
+#undef stpcpy
+#define stpcpy DP_STATIC_ASSERT(0, "stpcpy is forbidden for security reasons. Use dp_stpecpy or memcpy instead.")
+#undef ustpcpy
+#define ustpcpy DP_STATIC_ASSERT(0, "ustpcpy is forbidden for security reasons. Use dp_ustr2stp or memcpy instead.")
+#undef ustr2stp
+#define ustr2stp DP_STATIC_ASSERT(0, "ustr2stp is forbidden for security reasons. Use dp_ustr2stp or memcpy instead.")
 #undef sprintf
 #define sprintf DP_STATIC_ASSERT(0, "sprintf is forbidden for security reasons. Use dpsnprintf instead.")
 
+#undef strlcpy
+#define strlcpy DP_STATIC_ASSERT(0, "strlcpy is forbidden for stability and correctness. See common.h and common.c comments.")
+#undef strlcat
+#define strlcat DP_STATIC_ASSERT(0, "strlcat is forbidden for stability and correctness. See common.h and common.c comments.")
+
 
 //============================================================================
 
@@ -281,21 +288,14 @@ qbool COM_StringDecolorize(const char *in, size_t size_in, char *out, size_t siz
 void COM_ToLowerString (const char *in, char *out, size_t size_out);
 void COM_ToUpperString (const char *in, char *out, size_t size_out);
 
-/*!
- * Appends src to string dst of size dsize (unlike strncat, dsize is the
- * full size of dst, not space left).  At most dsize-1 characters
- * will be copied.  Always NUL terminates (unless dsize <= strlen(dst)).
- * Returns strlen(src) + MIN(dsize, strlen(initial dst)).
- * If retval >= dsize, truncation occurred.
- */
-size_t dp_strlcat(char *dst, const char *src, size_t dsize);
 
-/*!
- * Copy string src to buffer dst of size dsize.  At most dsize-1
- * chars will be copied.  Always NUL terminates (unless dsize == 0).
- * Returns strlen(src); if retval >= dsize, truncation occurred.
- */
-size_t dp_strlcpy(char *dst, const char *src, size_t dsize);
+#define dp_strlcpy(dst, src, dsize) dp__strlcpy(dst, src, dsize, __func__, __LINE__)
+#define dp_strlcat(dst, src, dsize) dp__strlcat(dst, src, dsize, __func__, __LINE__)
+size_t dp__strlcpy(char *dst, const char *src, size_t dsize, const char *func, unsigned line);
+size_t dp__strlcat(char *dst, const char *src, size_t dsize, const char *func, unsigned line);
+char *dp_stpecpy(char *dst, char *end, const char *src);
+char *dp_ustr2stp(char *dst, size_t dsize, const char *src, size_t ssize);
+
 
 void FindFraction(double val, int *num, int *denom, int denomMax);
 

diff --git a/prvm_exec.c b/prvm_exec.c
index e3ad8a8283295907ee0b684eadae80ee268f9ff6..ee738b9c3cbc0254d27ba7b199bae9676b1df96f 100644 (file)
--- a/prvm_exec.c
+++ b/prvm_exec.c
@@ -434,10 +434,11 @@ void PRVM_ShortStackTrace(prvm_prog_t *prog, char *buf, size_t bufsize)
        mfunction_t     *f;
        int                     i;
        char vabuf[1024];
+       char *p;
 
        if(prog)
        {
-               dpsnprintf(buf, bufsize, "(%s) ", prog->name);
+               p = buf + max(0, dpsnprintf(buf, bufsize, "(%s) ", prog->name));
        }
        else
        {
@@ -450,13 +451,14 @@ void PRVM_ShortStackTrace(prvm_prog_t *prog, char *buf, size_t bufsize)
        for (i = prog->depth;i > 0;i--)
        {
                f = prog->stack[i].f;
-
-               if(dp_strlcat(buf,
+               p = dp_stpecpy(
+                       p,
+                       buf + bufsize,
                        f
                                ? va(vabuf, sizeof(vabuf), "%s:%s(%i) ", PRVM_GetString(prog, f->s_file), PRVM_GetString(prog, f->s_name), prog->stack[i].s - f->first_statement)
-                               : "<NULL> ",
-                       bufsize
-               ) >= bufsize)
+                               : "<NULL> "
+                       );
+               if (p == buf + bufsize)
                        break;
        }
 }