[PATCH v2] commit.c: ensure find_header_mem() doesn't scan beyond given range

From: Chandra Pratap via GitGitGadget <gitgitgadget@gmail.com>
Date: Feb 7, 2024, 13:57 UTC
Message-ID: <pull.1652.v2.git.1707314246530.gitgitgadget@gmail.com>
In-Reply-To: <pull.1652.git.1707153705840.gitgitgadget@gmail.com>

From: Chandra Pratap <chandrapratap3519@gmail.com>

Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
    commit.c: ensure find_header_mem() doesn't scan beyond given range
    
    Thanks for the feedback, Kyle and René! I have update the patch to
    actually solve the problem at hand but I am not very sure about the
    resulting dropping of const-ness of 'eol' from this and how big of a
    problem it might create (if any). I wonder if a custom strchrnul() is
    the best solution to this after all.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1652%2FChand-ra%2Fstrchrnul-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1652/Chand-ra/strchrnul-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1652

Range-diff vs v1:

 1:  1c62f6ee353 ! 1:  dcb2de3faea commit.c: ensure strchrnul() doesn't scan beyond range
     @@ Metadata
      Author: Chandra Pratap <chandrapratap3519@gmail.com>
      
       ## Commit message ##
     -    commit.c: ensure strchrnul() doesn't scan beyond range
     +    commit.c: ensure find_header_mem() doesn't scan beyond given range
      
          Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
      
     @@ commit.c: const char *find_header_mem(const char *msg, size_t len,
      -	 * at msg beyond the len provided by the caller.
      -	 */
       	while (line && line < msg + len) {
     - 		const char *eol = strchrnul(line, '\n');
     -+		assert(eol - line <= len);
     +-		const char *eol = strchrnul(line, '\n');
     ++		char *eol = (char *) line;
     ++		for (size_t i = 0; i < len && *eol && *eol != '\n'; i++) {
     ++			eol++;
     ++		}
       
       		if (line == eol)
       			return NULL;

 commit.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/commit.c b/commit.c
index ef679a0b939..9a460b2fd6f 100644
--- a/commit.c
+++ b/commit.c
@@ -1743,15 +1743,11 @@ const char *find_header_mem(const char *msg, size_t len,
 	int key_len = strlen(key);
 	const char *line = msg;
 
-	/*
-	 * NEEDSWORK: It's possible for strchrnul() to scan beyond the range
-	 * given by len. However, current callers are safe because they compute
-	 * len by scanning a NUL-terminated block of memory starting at msg.
-	 * Nonetheless, it would be better to ensure the function does not look
-	 * at msg beyond the len provided by the caller.
-	 */
 	while (line && line < msg + len) {
-		const char *eol = strchrnul(line, '\n');
+		char *eol = (char *) line;
+		for (size_t i = 0; i < len && *eol && *eol != '\n'; i++) {
+			eol++;
+		}
 
 		if (line == eol)
 			return NULL;

base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b

-- 
gitgitgadget