On 2022/08/10 14:54:16 +0200, Theo Buehler <tb@theobuehler.org> wrote:
> > I've also spotted some unneeded checks for snprintf.  The correct
> > check would be `ret < 0', but since snprintf can only fail due to an
> > encoding error, I think we can just check for the partial write.
> 
> I'd rather you changed those into ret < 0 to match what the CAVEAT in
> the snprintf(3) manual says - it used to say ret == -1 but that was
> changed a few years back because standards decided to allow negative
> returns instead of only -1.

oh, I've forgot about that part of CAVEAT.  Updated diff below.

I've only left

    (got-read-pack.c)
    553         n = snprintf(buf, sizeof(buf), "done\n");
    554         err = got_pkt_writepkt(fd, buf, n, chattygot);

that may look suspicious but I felt bad adding a check there: we're
just writing six bytes after all.

 ---

Just for curiosity, can

	snprintf(msg, sizeof(msg), "syntax error on line %d", lineno);

really fail or is more like an hypotethical thing allowed by the
standard?  (I'm not talking about overflowing `msg' but returning a
negative value.)

Thanks!

diff /home/op/w/got
commit - bf80b15220f51490025e916633cdd70816113604
path + /home/op/w/got
blob - 39827e91edca9da87bdc0ce98c9a77260b22dd4e
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -10611,7 +10611,7 @@ histedit_syntax_error(int lineno)
 
 	ret = snprintf(msg, sizeof(msg), "histedit syntax error on line %d",
 	    lineno);
-	if (ret == -1 || ret >= sizeof(msg))
+	if (ret < 0 || (size_t)ret >= sizeof(msg))
 		return got_error(GOT_ERR_HISTEDIT_SYNTAX);
 
 	return got_error_msg(GOT_ERR_HISTEDIT_SYNTAX, msg);
blob - ecce1768fa5b41babe7ad5226f47933e94aeaf64
file + gotwebd/parse.y
--- gotwebd/parse.y
+++ gotwebd/parse.y
@@ -202,7 +202,8 @@ main		: PREFORK NUMBER {
 			    sizeof(gotwebd->unix_socket_name), "%s%s",
 			    strlen(gotwebd->httpd_chroot) ?
 			    gotwebd->httpd_chroot : D_HTTPD_CHROOT, $2);
