Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers

From: Junio C Hamano <gitster@pobox.com>
Date: May 21, 2018, 00:58 UTC
Message-ID: <xmqqmuwt3jvn.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <cd6dba3c-19bb-e6c7-0c1f-5cea0c8f9d81@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 18 quoted lines

> How about this instead?
>
> -- >8 --
> Subject: [PATCH] fsmonitor: use internal argv_array of struct child_process
>
> Avoid magic array sizes and indexes by constructing the fsmonitor
> command line using the embedded argv_array of the child_process.  The
> resulting code is shorter and easier to extend.
>
> Getting rid of the snprintf() calls is a bonus -- even though the
> buffers were big enough here to avoid truncation -- as it makes auditing
> the remaining callers easier.
> ...
> -	if (!(argv[0] = core_fsmonitor))
> +	if (!core_fsmonitor)
>  		return -1;
>  
> -	snprintf(ver, sizeof(ver), "%d", version);

I really like this change, as this exact line used to take sizeof(version) instead of sizeof(ver), which has been corrected recently.

Show 7 quoted lines

> -	snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update);
> -	argv[1] = ver;
> -	argv[2] = date;
> -	argv[3] = NULL;
> -	cp.argv = argv;
> +	argv_array_push(&cp.args, core_fsmonitor);
> +	argv_array_pushf(&cp.args, "%d", version);

If it were written like this from the beginning, such a bug would never have happened ;-)

> +	argv_array_pushf(&cp.args, "%" PRIuMAX, (uintmax_t)last_update);
>  	cp.use_shell = 1;
>  	cp.dir = get_git_work_tree();