# [PATCH] Limit filename for format-patch 9 messages from 2007-02-23 to 2007-02-24. Participants: Robin Rosenberg, Johannes Schindelin, Junio C Hamano. Thread: https://gitlist.dev/t/6920 ## Robin Rosenberg, 2007-02-23 00:37 Subject: [PATCH] Limit filename for format-patch Message-ID: <20070223003711.6895.24185.stgit@lathund.dewire.com> URL: https://gitlist.dev/e/20070223003711.6895.24185.stgit%40lathund.dewire.com ``` Badly formatted commits may have very long comments. This causes git-format-patch to fail. To avoid that, truncate the filename to a value we believe will always work. Err out if the patch file cannot be created. Signed-off-by: Robin Rosenberg --- builtin-log.c | 22 ++++++++++++++++------ 1 files changed, 16 insertions(+), 6 deletions(-) diff --git a/builtin-log.c b/builtin-log.c index af2de54..e8dcea0 100644 --- a/builtin-log.c +++ b/builtin-log.c @@ -264,15 +264,17 @@ static int git_format_config(const char *var, const char *value) static FILE *realstdout = NULL; static const char *output_directory = NULL; -static void reopen_stdout(struct commit *commit, int nr, int keep_subject) +static int reopen_stdout(struct commit *commit, int nr, int keep_subject) { - char filename[1024]; + char filename[PATH_MAX]; char *sol; int len = 0; - int suffix_len = strlen(fmt_patch_suffix) + 10; /* ., NUL and slop */ + int suffix_len = strlen(fmt_patch_suffix) + 1; if (output_directory) { - strlcpy(filename, output_directory, 1000); + if (strlen(output_directory) > sizeof filename - suffix_len) + return error("name of output directory is too long"); + strlcpy(filename, output_directory, sizeof filename - suffix_len); len = strlen(filename); if (filename[len - 1] != '/') filename[len++] = '/'; @@ -297,6 +299,7 @@ static void reopen_stdout(struct commit *commit, int nr, int keep_subject) } for (j = 0; + j< 64-suffix_len-5 && len < sizeof(filename) - suffix_len && sol[j] && sol[j] != '\n'; j++) { @@ -314,10 +317,16 @@ static void reopen_stdout(struct commit *commit, int nr, int keep_subject) } while (filename[len - 1] == '.' || filename[len - 1] == '-') len--; + filename[len] = 0; } + if (len + strlen(fmt_patch_suffix) >= sizeof filename) + return error("Patch pathname too long"); strcpy(filename + len, fmt_patch_suffix); fprintf(realstdout, "%s\n", filename); - freopen(filename, "w", stdout); + if (freopen(filename, "w", stdout) == NULL) + return error("Cannot open patch file %s",filename); + return 0; + } static int get_patch_id(struct commit *commit, struct diff_options *options, @@ -573,7 +582,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) rev.message_id = message_id; } if (!use_stdout) - reopen_stdout(commit, rev.nr, keep_subject); + if (reopen_stdout(commit, rev.nr, keep_subject)) + die("Failed to create output files"); shown = log_tree_commit(&rev, commit); free(commit->buffer); commit->buffer = NULL; ``` ## Johannes Schindelin, 2007-02-23 00:49 Subject: Re: [PATCH] Limit filename for format-patch Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0702230143370.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <20070223003711.6895.24185.stgit@lathund.dewire.com> ``` Hi, I agree that your patch fixes a long-time issue. On Fri, 23 Feb 2007, Robin Rosenberg wrote: > + if (strlen(output_directory) > sizeof filename - suffix_len) I know that "sizeof filename" works, but in git.git, `git grep sizeof' returns > 700 instances, only 23 of which do not use the "sizeof(filename)" form. It's just a style issue, but I prefer the latter nevertheless... > for (j = 0; Does this: > + j< 64-suffix_len-5 && > len < sizeof(filename) - suffix_len && > sol[j] && sol[j] != '\n'; > j++) { not make this: > + if (len + strlen(fmt_patch_suffix) >= sizeof filename) > + return error("Patch pathname too long"); unnecessary for the case that there _was_ an output directory specified? I'd make that an "else if"... But I might be missing something. Ciao, Dscho ``` ## Robin Rosenberg, 2007-02-23 21:39 Subject: Re: [PATCH] Limit filename for format-patch Message-ID: <200702232239.19504.robin.rosenberg@dewire.com> URL: https://gitlist.dev/e/200702232239.19504.robin.rosenberg%40dewire.com In-Reply-To: ``` fredag 23 februari 2007 01:49 skrev Johannes Schindelin: > Hi, > > I agree that your patch fixes a long-time issue. > > On Fri, 23 Feb 2007, Robin Rosenberg wrote: > > > + if (strlen(output_directory) > sizeof filename - suffix_len) > > I know that "sizeof filename" works, but in git.git, `git grep sizeof' > returns > 700 instances, only 23 of which do not use the > "sizeof(filename)" form. It's just a style issue, but I prefer the latter > nevertheless... Actually there are (only) about 500 of them that are not the sizeof(type) kind. I prefer not to use extra parentheses except for grouping complex expressions for readability. > > for (j = 0; > > Does this: > > > + j< 64-suffix_len-5 && > > len < sizeof(filename) - suffix_len && > > sol[j] && sol[j] != '\n'; > > j++) { > > not make this: > > > + if (len + strlen(fmt_patch_suffix) >= sizeof filename) > > + return error("Patch pathname too long"); > > unnecessary for the case that there _was_ an output directory specified? > I'd make that an "else if"... But I might be missing something. The last statement errs out if the total length is too long which is a different issue than truncating the patch name. There is no reason to two paths when one does perfectly well. But I could replace strlen there. -- robin ``` ## Robin Rosenberg, 2007-02-23 21:47 Subject: [PATCH] Limit filename for format-patch Message-ID: <20070223214631.24544.96597.stgit@lathund.dewire.com> URL: https://gitlist.dev/e/20070223214631.24544.96597.stgit%40lathund.dewire.com In-Reply-To: <200702232239.19504.robin.rosenberg@dewire.com> ``` Badly formatted commits may have very long comments. This causes git-format-patch to fail. To avoid that, truncate the filename to a value we believe will always work. Err out if the patch file cannot be created. Signed-off-by: Robin Rosenberg --- Same as previous patch minus one strlen. builtin-log.c | 22 ++++++++++++++++------ 1 files changed, 16 insertions(+), 6 deletions(-) diff --git a/builtin-log.c b/builtin-log.c index ad1e8c0..5afd772 100644 --- a/builtin-log.c +++ b/builtin-log.c @@ -264,15 +264,17 @@ static int git_format_config(const char *var, const char *value) static FILE *realstdout = NULL; static const char *output_directory = NULL; -static void reopen_stdout(struct commit *commit, int nr, int keep_subject) +static int reopen_stdout(struct commit *commit, int nr, int keep_subject) { - char filename[1024]; + char filename[PATH_MAX]; char *sol; int len = 0; - int suffix_len = strlen(fmt_patch_suffix) + 10; /* ., NUL and slop */ + int suffix_len = strlen(fmt_patch_suffix) + 1; if (output_directory) { - strlcpy(filename, output_directory, 1000); + if (strlen(output_directory) > sizeof filename - suffix_len) + return error("name of output directory is too long"); + strlcpy(filename, output_directory, sizeof filename - suffix_len); len = strlen(filename); if (filename[len - 1] != '/') filename[len++] = '/'; @@ -297,6 +299,7 @@ static void reopen_stdout(struct commit *commit, int nr, int keep_subject) } for (j = 0; + j< 64-suffix_len-5 && len < sizeof(filename) - suffix_len && sol[j] && sol[j] != '\n'; j++) { @@ -314,10 +317,16 @@ static void reopen_stdout(struct commit *commit, int nr, int keep_subject) } while (filename[len - 1] == '.' || filename[len - 1] == '-') len--; + filename[len] = 0; } + if (len + suffix_len >= sizeof filename) + return error("Patch pathname too long"); strcpy(filename + len, fmt_patch_suffix); fprintf(realstdout, "%s\n", filename); - freopen(filename, "w", stdout); + if (freopen(filename, "w", stdout) == NULL) + return error("Cannot open patch file %s",filename); + return 0; + } static int get_patch_id(struct commit *commit, struct diff_options *options, @@ -573,7 +582,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) rev.message_id = message_id; } if (!use_stdout) - reopen_stdout(commit, rev.nr, keep_subject); + if (reopen_stdout(commit, rev.nr, keep_subject)) + die("Failed to create output files"); shown = log_tree_commit(&rev, commit); free(commit->buffer); commit->buffer = NULL; ``` ## Robin Rosenberg, 2007-02-23 22:27 Subject: [PATCH] Limit filename for format-patch Message-ID: <20070223222606.28660.46208.stgit@lathund.dewire.com> URL: https://gitlist.dev/e/20070223222606.28660.46208.stgit%40lathund.dewire.com In-Reply-To: <200702232239.19504.robin.rosenberg@dewire.com> ``` Badly formatted commits may have very long comments. This causes git-format-patch to fail. To avoid that, truncate the filename to a value we believe will always work. Err out if the patch file cannot be created. Signed-off-by: Robin Rosenberg --- builtin-log.c | 22 ++++++++++++++++------ 1 files changed, 16 insertions(+), 6 deletions(-) Third edition. A buffer overflow fixed and more peaceful sizeof syntax. diff --git a/builtin-log.c b/builtin-log.c index ad1e8c0..9054fd4 100644 --- a/builtin-log.c +++ b/builtin-log.c @@ -264,15 +264,17 @@ static int git_format_config(const char *var, const char *value) static FILE *realstdout = NULL; static const char *output_directory = NULL; -static void reopen_stdout(struct commit *commit, int nr, int keep_subject) +static int reopen_stdout(struct commit *commit, int nr, int keep_subject) { - char filename[1024]; + char filename[PATH_MAX]; char *sol; int len = 0; - int suffix_len = strlen(fmt_patch_suffix) + 10; /* ., NUL and slop */ + int suffix_len = strlen(fmt_patch_suffix) + 1; if (output_directory) { - strlcpy(filename, output_directory, 1000); + if (strlen(output_directory) >= sizeof(filename) - 64 - suffix_len) + return error("name of output directory is too long"); + strlcpy(filename, output_directory, sizeof(filename) - suffix_len); len = strlen(filename); if (filename[len - 1] != '/') filename[len++] = '/'; @@ -297,6 +299,7 @@ static void reopen_stdout(struct commit *commit, int nr, int keep_subject) } for (j = 0; + j< 64-suffix_len-5 && len < sizeof(filename) - suffix_len && sol[j] && sol[j] != '\n'; j++) { @@ -314,10 +317,16 @@ static void reopen_stdout(struct commit *commit, int nr, int keep_subject) } while (filename[len - 1] == '.' || filename[len - 1] == '-') len--; + filename[len] = 0; } + if (len + suffix_len >= sizeof(filename)) + return error("Patch pathname too long"); strcpy(filename + len, fmt_patch_suffix); fprintf(realstdout, "%s\n", filename); - freopen(filename, "w", stdout); + if (freopen(filename, "w", stdout) == NULL) + return error("Cannot open patch file %s",filename); + return 0; + } static int get_patch_id(struct commit *commit, struct diff_options *options, @@ -573,7 +582,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) rev.message_id = message_id; } if (!use_stdout) - reopen_stdout(commit, rev.nr, keep_subject); + if (reopen_stdout(commit, rev.nr, keep_subject)) + die("Failed to create output files"); shown = log_tree_commit(&rev, commit); free(commit->buffer); commit->buffer = NULL; ``` ## Johannes Schindelin, 2007-02-24 01:27 Subject: Re: [PATCH] Limit filename for format-patch Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0702240227150.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <20070223222606.28660.46208.stgit@lathund.dewire.com> ``` Hi, On Fri, 23 Feb 2007, Robin Rosenberg wrote: > Badly formatted commits may have very long comments. This causes > git-format-patch to fail. To avoid that, truncate the filename to a > value we believe will always work. > > Err out if the patch file cannot be created. > > Signed-off-by: Robin Rosenberg In only read the patch, but it looks sane enough. Acked-by: me. Ciao, Dscho ``` ## Junio C Hamano, 2007-02-24 01:33 Subject: Re: [PATCH] Limit filename for format-patch Message-ID: <7vr6sg1gr4.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vr6sg1gr4.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20070223222606.28660.46208.stgit@lathund.dewire.com> ``` Thanks. ``` ## Junio C Hamano, 2007-02-24 08:51 Subject: Re: [PATCH] Limit filename for format-patch Message-ID: <7vtzxcym3t.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vtzxcym3t.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20070223222606.28660.46208.stgit@lathund.dewire.com> ``` Robin Rosenberg writes: > -static void reopen_stdout(struct commit *commit, int nr, int keep_subject) > +static int reopen_stdout(struct commit *commit, int nr, int keep_subject) > { > - char filename[1024]; > + char filename[PATH_MAX]; > char *sol; > int len = 0; > - int suffix_len = strlen(fmt_patch_suffix) + 10; /* ., NUL and slop */ > + int suffix_len = strlen(fmt_patch_suffix) + 1; > > if (output_directory) { > - strlcpy(filename, output_directory, 1000); > + if (strlen(output_directory) >= sizeof(filename) - 64 - suffix_len) > + return error("name of output directory is too long"); > + strlcpy(filename, output_directory, sizeof(filename) - suffix_len); Sorry for a late doubt, but I started wondering if we should use NAME_MAX instead of hardcoded 64. Purists might argue for using pathconf() but I think it is an overkill. NAME_MAX is 255 on Linux, POSIX says it should be 14 at least (and further says if the platform supports only smaller max, NAME_MAX should not be defined -- heh). I do not know how universal NAME_MAX is defined, and I hate dealing with header incompatibility across different systems, so I am tempted to just do something like: #define FORMAT_PATCH_NAME_MAX 64 locally, and use that instead. ``` ## Robin Rosenberg, 2007-02-24 11:24 Subject: Re: [PATCH] Limit filename for format-patch Message-ID: <200702241224.41208.robin.rosenberg@dewire.com> URL: https://gitlist.dev/e/200702241224.41208.robin.rosenberg%40dewire.com In-Reply-To: <7vtzxcym3t.fsf@assigned-by-dhcp.cox.net> ``` lördag 24 februari 2007 09:51 skrev Junio C Hamano: > Robin Rosenberg writes: > > > -static void reopen_stdout(struct commit *commit, int nr, int keep_subject) > > +static int reopen_stdout(struct commit *commit, int nr, int keep_subject) > > { > > - char filename[1024]; > > + char filename[PATH_MAX]; > > char *sol; > > int len = 0; > > - int suffix_len = strlen(fmt_patch_suffix) + 10; /* ., NUL and slop */ > > + int suffix_len = strlen(fmt_patch_suffix) + 1; > > > > if (output_directory) { > > - strlcpy(filename, output_directory, 1000); > > + if (strlen(output_directory) >= sizeof(filename) - 64 - suffix_len) > > + return error("name of output directory is too long"); > > + strlcpy(filename, output_directory, sizeof(filename) - suffix_len); > > Sorry for a late doubt, but I started wondering if we should use > NAME_MAX instead of hardcoded 64. Purists might argue for using > pathconf() but I think it is an overkill. I think it'd sane to keep the name much shorter than what is actually possible. The patches will have a counter to keep the names unique anyway. > > NAME_MAX is 255 on Linux, POSIX says it should be 14 at least > (and further says if the platform supports only smaller max, > NAME_MAX should not be defined -- heh). I do not know how > universal NAME_MAX is defined, and I hate dealing with header > incompatibility across different systems, so I am tempted to > just do something like: > > #define FORMAT_PATCH_NAME_MAX 64 > > locally, and use that instead. Ok with me. -- robin ```