-			if (n < 0) {
+			if (n < 0 ||
+			    (size_t)n >= sizeof(gotwebd->unix_socket_name)) {
 				yyerror("%s: unix_socket_name truncated",
 				    __func__);
 				free($2);
@@ -367,7 +368,8 @@ serveropts1	: REPOS_PATH STRING {
 			    sizeof(new_srv->unix_socket_name), "%s%s",
 			    strlen(gotwebd->httpd_chroot) ?
 			    gotwebd->httpd_chroot : D_HTTPD_CHROOT, $2);
-			if (n < 0) {
+			if (n < 0 ||
+			    (size_t)n >= sizeof(new_srv->unix_socket_name)) {
 				yyerror("%s: unix_socket_name truncated",
 				    __func__);
 				free($2);
@@ -886,7 +888,7 @@ conf_new_server(const char *name)
 	n = snprintf(srv->unix_socket_name,
 	    sizeof(srv->unix_socket_name), "%s%s", D_HTTPD_CHROOT,
 	    D_UNIX_SOCKET);
-	if (n < 0)
+	if (n < 0 || (size_t)n >= sizeof(srv->unix_socket_name))
 		fatalx("%s: snprintf", __func__);
 	n = strlcpy(srv->repos_path, D_GOTPATH,
 	    sizeof(srv->repos_path));
blob - 0b020ab754c33ba525068d380e871877d551f8e5
file + gotwebd/sockets.c
--- gotwebd/sockets.c
+++ gotwebd/sockets.c
@@ -179,7 +179,7 @@ sockets_dup_new_socket(struct socket *p_sock, struct s
 
 	n = snprintf(sock->conf.name, GOTWEBD_MAXTEXT, "%s_child",
 	    p_sock->conf.srv_name);
-	if (n < 0) {
+	if (n < 0 || (size_t)n >= GOTWEBD_MAXTEXT) {
 		free(p_sock->conf.al);
 		free(p_sock);
 		free(sock->conf.al);
@@ -252,7 +252,7 @@ sockets_conf_new_socket(struct gotwebd *env, struct se
 
 	n = snprintf(sock->conf.name, GOTWEBD_MAXTEXT, "%s_parent",
 	    srv->name);
-	if (n < 0) {
+	if (n < 0 || (size_t)n >= GOTWEBD_MAXTEXT) {
 		free(sock->conf.al);
 		free(sock);
 		fatalx("%s: snprintf", __func__);
blob - f671a4f105400e52465bc32e860ae0b0b1e71afd
file + lib/diff.c
--- lib/diff.c
+++ lib/diff.c
@@ -903,6 +903,9 @@ show_object_id(struct got_diff_line **lines, size_t *n
 	off_t outoff = 0;
 
 	n = fprintf(outfile, "%s %c %s\n", obj_typestr, ch, id_str);
+	if (n < 0)
+		return got_error_from_errno("fprintf");
+
 	if (lines != NULL && *lines != NULL) {
 		if (*nlines == 0) {
 			err = add_line_metadata(lines, nlines, 0,
blob - 9ce30e5bcc35c7934d3795a978a487d7624ef00b
file + lib/error.c
--- lib/error.c
+++ lib/error.c
@@ -364,7 +364,7 @@ got_error_no_obj(struct got_object_id *id)
 		return got_error(GOT_ERR_NO_OBJ);
 
 	ret = snprintf(msg, sizeof(msg), "object %s not found", id_str);
-	if (ret == -1 || ret >= sizeof(msg))
+	if (ret < 0 || (size_t)ret >= sizeof(msg))
 		return got_error(GOT_ERR_NO_OBJ);
 
 	return got_error_msg(GOT_ERR_NO_OBJ, msg);
@@ -377,7 +377,7 @@ got_error_not_ref(const char *refname)
 	int ret;
 
 	ret = snprintf(msg, sizeof(msg), "reference %s not found", refname);
-	if (ret == -1 || ret >= sizeof(msg))
+	if (ret < 0 || (size_t)ret >= sizeof(msg))
 		return got_error(GOT_ERR_NOT_REF);
 
 	return got_error_msg(GOT_ERR_NOT_REF, msg);
blob - 05652e3057827e3b2c8cc76f5e95d07a081979a7
file + lib/object_create.c
--- lib/object_create.c
+++ lib/object_create.c
@@ -272,7 +272,7 @@ te_mode2str(char *buf, size_t len, struct got_tree_ent
 		return got_error(GOT_ERR_BAD_FILETYPE);
 
 	ret = snprintf(buf, len, "%o ", mode);
-	if (ret == -1 || ret >= len)
+	if (ret < 0 || (size_t)ret >= len)
 		return got_error(GOT_ERR_NO_SPACE);
 	return NULL;
 }
blob - a845118f6e6f64c5061dd5eef11341341e9ad2ff
file + lib/pkt.c
--- lib/pkt.c
+++ lib/pkt.c
@@ -160,10 +160,11 @@ const struct got_error *
 got_pkt_writepkt(int fd, char *buf, int nbuf, int chattygot)
 {
 	char len[5];
-	int i;
+	int i, ret;
 	ssize_t w;
 
-	if (snprintf(len, sizeof(len), "%04x", nbuf + 4) >= sizeof(len))
+	ret = snprintf(len, sizeof(len), "%04x", nbuf + 4);
+	if (ret < 0 || (size_t)ret >= sizeof(len))
 		return got_error(GOT_ERR_NO_SPACE);
 	w = write(fd, len, 4);
 	if (w == -1)
blob - 1cbce6b3ad5b6169b26f1ca72e58e3bb1bad9d7f
file + libexec/got-fetch-pack/got-fetch-pack.c
--- libexec/got-fetch-pack/got-fetch-pack.c
+++ libexec/got-fetch-pack/got-fetch-pack.c
@@ -490,7 +490,7 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 		n = snprintf(buf, sizeof(buf), "want %s%s\n", hashstr,
 		    sent_my_capabilites || my_capabilities == NULL ?
 		    "" : my_capabilities);
-		if (n >= sizeof(buf)) {
+		if (n < 0 || (size_t)n >= sizeof(buf)) {
 			err = got_error(GOT_ERR_NO_SPACE);
 			goto done;
 		}
@@ -511,7 +511,7 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 		struct got_object_id *id = pe->data;
 		got_sha1_digest_to_str(id->sha1, hashstr, sizeof(hashstr));
 		n = snprintf(buf, sizeof(buf), "have %s\n", hashstr);
-		if (n >= sizeof(buf)) {
+		if (n < 0 || (size_t)n >= sizeof(buf)) {
 			err = got_error(GOT_ERR_NO_SPACE);
 			goto done;
 		}
blob - 11e392eb0efbb98ce85ecbfda9488b3f1373f483
file + libexec/got-send-pack/got-send-pack.c
--- libexec/got-send-pack/got-send-pack.c
+++ libexec/got-send-pack/got-send-pack.c
@@ -284,7 +284,7 @@ describe_refchange(int *n, int *sent_my_capabilites,
 {
 	*n = snprintf(buf, bufsize, "%s %s %s",
 	    old_hashstr, new_hashstr, refname);
-	if (*n >= bufsize)
+	if (*n < 0 || (size_t)*n >= bufsize)
 		return got_error(GOT_ERR_NO_SPACE);
 
 	/*
@@ -300,7 +300,7 @@ describe_refchange(int *n, int *sent_my_capabilites,
 			return got_error(GOT_ERR_NO_SPACE);
 		m = snprintf(buf + *n + 1, /* offset after '\0' */
 		    bufsize - (*n + 1), "%s\n", my_capabilities);
-		if (*n + m >= bufsize)
+		if (m < 0 || *n + m >= bufsize)
 			return got_error(GOT_ERR_NO_SPACE);
 		*n += m;
 		*sent_my_capabilites = 1;