From: Omar Polo Subject: Re: *printf return value To: Theo Buehler Cc: gameoftrees@openbsd.org Date: Wed, 10 Aug 2022 15:34:46 +0200 On 2022/08/10 14:54:16 +0200, Theo Buehler 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;