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

don't accept bogus N in `HEAD~N'

From
Jim Meyering <jim@meyering.net>
Date
May 21, 2006, 21:35 UTC
Message-ID
<87d5e7hxhl.fsf_-_@rho.meyering.net>
In-Reply-To
<7v3bf3jl15.fsf@assigned-by-dhcp.cox.net>

In a very shallow audit, I spotted code where overflow was not detected. But it's hardly critical.

Currently,
  git-diff HEAD HEAD
is equivalent to this
  git-diff HEAD HEAD~18446744073709551616   # aka 2^64
Exercising git-rev-parse directly, currently I get this:
  $ git-rev-parse --no-flags --sq HEAD~18446744073709551616
  '639ca5497279607665847f2e3a11064441a8f2a6'
It'd be better to produce a diagnostic and fail:
  $ ./git-rev-parse --no-flags --sq -- HEAD~18446744073709551616 > /dev/null
  fatal: ambiguous argument 'HEAD~18446744073709551616': unknown revision or filename
The code in question is in sha1_name.c (get_sha1_1):
               int num = 0;
               ...
               while (cp < name + len)
                       num = num * 10 + *cp++ - '0';

Looking at how to fix it, my first reflex was to replace that loop with this one:

		while (cp < name + len) {
			int tmp = num * 10 + *cp++ - '0';
			if (INT_MAX / 10 < num || tmp < num)
				return -1;
			num = tmp;
		}

But INT_MAX is used nowhere else, so I wonder if git avoids using it for some reason. At least `make check' gripes about __INT_MAX__. Anyhow, here's the patch I used. With it, git still passes `make test'.

diff --git a/sha1_name.c b/sha1_name.c
index dc68355..c813ba0 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -429,8 +429,12 @@ static int get_sha1_1(const char *name,
 		int num = 0;
 		int len1 = cp - name;
 		cp++;
-		while (cp < name + len)
-			num = num * 10 + *cp++ - '0';
+		while (cp < name + len) {
+			int tmp = num * 10 + *cp++ - '0';
+			if (INT_MAX / 10 < num || tmp < num)
+				return -1;
+			num = tmp;
+		}
 		if (has_suffix == '^') {
 			if (!num && len1 == len - 1)
 				num = 1;
Previous: Jim MeyeringNext: Jakub Narebski
Message 10 of 20 in “synchronizing incremental git changes to cvs”
  1. Jim MeyeringMay 20, 2006
  2. Johannes SchindelinMay 20, 2006
  3. Jim MeyeringMay 21, 2006
  4. Pavel RoskinMay 22, 2006
  5. Jakub NarebskiMay 22, 2006
  6. Martin LanghoffMay 21, 2006
  7. Jim MeyeringMay 21, 2006
  8. Junio C HamanoMay 21, 2006
  9. Jim MeyeringMay 21, 2006
  10. don't accept bogus N in `HEAD~N'Jim Meyering, May 21, 2006
  11. Jakub NarebskiMay 21, 2006
  12. Jim MeyeringMay 22, 2006
  13. Junio C HamanoMay 22, 2006
  14. Junio C HamanoMay 22, 2006
  15. avoid atoi, when possible; int overflow -> heap corruptionJim Meyering, May 22, 2006
  16. Morten WelinderMay 22, 2006
  17. Jim MeyeringMay 22, 2006
  18. Jeff KingMay 22, 2006
  19. Morten WelinderMay 22, 2006
  20. detect write failure, even for stdoutJim Meyering, May 22, 2006

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.