From: Stefan Sperling <stsp@stsp.name>
Subject: Re: use size_t for loop indices to avoid signedness warnings
To: "Todd C. Miller" <millert@openbsd.org>
Cc: Ed Maste <emaste@freebsd.org>, gameoftrees@openbsd.org
Date: Sat, 19 Dec 2020 12:57:20 +0100

On Fri, Dec 18, 2020 at 09:06:59AM -0700, Todd C. Miller wrote:
> On Fri, 18 Dec 2020 17:04:10 +0100, Stefan Sperling wrote:
> 
> > Macro question aside, do I understand correctly that casting sizeof(x) to
> > ssize_t is preferred over casting to an int?
> >
> > Where my patch uses (int)sizeof(msg) to compare to an int, it should do
> > (ssize_t)sizeof(msg) instead?
> >
> > I guess we'd otherwise risk truncation when the result of sizeof(x) exceeds
> > the size of an int? Or is there another problem?
> 
> I generally cast to the signed equivalent of the type in question
> to avoid truncation.  Realistically, this is a non-issue with
> sizeof(foo) since you are not going to have data types with a size
> larger than INT_MAX.  So I don't think it really matters in practice.
> 
>  - todd

After revisiting C integer promotion rules and investigating some more of
these warnings, I believe I will have to stretch this work out over time
in small steps. My initial plan was to produce one patch that would fix
all the problems and flip on -Wsign-compare, but that results in chaos :-/

I have found one case where the ssizeof() macro you suggested helps.
Note that packfile_size is already an off_t in this file (elsewhere it
is size_t but I'm trying to fix that as you may have seen in another
discussion thread).

ok?

diff 91dc86ae797191eff2211c2555318120b1dcbbff 0aab76f5fc3a5dc2a5b75368480b9f3e9e512f59
blob - 91dc86ae797191eff2211c2555318120b1dcbbff
blob + 0aab76f5fc3a5dc2a5b75368480b9f3e9e512f59
--- lib/fetch.c
+++ lib/fetch.c
@@ -67,6 +67,10 @@
 #define nitems(_a)	(sizeof((_a)) / sizeof((_a)[0]))
 #endif
 
+#ifndef ssizeof
+#define ssizeof(_x) ((ssize_t)(sizeof(_x)))
+#endif
+
 #ifndef MIN
 #define	MIN(_a,_b) ((_a) < (_b) ? (_a) : (_b))
 #endif
@@ -129,7 +133,7 @@ dial_ssh(pid_t *fetchpid, int *fetchfd, const char *ho
 		dup2(pfd[0], 0);
 		dup2(pfd[0], 1);
 		n = snprintf(cmd, sizeof(cmd), "git-%s-pack", direction);
-		if (n < 0 || n >= sizeof(cmd))
+		if (n < 0 || n >= ssizeof(cmd))
 			err(1, "snprintf");
 		if (execv(GOT_FETCH_PATH_SSH, argv) == -1)
 			err(1, "execl");
@@ -669,18 +673,18 @@ got_fetch_pack(struct got_object_id **pack_hash, struc
 		free(*pack_hash);
 		*pack_hash = NULL;
 		goto done;
-	} else if (packfile_size < sizeof(pack_hdr) + SHA1_DIGEST_LENGTH) {
+	} else if (packfile_size < ssizeof(pack_hdr) + SHA1_DIGEST_LENGTH) {
 		err = got_error_msg(GOT_ERR_BAD_PACKFILE, "short pack file");
 		goto done;
 	} else {
 		ssize_t n;
 
-		n = read(packfd, &pack_hdr, sizeof(pack_hdr));
+		n = read(packfd, &pack_hdr, ssizeof(pack_hdr));
 		if (n == -1) {
 			err = got_error_from_errno("read");
 			goto done;
 		}
-		if (n != sizeof(pack_hdr)) {
+		if (n != ssizeof(pack_hdr)) {
 			err = got_error(GOT_ERR_IO);
 			goto done;
 		}
@@ -696,11 +700,11 @@ got_fetch_pack(struct got_object_id **pack_hash, struc
 		}
 		nobj = be32toh(pack_hdr.nobjects);
 		if (nobj == 0 &&
-		    packfile_size > sizeof(pack_hdr) + SHA1_DIGEST_LENGTH)
+		    packfile_size > ssizeof(pack_hdr) + SHA1_DIGEST_LENGTH)
 			return got_error_msg(GOT_ERR_BAD_PACKFILE,
 			    "bad pack file with zero objects");
 		if (nobj != 0 &&
-		    packfile_size <= sizeof(pack_hdr) + SHA1_DIGEST_LENGTH)
+		    packfile_size <= ssizeof(pack_hdr) + SHA1_DIGEST_LENGTH)
 			return got_error_msg(GOT_ERR_BAD_PACKFILE,
 			    "empty pack file with non-zero object count");
 	}