From: Omar Polo Subject: Re: replace malloc+memcpy with strndup To: Bryan Steele Cc: gameoftrees@openbsd.org Date: Tue, 10 Jan 2023 23:10:01 +0100 On 2023/01/10 15:05:07 -0500, Bryan Steele wrote: > On Tue, Jan 10, 2023 at 07:11:00PM +0100, Omar Polo wrote: > > as per subject. no functional change intended, just shorten some > > chunks of code > > > > ok? > > > > ... > > @@ -667,25 +665,18 @@ recv_capability(struct gotd_session_client *client, st > > if (datalen != sizeof(icapa) + icapa.key_len + icapa.value_len) > > return got_error(GOT_ERR_PRIVSEP_LEN); > > > > - key = malloc(icapa.key_len + 1); > > + key = strndp(imsg->data + sizeof(icapa), icapa.key_len); > > Typo here, but you probably already noticed. :-) No idea how that ended up there. there was another typo in the diff, sorry. Probably i've forgot to rebuild gotd after the first sweep. updated and recheked diff, this one should be fine :) thanks! ----------------------------------------------- commit 5055132302bb46b787a31fd6a2b46d50fb2eb431 (x) from: Omar Polo date: Tue Jan 10 22:07:30 2023 UTC replace malloc+memcpy with strndup. no functional change intended diff e26970ccc755b7327924d761d1772a1bc5a5bf01 5055132302bb46b787a31fd6a2b46d50fb2eb431 commit - e26970ccc755b7327924d761d1772a1bc5a5bf01 commit + 5055132302bb46b787a31fd6a2b46d50fb2eb431 blob - 3c8cfad838be7bad650a556478fd84cdb32844f8 blob + 7f653f3ebb5d4da89d29058699af3f90cdb7baca --- gotd/repo_write.c +++ gotd/repo_write.c @@ -349,11 +349,9 @@ recv_ref_update(struct imsg *imsg) imsg_init(&ibuf, client->fd); - refname = malloc(iref.name_len + 1); + refname = strndup(imsg->data + sizeof(iref), iref.name_len); if (refname == NULL) - return got_error_from_errno("malloc"); - memcpy(refname, imsg->data + sizeof(iref), iref.name_len); - refname[iref.name_len] = '\0'; + return got_error_from_errno("strndup"); ref_update = calloc(1, sizeof(*ref_update)); if (ref_update == NULL) { blob - a6b8f011af70edd6ca8e1dd6ffc0658788c67ad1 blob + c40b69f8f26a58591321c159a47de3d0835b9dfb --- gotd/session.c +++ gotd/session.c @@ -419,11 +419,9 @@ update_ref(struct gotd_session_client *client, const c memcpy(&iref, imsg->data, sizeof(iref)); if (datalen != sizeof(iref) + iref.name_len) return got_error(GOT_ERR_PRIVSEP_LEN); - refname = malloc(iref.name_len + 1); + refname = strndup(imsg->data + sizeof(iref), iref.name_len); if (refname == NULL) - return got_error_from_errno("malloc"); - memcpy(refname, imsg->data + sizeof(iref), iref.name_len); - refname[iref.name_len] = '\0'; + return got_error_from_errno("strndup"); log_debug("updating ref %s for uid %d", refname, client->euid); @@ -667,25 +665,18 @@ recv_capability(struct gotd_session_client *client, st if (datalen != sizeof(icapa) + icapa.key_len + icapa.value_len) return got_error(GOT_ERR_PRIVSEP_LEN); - key = malloc(icapa.key_len + 1); + key = strndup(imsg->data + sizeof(icapa), icapa.key_len); if (key == NULL) - return got_error_from_errno("malloc"); + return got_error_from_errno("strndup"); if (icapa.value_len > 0) { - value = malloc(icapa.value_len + 1); + value = strndup(imsg->data + sizeof(icapa) + icapa.key_len, + icapa.value_len); if (value == NULL) { free(key); - return got_error_from_errno("malloc"); + return got_error_from_errno("strndup"); } } - memcpy(key, imsg->data + sizeof(icapa), icapa.key_len); - key[icapa.key_len] = '\0'; - if (value) { - memcpy(value, imsg->data + sizeof(icapa) + icapa.key_len, - icapa.value_len); - value[icapa.value_len] = '\0'; - } - capa = &client->capabilities[client->ncapabilities++]; capa->key = key; capa->value = value; blob - a5c2c23ebfc28776e46ebd4b14022bf9b712e0e3 blob + 9762b99158cbd5c99d5b4169ddb59dc85d9f747c --- lib/gitconfig.c +++ lib/gitconfig.c @@ -237,10 +237,9 @@ conf_parse_line(char **section, struct got_gitconfig * *section = NULL; return NULL; } - *section = malloc(i); + *section = strndup(line + 1, i - 1); if (*section == NULL) - return got_error_from_errno("malloc"); - strlcpy(*section, line + 1, i); + return got_error_from_errno("strndup"); return NULL; } while (isspace((unsigned char)*line)) blob - 174d412f27d59a5180400ca4bbd33f6a4b8e5564 blob + d7a1dda565c2f2ee404c15276ebdd47e29965ed9 --- lib/privsep.c +++ lib/privsep.c @@ -2088,14 +2088,11 @@ got_privsep_recv_gitconfig_str(char **str, struct imsg case GOT_IMSG_GITCONFIG_STR_VAL: if (datalen == 0) break; - /* datalen does not include terminating \0 */ - *str = malloc(datalen + 1); + *str = strndup(imsg.data, datalen); if (*str == NULL) { - err = got_error_from_errno("malloc"); + err = got_error_from_errno("strndup"); break; } - memcpy(*str, imsg.data, datalen); - (*str)[datalen] = '\0'; break; default: err = got_error(GOT_ERR_PRIVSEP_MSG); @@ -2366,14 +2363,11 @@ got_privsep_recv_gotconfig_str(char **str, struct imsg case GOT_IMSG_GOTCONFIG_STR_VAL: if (datalen == 0) break; - /* datalen does not include terminating \0 */ - *str = malloc(datalen + 1); + *str = strndup(imsg.data, datalen); if (*str == NULL) { - err = got_error_from_errno("malloc"); + err = got_error_from_errno("strndup"); break; } - memcpy(*str, imsg.data, datalen); - (*str)[datalen] = '\0'; break; default: err = got_error(GOT_ERR_PRIVSEP_MSG); @@ -2828,9 +2822,9 @@ got_privsep_recv_enumerated_objects(int *found_all_obj } memcpy(tree_id.sha1, itree->id, sizeof(tree_id.sha1)); free(path); - path = malloc(path_len + 1); + path = strndup(imsg.data + sizeof(*itree), path_len); if (path == NULL) { - err = got_error_from_errno("malloc"); + err = got_error_from_errno("strndup"); break; } free(canon_path); @@ -2839,9 +2833,6 @@ got_privsep_recv_enumerated_objects(int *found_all_obj err = got_error_from_errno("malloc"); break; } - memcpy(path, (uint8_t *)imsg.data + sizeof(*itree), - path_len); - path[path_len] = '\0'; if (!got_path_is_absolute(path)) { err = got_error(GOT_ERR_BAD_PATH); break; blob - 1916c9b0c7afe7a7fba9ce4f0009229fc986da6a blob + 6e4c3ef0627fe92f92f4091b2926040aa3d9d9db --- lib/serve.c +++ lib/serve.c @@ -339,14 +339,12 @@ announce_refs(int outfd, struct imsgbuf *ibuf, int cli err = got_error(GOT_ERR_PRIVSEP_LEN); goto done; } - refname = malloc(iref.name_len + 1); - if (refname == NULL) { - err = got_error_from_errno("malloc"); - goto done; - } - memcpy(refname, imsg.data + sizeof(iref), + refname = strndup(imsg.data + sizeof(iref), iref.name_len); - refname[iref.name_len] = '\0'; + if (refname == NULL) { + err = got_error_from_errno("strndup"); + goto done; + } err = send_ref(outfd, iref.id, refname, !sent_capabilities, client_is_reading, NULL, chattygot); @@ -382,24 +380,20 @@ announce_refs(int outfd, struct imsgbuf *ibuf, int cli symrefname != NULL || symreftarget != NULL) break; - symrefname = malloc(isymref.name_len + 1); - if (symrefname == NULL) { - err = got_error_from_errno("malloc"); - goto done; - } - memcpy(symrefname, imsg.data + sizeof(isymref), + symrefname = strndup(imsg.data + sizeof(isymref), isymref.name_len); - symrefname[isymref.name_len] = '\0'; - - symreftarget = malloc(isymref.target_len + 1); - if (symreftarget == NULL) { + if (symrefname == NULL) { err = got_error_from_errno("malloc"); goto done; } - memcpy(symreftarget, + + symreftarget = strndup( imsg.data + sizeof(isymref) + isymref.name_len, isymref.target_len); - symreftarget[isymref.target_len] = '\0'; + if (symreftarget == NULL) { + err = got_error_from_errno("strndup"); + goto done; + } if (asprintf(&symrefstr, "%s:%s", symrefname, symreftarget) == -1) { @@ -1219,13 +1213,11 @@ report_unpack_status(struct imsg *imsg, int outfd, int if (datalen != sizeof(istatus) + istatus.reason_len) return got_error(GOT_ERR_PRIVSEP_LEN); - reason = malloc(istatus.reason_len + 1); + reason = strndup(imsg->data + sizeof(istatus), istatus.reason_len); if (reason == NULL) { - err = got_error_from_errno("malloc"); + err = got_error_from_errno("strndup"); goto done; } - memcpy(reason, imsg->data + sizeof(istatus), istatus.reason_len); - reason[istatus.reason_len] = '\0'; if (err == NULL) len = snprintf(buf, sizeof(buf), "unpack ok\n"); @@ -1260,11 +1252,9 @@ recv_ref_update_ok(struct imsg *imsg, int outfd, int c memcpy(&iok, imsg->data, sizeof(iok)); - refname = malloc(iok.name_len + 1); + refname = strndup(imsg->data + sizeof(iok), iok.name_len); if (refname == NULL) - return got_error_from_errno("malloc"); - memcpy(refname, imsg->data + sizeof(iok), iok.name_len); - refname[iok.name_len] = '\0'; + return got_error_from_errno("strndup"); len = snprintf(buf, sizeof(buf), "ok %s\n", refname); if (len >= sizeof(buf)) { @@ -1296,20 +1286,16 @@ recv_ref_update_ng(struct imsg *imsg, int outfd, int c memcpy(&ing, imsg->data, sizeof(ing)); - refname = malloc(ing.name_len + 1); + refname = strndup(imsg->data + sizeof(ing), ing.name_len); if (refname == NULL) - return got_error_from_errno("malloc"); - memcpy(refname, imsg->data + sizeof(ing), ing.name_len); - refname[ing.name_len] = '\0'; + return got_error_from_errno("strndup"); - reason = malloc(ing.reason_len + 1); - if (reason == NULL) { - err = got_error_from_errno("malloc"); - goto done; - } - memcpy(refname, imsg->data + sizeof(ing) + ing.name_len, + reason = strndup(imsg->data + sizeof(ing) + ing.name_len, ing.reason_len); - refname[ing.reason_len] = '\0'; + if (reason == NULL) { + err = got_error_from_errno("strndup"); + goto done; + } len = snprintf(buf, sizeof(buf), "ng %s %s\n", refname, reason); if (len >= sizeof(buf)) { blob - cbcfef7d6f9e5c10b699ada020cdf69f79490487 blob + 6e1ae2146ae101481c8223adead2d0fbc046ed00 --- libexec/got-fetch-pack/got-fetch-pack.c +++ libexec/got-fetch-pack/got-fetch-pack.c @@ -854,13 +854,11 @@ main(int argc, char **argv) err = got_error(GOT_ERR_PRIVSEP_LEN); goto done; } - refname = malloc(href.name_len + 1); + refname = strndup(imsg.data + sizeof(href), href.name_len); if (refname == NULL) { - err = got_error_from_errno("malloc"); + err = got_error_from_errno("strndup"); goto done; } - memcpy(refname, imsg.data + sizeof(href), href.name_len); - refname[href.name_len] = '\0'; id = malloc(sizeof(*id)); if (id == NULL) { @@ -904,13 +902,12 @@ main(int argc, char **argv) err = got_error(GOT_ERR_PRIVSEP_LEN); goto done; } - refname = malloc(wbranch.name_len + 1); + refname = strndup(imsg.data + sizeof(wbranch), + wbranch.name_len); if (refname == NULL) { - err = got_error_from_errno("malloc"); + err = got_error_from_errno("strndup"); goto done; } - memcpy(refname, imsg.data + sizeof(wbranch), wbranch.name_len); - refname[wbranch.name_len] = '\0'; err = got_pathlist_append(&wanted_branches, refname, NULL); if (err) { @@ -946,13 +943,11 @@ main(int argc, char **argv) err = got_error(GOT_ERR_PRIVSEP_LEN); goto done; } - refname = malloc(wref.name_len + 1); + refname = strndup(imsg.data + sizeof(wref), wref.name_len); if (refname == NULL) { - err = got_error_from_errno("malloc"); + err = got_error_from_errno("strndup"); goto done; } - memcpy(refname, imsg.data + sizeof(wref), wref.name_len); - refname[wref.name_len] = '\0'; err = got_pathlist_append(&wanted_refs, refname, NULL); if (err) {