| Message ID | 1b5f3b1d-60e2-4fe7-9ac8-a63ad861cd16@web.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] apply: avoid fixed-size buffer in create_one_file() | expand |
René Scharfe <l.s.r@web.de> writes: > Changes since v1: > - Split out removal of mksnpath() into a separate patch. > - Use errno only before calling free(3). Relative to the previous round, the body of the loop has become much easier to follow. Nice clean-up. Thanks, both. > apply.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/apply.c b/apply.c > index 432837a674..e171b42904 100644 > --- a/apply.c > +++ b/apply.c > @@ -4441,6 +4441,7 @@ static int create_one_file(struct apply_state *state, > const char *buf, > unsigned long size) > { > + char *newpath = NULL; > int res; > > if (state->cached) > @@ -4502,24 +4503,26 @@ static int create_one_file(struct apply_state *state, > unsigned int nr = getpid(); > > for (;;) { > - char newpath[PATH_MAX]; > - mksnpath(newpath, sizeof(newpath), "%s~%u", path, nr); > + newpath = mkpathdup("%s~%u", path, nr); > res = try_create_file(state, newpath, mode, buf, size); > if (res < 0) > - return -1; > + goto out; > if (!res) { > if (!rename(newpath, path)) > - return 0; > + goto out; > unlink_or_warn(newpath); > break; > } > if (errno != EEXIST) > break; > ++nr; > + FREE_AND_NULL(newpath); > } > } > - return error_errno(_("unable to write file '%s' mode %o"), > - path, mode); > + res = error_errno(_("unable to write file '%s' mode %o"), path, mode); > +out: > + free(newpath); > + return res; > } > > static int add_conflicted_stages_file(struct apply_state *state, > -- > 2.44.0
diff --git a/apply.c b/apply.c index 432837a674..e171b42904 100644 --- a/apply.c +++ b/apply.c @@ -4441,6 +4441,7 @@ static int create_one_file(struct apply_state *state, const char *buf, unsigned long size) { + char *newpath = NULL; int res; if (state->cached) @@ -4502,24 +4503,26 @@ static int create_one_file(struct apply_state *state, unsigned int nr = getpid(); for (;;) { - char newpath[PATH_MAX]; - mksnpath(newpath, sizeof(newpath), "%s~%u", path, nr); + newpath = mkpathdup("%s~%u", path, nr); res = try_create_file(state, newpath, mode, buf, size); if (res < 0) - return -1; + goto out; if (!res) { if (!rename(newpath, path)) - return 0; + goto out; unlink_or_warn(newpath); break; } if (errno != EEXIST) break; ++nr; + FREE_AND_NULL(newpath); } } - return error_errno(_("unable to write file '%s' mode %o"), - path, mode); + res = error_errno(_("unable to write file '%s' mode %o"), path, mode); +out: + free(newpath); + return res; } static int add_conflicted_stages_file(struct apply_state *state,
PATH_MAX is not always a hard limit and 'path' in create_one_file() could be longer -- it's taken from the patch file and allocated dynamically. Allocate the name of the temporary file on the heap as well instead of using a fixed-size buffer to avoid that arbitrary limit. Resist the temptation of using the more convenient mkpath() to avoid introducing a dependency on a static variable deep inside the apply machinery. Take care to work around (arguably buggy) implementations of free(3) that modify errno, by calling it only after using the errno value. Suggested-by: Jeff King <peff@peff.net> Helped-by: Jeff King <peff@peff.net> Signed-off-by: René Scharfe <l.s.r@web.de> --- Changes since v1: - Split out removal of mksnpath() into a separate patch. - Use errno only before calling free(3). apply.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) -- 2.44.0