Re: [PATCH v2] config: avoid "write_in_full(fd, buf, len) < len" pattern

[Jonathan Nieder <jrnieder@gmail.com>]

Jeff King wrote:

> AFAIK there's no way to turn it on for specific functions, but I'm far
> from a gcc-warning guru. Even if you could, though, the error may be far
> from the function (e.g., if we store the result in an ssize_t and then
> compare that with a size_t).

It turns out that yes, we have two of those.  Both handle errors
separately already, so they should be safe.

While investigating the second, I noticed that read_in_full can
overflow its return value.  malloc doesn't typically allow allocating
a buffer with size greater than SSIZE_MAX so this should be safe, but
it would be confusing to static analyzers.

Combining these observations yields the following (just for
illustration):

diff --git i/bulk-checkin.c w/bulk-checkin.c
index 9a1f6c49ab..f8e3060041 100644
--- i/bulk-checkin.c
+++ w/bulk-checkin.c
@@ -114,7 +114,7 @@ static int stream_to_pack(struct bulk_checkin_state *state,
 		unsigned char ibuf[16384];
 
 		if (size && !s.avail_in) {
-			ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
+			size_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
 			if (read_in_full(fd, ibuf, rsize) != rsize)
 				die("failed to read %d bytes from '%s'",
 				    (int)rsize, path);
diff --git i/combine-diff.c w/combine-diff.c
index 9e163d5ada..e966d4f393 100644
--- i/combine-diff.c
+++ w/combine-diff.c
@@ -1026,7 +1026,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			result_size = fill_textconv(textconv, df, &result);
 			free_filespec(df);
 		} else if (0 <= (fd = open(elem->path, O_RDONLY))) {
-			size_t len = xsize_t(st.st_size);
+			ssize_t len = xssize_t(st.st_size);
 			ssize_t done;
 			int is_file, i;
 
@@ -1040,6 +1040,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			if (!is_file)
 				elem->mode = canon_mode(S_IFLNK);
 
+			if (len > ULONG_MAX)
+				die("cannot handle files this big");
 			result_size = len;
 			result = xmallocz(len);
 
diff --git i/git-compat-util.h w/git-compat-util.h
index 6678b488cc..20fea81589 100644
--- i/git-compat-util.h
+++ w/git-compat-util.h
@@ -903,6 +903,13 @@ static inline size_t xsize_t(off_t len)
 	return (size_t)len;
 }
 
+static inline ssize_t xssize_t(off_t len)
+{
+	if (len > SSIZE_MAX)
+		die("cannot handle files this big");
+	return (ssize_t)len;
+}
+
 __attribute__((format (printf, 3, 4)))
 extern int xsnprintf(char *dst, size_t max, const char *fmt, ...);
 
diff --git i/wrapper.c w/wrapper.c
index 36630e5d18..2b52b7367d 100644
--- i/wrapper.c
+++ w/wrapper.c
@@ -314,6 +314,9 @@ ssize_t read_in_full(int fd, void *buf, size_t count)
 	char *p = buf;
 	ssize_t total = 0;
 
+	if (count > SSIZE_MAX)
+		BUG("read_in_full called with absurdly high count %"PRIuMAX,
+		    (uintmax_t) count);
 	while (count > 0) {
 		ssize_t loaded = xread(fd, p, count);
 		if (loaded < 0)