git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 08/18] avoid using mksnpath for refs

From
Jeff King <peff@peff.net>
Date
Mar 28, 2017, 19:46 UTC
Message-ID
<20170328194633.ok3tamreao5u7gjp@sigill.intra.peff.net>
In-Reply-To
<20170328194255.vf7nfzzmmzxsbn36@sigill.intra.peff.net>

Like the previous commit, we'd like to avoid the assumption that refs fit into PATH_MAX-sized buffers. These callsites have an extra twist, though: they write the refnames using mksnpath. This does two things beyond a regular snprintf:

  1. It quietly writes "/bad-path/" when truncation occurs.
     This saves the caller having to check the error code,
     but if you aren't actually feeding the result to a
     system call (and we aren't here), it's questionable.
  2. It calls cleanup_path(), which removes leading
     instances of "./".  That's questionable when dealing
     with refnames, as we could silently canonicalize a
     syntactically bogus refname into a valid one.

Let's convert each case to use a strbuf. This is preferable to xstrfmt() because we can reuse the same buffer as we loop.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)
diff --git a/refs.c b/refs.c
index e7606716d..9de6979ac 100644
--- a/refs.c
+++ b/refs.c
@@ -429,29 +429,31 @@ int expand_ref(const char *str, int len, unsigned char *sha1, char **ref)
 {
 	const char **p, *r;
 	int refs_found = 0;
+	struct strbuf fullref = STRBUF_INIT;
 
 	*ref = NULL;
 	for (p = ref_rev_parse_rules; *p; p++) {
-		char fullref[PATH_MAX];
 		unsigned char sha1_from_ref[20];
 		unsigned char *this_result;
 		int flag;
 
 		this_result = refs_found ? sha1_from_ref : sha1;
-		mksnpath(fullref, sizeof(fullref), *p, len, str);
-		r = resolve_ref_unsafe(fullref, RESOLVE_REF_READING,
+		strbuf_reset(&fullref);
+		strbuf_addf(&fullref, *p, len, str);
+		r = resolve_ref_unsafe(fullref.buf, RESOLVE_REF_READING,
 				       this_result, &flag);
 		if (r) {
 			if (!refs_found++)
 				*ref = xstrdup(r);
 			if (!warn_ambiguous_refs)
 				break;
-		} else if ((flag & REF_ISSYMREF) && strcmp(fullref, "HEAD")) {
-			warning("ignoring dangling symref %s.", fullref);
-		} else if ((flag & REF_ISBROKEN) && strchr(fullref, '/')) {
-			warning("ignoring broken ref %s.", fullref);
+		} else if ((flag & REF_ISSYMREF) && strcmp(fullref.buf, "HEAD")) {
+			warning("ignoring dangling symref %s.", fullref.buf);
+		} else if ((flag & REF_ISBROKEN) && strchr(fullref.buf, '/')) {
+			warning("ignoring broken ref %s.", fullref.buf);
 		}
 	}
+	strbuf_release(&fullref);
 	return refs_found;
 }
 
@@ -460,21 +462,22 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 	char *last_branch = substitute_branch_name(&str, &len);
 	const char **p;
 	int logs_found = 0;
+	struct strbuf path = STRBUF_INIT;
 
 	*log = NULL;
 	for (p = ref_rev_parse_rules; *p; p++) {
 		unsigned char hash[20];
-		char path[PATH_MAX];
 		const char *ref, *it;
 
-		mksnpath(path, sizeof(path), *p, len, str);
-		ref = resolve_ref_unsafe(path, RESOLVE_REF_READING,
+		strbuf_reset(&path);
+		strbuf_addf(&path, *p, len, str);
+		ref = resolve_ref_unsafe(path.buf, RESOLVE_REF_READING,
 					 hash, NULL);
 		if (!ref)
 			continue;
-		if (reflog_exists(path))
-			it = path;
-		else if (strcmp(ref, path) && reflog_exists(ref))
+		if (reflog_exists(path.buf))
+			it = path.buf;
+		else if (strcmp(ref, path.buf) && reflog_exists(ref))
 			it = ref;
 		else
 			continue;
@@ -485,6 +488,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 		if (!warn_ambiguous_refs)
 			break;
 	}
+	strbuf_release(&path);
 	free(last_branch);
 	return logs_found;
 }
@@ -944,6 +948,7 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 	static char **scanf_fmts;
 	static int nr_rules;
 	char *short_name;
+	struct strbuf resolved_buf = STRBUF_INIT;
 
 	if (!nr_rules) {
 		/*
@@ -1002,7 +1007,6 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 		 */
 		for (j = 0; j < rules_to_fail; j++) {
 			const char *rule = ref_rev_parse_rules[j];
-			char refname[PATH_MAX];
 
 			/* skip matched rule */
 			if (i == j)
@@ -1013,9 +1017,10 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 			 * (with this previous rule) to a valid ref
 			 * read_ref() returns 0 on success
 			 */
-			mksnpath(refname, sizeof(refname),
-				 rule, short_name_len, short_name);
-			if (ref_exists(refname))
+			strbuf_reset(&resolved_buf);
+			strbuf_addf(&resolved_buf, rule,
+				    short_name_len, short_name);
+			if (ref_exists(resolved_buf.buf))
 				break;
 		}
 
@@ -1023,10 +1028,13 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 		 * short name is non-ambiguous if all previous rules
 		 * haven't resolved to a valid ref
 		 */
-		if (j == rules_to_fail)
+		if (j == rules_to_fail) {
+			strbuf_release(&resolved_buf);
 			return short_name;
+		}
 	}
 
+	strbuf_release(&resolved_buf);
 	free(short_name);
 	return xstrdup(refname);
 }
-- 
2.12.2.845.g55fcf8b10
Previous: Jeff KingNext: Jeff King
Message 7 of 30 in “snprintf cleanups”
  1. 0/18 snprintf cleanupsJeff King, Mar 28, 2017
  2. 01/18 do not check odb_mkstemp return value for errorsJeff King, Mar 28, 2017
  3. 02/18 odb_mkstemp: write filename into strbufJeff King, Mar 28, 2017
  4. 03/18 odb_mkstemp: use git_path_bufJeff King, Mar 28, 2017
  5. 05/18 tag: use strbuf to format tag headerJeff King, Mar 28, 2017
  6. 09/18 create_branch: move msg setup closer to point of useJeff King, Mar 28, 2017
  7. 08/18 avoid using mksnpath for refsJeff King, Mar 28, 2017
  8. 06/18 fetch: use heap buffer to format reflogJeff King, Mar 28, 2017
  9. 04/18 diff: avoid fixed-size buffer for patch-idsJeff King, Mar 28, 2017
  10. Jeff KingMar 28, 2017
  11. 13/18 replace unchecked snprintf calls with heap buffersJeff King, Mar 28, 2017
  12. 12/18 receive-pack: print --pack-header directly into argv arrayJeff King, Mar 28, 2017
  13. 07/18 avoid using fixed PATH_MAX buffers for refsJeff King, Mar 28, 2017
  14. Junio C HamanoApr 17, 2017
  15. Jeff KingApr 18, 2017
  16. Junio C HamanoApr 18, 2017
  17. 11/18 name-rev: replace static buffer with strbufJeff King, Mar 28, 2017
  18. 15/18 convert unchecked snprintf into xsnprintfJeff King, Mar 28, 2017
  19. 10/18 create_branch: use xstrfmt for reflog messageJeff King, Mar 28, 2017
  20. 14/18 combine-diff: replace malloc/snprintf with xstrfmtJeff King, Mar 28, 2017
  21. 17/18 gc: replace local buffer with git_pathJeff King, Mar 28, 2017
  22. 16/18 transport-helper: replace checked snprintf with xsnprintfJeff King, Mar 28, 2017
  23. 18/18 daemon: use an argv_array to exec childrenJeff King, Mar 28, 2017
  24. Junio C HamanoMar 28, 2017
  25. Jeff KingMar 29, 2017
  26. Junio C HamanoMar 29, 2017
  27. Jeff KingMar 30, 2017
  28. Junio C HamanoMar 30, 2017
  29. Jeff KingMar 30, 2017
  30. Makefile: detect errors in running spatchJeff King, Mar 29, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.