Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers
On 5/19/2018 4:27 AM, René Scharfe wrote: Am 19.05.2018 um 03:57 schrieb Jeff King: These formatted integers should always fit into their 64-byte buffers. Let's use xsnprintf() to add an assertion that this is the case, which makes auditing for other unchecked snprintfs() easier. How about this instead? -- >8 -- - snprintf(ver, sizeof(ver), "%d", version); - snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update); - argv[1] = ver; - argv[2] = date; - argv[3] = NULL; - cp.argv = argv; + argv_array_push(, core_fsmonitor); + argv_array_pushf(, "%d", version); + argv_array_pushf(, "%" PRIuMAX, (uintmax_t)last_update); Looks good. Simpler, cleaner, less error prone. cp.use_shell = 1; cp.dir = get_git_work_tree();
Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers
René Scharfewrites: > 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. > - snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update); > - argv[1] = ver; > - argv[2] = date; > - argv[3] = NULL; > - cp.argv = argv; > + argv_array_push(, core_fsmonitor); > + argv_array_pushf(, "%d", version); If it were written like this from the beginning, such a bug would never have happened ;-) > + argv_array_pushf(, "%" PRIuMAX, (uintmax_t)last_update); > cp.use_shell = 1; > cp.dir = get_git_work_tree();
Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers
On Sat, May 19, 2018 at 10:27:46AM +0200, René Scharfe wrote: > Am 19.05.2018 um 03:57 schrieb Jeff King: > > These formatted integers should always fit into their > > 64-byte buffers. Let's use xsnprintf() to add an assertion > > that this is the case, which makes auditing for other > > unchecked snprintfs() easier. > > How about this instead? > > -- >8 -- > Subject: [PATCH] fsmonitor: use internal argv_array of struct child_process Yeah, I agree that is much nicer (I was so focused on the snprintfs I didn't bother to look at the context for a better solution). Thanks, getting a review in the form of a complete patch is my favorite kind of review. :) > Inspired-by: Jeff KingHeh. -Peff
Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers
Am 19.05.2018 um 03:57 schrieb Jeff King: > These formatted integers should always fit into their > 64-byte buffers. Let's use xsnprintf() to add an assertion > that this is the case, which makes auditing for other > unchecked snprintfs() easier. 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. Inspired-by: Jeff KingSigned-off-by: Rene Scharfe --- fsmonitor.c | 14 -- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/fsmonitor.c b/fsmonitor.c index ed3d1a074d..665bd2d425 100644 --- a/fsmonitor.c +++ b/fsmonitor.c @@ -97,19 +97,13 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate) static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result) { struct child_process cp = CHILD_PROCESS_INIT; - char ver[64]; - char date[64]; - const char *argv[4]; - if (!(argv[0] = core_fsmonitor)) + if (!core_fsmonitor) return -1; - snprintf(ver, sizeof(ver), "%d", version); - snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update); - argv[1] = ver; - argv[2] = date; - argv[3] = NULL; - cp.argv = argv; + argv_array_push(, core_fsmonitor); + argv_array_pushf(, "%d", version); + argv_array_pushf(, "%" PRIuMAX, (uintmax_t)last_update); cp.use_shell = 1; cp.dir = get_git_work_tree(); -- 2.17.0
[PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers
These formatted integers should always fit into their 64-byte buffers. Let's use xsnprintf() to add an assertion that this is the case, which makes auditing for other unchecked snprintfs() easier. Signed-off-by: Jeff King--- fsmonitor.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fsmonitor.c b/fsmonitor.c index ed3d1a074d..cc19b27e1d 100644 --- a/fsmonitor.c +++ b/fsmonitor.c @@ -104,8 +104,8 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que if (!(argv[0] = core_fsmonitor)) return -1; - snprintf(ver, sizeof(ver), "%d", version); - snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update); + xsnprintf(ver, sizeof(ver), "%d", version); + xsnprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update); argv[1] = ver; argv[2] = date; argv[3] = NULL; -- 2.17.0.1052.g7d69f75dbf