]> git.xonotic.org Git - xonotic/darkplaces.git/commitdiff
Fix uses of strlcpy on unterminated source strings
authorbones_was_here <bones_was_here@xonotic.au>
Mon, 18 Dec 2023 09:24:15 +0000 (19:24 +1000)
committerbones_was_here <bones_was_here@xonotic.au>
Sun, 21 Jan 2024 07:00:44 +0000 (17:00 +1000)
BSD strlcpy() keeps reading until a \0 or a segfault, even after it
stops writing. In these cases the source was part of a longer string so
it wouldn't have crashed.
These were caught by the new warning.

The +1 on the source sizes copied an extra char that would be changed to
\0 and is now redundant because the real dest and src sizes are always
passed to a purpose-written func.

Signed-off-by: bones_was_here <bones_was_here@xonotic.au>
cmd.c
cmd.h
console.c
fs.c

diff --git a/cmd.c b/cmd.c
index 8c6570ecdf3447017e2549ae23c880610e98975d..667b9009b0d24f858574311f0e84a03a94ca1615 100644 (file)
--- a/cmd.c
+++ b/cmd.c
@@ -169,7 +169,8 @@ static cmd_input_t *Cbuf_NodeGet(cmd_buf_t *cbuf, cmd_input_t *existing)
 ============
 Cbuf_LinkString
 
-Copies a command string into a buffer node
+Copies a command string into a buffer node.
+The input should not be null-terminated, the output will be.
 ============
 */
 static void Cbuf_LinkString(cmd_state_t *cmd, llist_t *head, cmd_input_t *existing, const char *text, qbool leavepending, unsigned int cmdsize)
@@ -193,7 +194,7 @@ static void Cbuf_LinkString(cmd_state_t *cmd, llist_t *head, cmd_input_t *existi
        }
        cbuf->size += cmdsize;
 
-       dp_strlcpy(&node->text[offset], text, cmdsize + 1); // always sets the last char to \0
+       dp_ustr2stp(&node->text[offset], node->length + 1, text, cmdsize);
        //Con_Printf("^5Cbuf_LinkString(): %s `^7%s^5`\n", node->pending ? "append" : "new", &node->text[offset]);
        node->pending = leavepending;
 }
diff --git a/cmd.h b/cmd.h
index 30b3abd05d732ce6d05b75d4d85f3c94a17e66eb..614aa0a094b725012e42037ba7ae3134f51144e3 100644 (file)
--- a/cmd.h
+++ b/cmd.h
@@ -156,8 +156,8 @@ typedef struct cmd_input_s
        llist_t list;
        cmd_state_t *source;
        vec_t delay;
-       size_t size;
-       size_t length;
+       size_t size;   ///< excludes \0 terminator
+       size_t length; ///< excludes \0 terminator
        char *text;
        qbool pending;
 } cmd_input_t;
index 7767617cfd7f2f8335a50511a51910a040420170..eefcb25e975b8c0a8f4de738ba07199c59dc6cde 100644 (file)
--- a/console.c
+++ b/console.c
@@ -391,8 +391,8 @@ const char *ConBuffer_GetLine(conbuffer_t *buf, int i)
 {
        static char copybuf[MAX_INPUTLINE]; // client only
        con_lineinfo_t *l = &CONBUFFER_LINES(buf, i);
-       size_t sz = l->len+1 > sizeof(copybuf) ? sizeof(copybuf) : l->len+1;
-       dp_strlcpy(copybuf, l->start, sz);
+
+       dp_ustr2stp(copybuf, sizeof(copybuf), l->start, l->len);
        return copybuf;
 }
 
@@ -2785,7 +2785,8 @@ int Con_CompleteCommandLine(cmd_state_t *cmd, qbool is_console)
        space = strchr(line + 1, ' ');
        if(space && pos == (space - line) + 1)
        {
-               dp_strlcpy(command, line + 1, min(sizeof(command), (unsigned int)(space - line)));
+               // adding 1 to line drops the leading ]
+               dp_ustr2stp(command, sizeof(command), line + 1, space - (line + 1));
 
                patterns = Cvar_VariableString(cmd->cvars, va(vabuf, sizeof(vabuf), "con_completion_%s", command), CF_CLIENT | CF_SERVER); // TODO maybe use a better place for this?
                if(patterns && !*patterns)
diff --git a/fs.c b/fs.c
index 9469956ae88b4ad10d39872fe42b122f2c931bac..c76a1467404af29dfba00a7c51fd962b57f14957 100644 (file)
--- a/fs.c
+++ b/fs.c
@@ -3846,7 +3846,7 @@ fssearch_t *FS_Search(const char *pattern, int caseinsensitive, int quiet, const
 
                                // prevseparator points past the '/' right before the wildcard and nextseparator at the one following it (or at the end of the string)
                                // copy everything up except nextseperator
-                               dp_strlcpy(subpattern, pattern, min(sizeof(subpattern), (size_t) (nextseparator - pattern + 1)));
+                               dp_ustr2stp(subpattern, sizeof(subpattern), pattern, nextseparator - pattern);
                                // find the last '/' before the wildcard
                                prevseparator = strrchr( subpattern, '/' );
                                if (!prevseparator)
@@ -3855,7 +3855,7 @@ fssearch_t *FS_Search(const char *pattern, int caseinsensitive, int quiet, const
                                        prevseparator++;
                                // copy everything from start to the previous including the '/' (before the wildcard)
                                // everything up to start is already included in the path of matchedSet's entries
-                               dp_strlcpy(subpath, start, min(sizeof(subpath), (size_t) ((prevseparator - subpattern) - (start - pattern) + 1)));
+                               dp_ustr2stp(subpath, sizeof(subpath), start, (prevseparator - subpattern) - (start - pattern));
 
                                // for each entry in matchedSet try to open the subdirectories specified in subpath
                                for( dirlistindex = 0 ; dirlistindex < matchedSet.numstrings ; dirlistindex++ ) {