From: Stefan Sperling <stsp@stsp.name>
Subject: Re: Only use string literals as format strings for dprintf()
To: Christian Weisgerber <naddy@mips.inka.de>
Cc: gameoftrees@openbsd.org
Date: Fri, 11 Sep 2020 10:13:23 +0200

On Fri, Sep 11, 2020 at 12:14:53AM +0200, Christian Weisgerber wrote:
> Todd C. Miller:
> 
> > asprintf(3) returns int, not size_t.
> 
> Ouch, of course.
> 
> > I would also split up the assignment for readability.
> 
> How about this:
> 
> diff 0823ffc2f6c509dbcedfb15d0d1011a253b45ef9 /home/naddy/got
> blob - 3a7013aa09b5952fd99ebb4cbf7e06235e769d42
> file + got/got.c
> --- got/got.c
> +++ got/got.c
> @@ -487,11 +487,13 @@ collect_import_msg(char **logmsg, char **logmsg_path, 
>  {
>  	char *initial_content = NULL;
>  	const struct got_error *err = NULL;
> +	int initial_content_len;
>  	int fd;
>  
> -	if (asprintf(&initial_content,
> +	initial_content_len = asprintf(&initial_content,
>  	    "\n# %s to be imported to branch %s\n", path_dir,
> -	    branch_name) == -1)
> +	    branch_name);
> +	if (initial_content_len == -1)
>  		return got_error_from_errno("asprintf");
>  
>  	err = got_opentemp_named_fd(logmsg_path, &fd,
> @@ -499,7 +501,7 @@ collect_import_msg(char **logmsg, char **logmsg_path, 
>  	if (err)
>  		goto done;
>  
> -	dprintf(fd, initial_content);
> +	write(fd, initial_content, initial_content_len);
>  	close(fd);

I would be pedantic and check the results of write(2) and close(2) here.
But that can be done as a follow-up change (don't worry about it if
you don't have time; I can get to it eventually).

Ok.

>  
>  	err = edit_logmsg(logmsg, editor, *logmsg_path, initial_content);
> @@ -5643,6 +5645,7 @@ get_tag_message(char **tagmsg, char **tagmsg_path, con
>  	const struct got_error *err = NULL;
>  	char *template = NULL, *initial_content = NULL;
>  	char *editor = NULL;
> +	int initial_content_len;
>  	int fd = -1;
>  
>  	if (asprintf(&template, GOT_TMPDIR_STR "/got-tagmsg") == -1) {
> @@ -5650,8 +5653,10 @@ get_tag_message(char **tagmsg, char **tagmsg_path, con
>  		goto done;
>  	}
>  
> -	if (asprintf(&initial_content, "\n# tagging commit %s as %s\n",
> -	    commit_id_str, tag_name) == -1) {
> +	initial_content_len = asprintf(&initial_content,
> +	    "\n# tagging commit %s as %s\n",
> +	    commit_id_str, tag_name);
> +	if (initial_content_len == -1) {
>  		err = got_error_from_errno("asprintf");
>  		goto done;
>  	}
> @@ -5660,7 +5665,7 @@ get_tag_message(char **tagmsg, char **tagmsg_path, con
>  	if (err)
>  		goto done;
>  
> -	dprintf(fd, initial_content);
> +	write(fd, initial_content, initial_content_len);
>  	close(fd);
>  
>  	err = get_editor(&editor);
> @@ -6505,6 +6510,7 @@ collect_commit_logmsg(struct got_pathlist_head *commit
>  	const struct got_error *err = NULL;
>  	char *template = NULL;
>  	struct collect_commit_logmsg_arg *a = arg;
> +	int initial_content_len;
>  	int fd;
>  	size_t len;
>  
> @@ -6521,16 +6527,17 @@ collect_commit_logmsg(struct got_pathlist_head *commit
>  	if (asprintf(&template, "%s/logmsg", a->worktree_path) == -1)
>  		return got_error_from_errno("asprintf");
>  
> -	if (asprintf(&initial_content,
> +	initial_content_len = asprintf(&initial_content,
>  	    "\n# changes to be committed on branch %s:\n",
> -	    a->branch_name) == -1)
> +	    a->branch_name);
> +	if (initial_content_len == -1)
>  		return got_error_from_errno("asprintf");
>  
>  	err = got_opentemp_named_fd(&a->logmsg_path, &fd, template);
>  	if (err)
>  		goto done;
>  
> -	dprintf(fd, initial_content);
> +	write(fd, initial_content, initial_content_len);
>  
>  	TAILQ_FOREACH(pe, commitable_paths, entry) {
>  		struct got_commitable *ct = pe->data;
> @@ -7715,6 +7722,7 @@ histedit_edit_logmsg(struct got_histedit_list_entry *h
>  	char *logmsg = NULL, *new_msg = NULL, *editor = NULL;
>  	const struct got_error *err = NULL;
>  	struct got_commit_object *commit = NULL;
> +	int logmsg_len;
>  	int fd;
>  	struct got_histedit_list_entry *folded = NULL;
>  
> @@ -7745,9 +7753,10 @@ histedit_edit_logmsg(struct got_histedit_list_entry *h
>  	err = got_object_commit_get_logmsg(&orig_logmsg, commit);
>  	if (err)
>  		goto done;
> -	if (asprintf(&new_msg,
> +	logmsg_len = asprintf(&new_msg,
>  	    "%s\n# original log message of commit %s: %s",
> -	    logmsg ? logmsg : "", id_str, orig_logmsg) == -1) {
> +	    logmsg ? logmsg : "", id_str, orig_logmsg);
> +	if (logmsg_len == -1) {
>  		err = got_error_from_errno("asprintf");
>  		goto done;
>  	}
> @@ -7763,7 +7772,7 @@ histedit_edit_logmsg(struct got_histedit_list_entry *h
>  	if (err)
>  		goto done;
>  
> -	dprintf(fd, logmsg);
> +	write(fd, logmsg, logmsg_len);
>  	close(fd);
>  
>  	err = get_editor(&editor);
> -- 
> Christian "naddy" Weisgerber                          naddy@mips.inka.de
> 
>