{"thread":{"id":"6920","subject":"[PATCH] Limit filename for format-patch","startedAt":"2007-02-23T00:37:11Z","lastAt":"2007-02-24T11:24:40Z","messageCount":9,"participants":["Robin Rosenberg","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"35282","messageId":"20070223003711.6895.24185.stgit@lathund.dewire.com","threadId":"6920","inReplyTo":null,"subject":"[PATCH] Limit filename for format-patch","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2007-02-23T00:37:11Z","receivedAt":"2007-02-23T00:37:11Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"Badly formatted commits may have very long comments. This causes\ngit-format-patch to fail. To avoid that, truncate the filename\nto a value we believe will always work. \n\nErr out if the patch file cannot be created.\n\nSigned-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>\n---\n\n builtin-log.c |   22 ++++++++++++++++------\n 1 files changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex af2de54..e8dcea0 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -264,15 +264,17 @@ static int git_format_config(const char *var, const char *value)\n static FILE *realstdout = NULL;\n static const char *output_directory = NULL;\n \n-static void reopen_stdout(struct commit *commit, int nr, int keep_subject)\n+static int reopen_stdout(struct commit *commit, int nr, int keep_subject)\n {\n-\tchar filename[1024];\n+\tchar filename[PATH_MAX];\n \tchar *sol;\n \tint len = 0;\n-\tint suffix_len = strlen(fmt_patch_suffix) + 10; /* ., NUL and slop */\n+\tint suffix_len = strlen(fmt_patch_suffix) + 1;\n \n \tif (output_directory) {\n-\t\tstrlcpy(filename, output_directory, 1000);\n+\t\tif (strlen(output_directory) > sizeof filename - suffix_len)\n+\t\t\treturn error(\"name of output directory is too long\");\n+\t\tstrlcpy(filename, output_directory, sizeof filename - suffix_len);\n \t\tlen = strlen(filename);\n \t\tif (filename[len - 1] != '/')\n \t\t\tfilename[len++] = '/';\n@@ -297,6 +299,7 @@ static void reopen_stdout(struct commit *commit, int nr, int keep_subject)\n \t\t}\n \n \t\tfor (j = 0;\n+\t\t     j< 64-suffix_len-5 && \n \t\t     len < sizeof(filename) - suffix_len &&\n \t\t\t     sol[j] && sol[j] != '\\n';\n \t\t     j++) {\n@@ -314,10 +317,16 @@ static void reopen_stdout(struct commit *commit, int nr, int keep_subject)\n \t\t}\n \t\twhile (filename[len - 1] == '.' || filename[len - 1] == '-')\n \t\t\tlen--;\n+\t\tfilename[len] = 0;\n \t}\n+\tif (len + strlen(fmt_patch_suffix) >= sizeof filename)\n+\t\treturn error(\"Patch pathname too long\");\n \tstrcpy(filename + len, fmt_patch_suffix);\n \tfprintf(realstdout, \"%s\\n\", filename);\n-\tfreopen(filename, \"w\", stdout);\n+\tif (freopen(filename, \"w\", stdout) == NULL)\n+\t\treturn error(\"Cannot open patch file %s\",filename);\n+\treturn 0;\n+\t\n }\n \n static int get_patch_id(struct commit *commit, struct diff_options *options,\n@@ -573,7 +582,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\trev.message_id = message_id;\n \t\t}\n \t\tif (!use_stdout)\n-\t\t\treopen_stdout(commit, rev.nr, keep_subject);\n+\t\t\tif (reopen_stdout(commit, rev.nr, keep_subject))\n+\t\t\t\tdie(\"Failed to create output files\");\n \t\tshown = log_tree_commit(&rev, commit);\n \t\tfree(commit->buffer);\n \t\tcommit->buffer = NULL;\n"},{"id":"35283","messageId":"Pine.LNX.4.63.0702230143370.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6920","inReplyTo":"20070223003711.6895.24185.stgit@lathund.dewire.com","subject":"Re: [PATCH] Limit filename for format-patch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-02-23T00:49:01Z","receivedAt":"2007-02-23T00:49:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nI agree that your patch fixes a long-time issue.\n\nOn Fri, 23 Feb 2007, Robin Rosenberg wrote:\n\n> +\t\tif (strlen(output_directory) > sizeof filename - suffix_len)\n\nI know that \"sizeof filename\" works, but in git.git, `git grep sizeof' \nreturns > 700 instances, only 23 of which do not use the \n\"sizeof(filename)\" form. It's just a style issue, but I prefer the latter \nnevertheless...\n\n>  \t\tfor (j = 0;\n\nDoes this:\n\n> +\t\t     j< 64-suffix_len-5 && \n>  \t\t     len < sizeof(filename) - suffix_len &&\n>  \t\t\t     sol[j] && sol[j] != '\\n';\n>  \t\t     j++) {\n\nnot make this:\n\n> +\tif (len + strlen(fmt_patch_suffix) >= sizeof filename)\n> +\t\treturn error(\"Patch pathname too long\");\n\nunnecessary for the case that there _was_ an output directory specified? \nI'd make that an \"else if\"... But I might be missing something.\n\nCiao,\nDscho\n"},{"id":"35349","messageId":"200702232239.19504.robin.rosenberg@dewire.com","threadId":"6920","inReplyTo":"Pine.LNX.4.63.0702230143370.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] Limit filename for format-patch","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2007-02-23T21:39:18Z","receivedAt":"2007-02-23T21:39:18Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"fredag 23 februari 2007 01:49 skrev Johannes Schindelin:\n> Hi,\n> \n> I agree that your patch fixes a long-time issue.\n> \n> On Fri, 23 Feb 2007, Robin Rosenberg wrote:\n> \n> > +\t\tif (strlen(output_directory) > sizeof filename - suffix_len)\n> \n> I know that \"sizeof filename\" works, but in git.git, `git grep sizeof' \n> returns > 700 instances, only 23 of which do not use the \n> \"sizeof(filename)\" form. It's just a style issue, but I prefer the latter \n> nevertheless...\nActually there are (only) about 500 of them that are not the sizeof(type) kind.\n\nI prefer not to use extra parentheses except for grouping complex expressions \nfor readability.\n\n> >  \t\tfor (j = 0;\n> \n> Does this:\n> \n> > +\t\t     j< 64-suffix_len-5 && \n> >  \t\t     len < sizeof(filename) - suffix_len &&\n> >  \t\t\t     sol[j] && sol[j] != '\\n';\n> >  \t\t     j++) {\n> \n> not make this:\n> \n> > +\tif (len + strlen(fmt_patch_suffix) >= sizeof filename)\n> > +\t\treturn error(\"Patch pathname too long\");\n> \n> unnecessary for the case that there _was_ an output directory specified? \n> I'd make that an \"else if\"... But I might be missing something.\n\nThe last statement errs out if the total length is too long which is a different\nissue than truncating the patch name. There is no reason to two paths when\none does perfectly well.  But I could replace strlen there.\n\n-- robin\n"},{"id":"35350","messageId":"20070223214631.24544.96597.stgit@lathund.dewire.com","threadId":"6920","inReplyTo":"200702232239.19504.robin.rosenberg@dewire.com","subject":"[PATCH] Limit filename for format-patch","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2007-02-23T21:47:26Z","receivedAt":"2007-02-23T21:47:26Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"Badly formatted commits may have very long comments. This causes\ngit-format-patch to fail. To avoid that, truncate the filename\nto a value we believe will always work. \n\nErr out if the patch file cannot be created.\n\nSigned-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>\n---\n\nSame as previous patch minus one strlen.\n\n builtin-log.c |   22 ++++++++++++++++------\n 1 files changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex ad1e8c0..5afd772 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -264,15 +264,17 @@ static int git_format_config(const char *var, const char *value)\n static FILE *realstdout = NULL;\n static const char *output_directory = NULL;\n \n-static void reopen_stdout(struct commit *commit, int nr, int keep_subject)\n+static int reopen_stdout(struct commit *commit, int nr, int keep_subject)\n {\n-\tchar filename[1024];\n+\tchar filename[PATH_MAX];\n \tchar *sol;\n \tint len = 0;\n-\tint suffix_len = strlen(fmt_patch_suffix) + 10; /* ., NUL and slop */\n+\tint suffix_len = strlen(fmt_patch_suffix) + 1;\n \n \tif (output_directory) {\n-\t\tstrlcpy(filename, output_directory, 1000);\n+\t\tif (strlen(output_directory) > sizeof filename - suffix_len)\n+\t\t\treturn error(\"name of output directory is too long\");\n+\t\tstrlcpy(filename, output_directory, sizeof filename - suffix_len);\n \t\tlen = strlen(filename);\n \t\tif (filename[len - 1] != '/')\n \t\t\tfilename[len++] = '/';\n@@ -297,6 +299,7 @@ static void reopen_stdout(struct commit *commit, int nr, int keep_subject)\n \t\t}\n \n \t\tfor (j = 0;\n+\t\t     j< 64-suffix_len-5 && \n \t\t     len < sizeof(filename) - suffix_len &&\n \t\t\t     sol[j] && sol[j] != '\\n';\n \t\t     j++) {\n@@ -314,10 +317,16 @@ static void reopen_stdout(struct commit *commit, int nr, int keep_subject)\n \t\t}\n \t\twhile (filename[len - 1] == '.' || filename[len - 1] == '-')\n \t\t\tlen--;\n+\t\tfilename[len] = 0;\n \t}\n+\tif (len + suffix_len >= sizeof filename)\n+\t\treturn error(\"Patch pathname too long\");\n \tstrcpy(filename + len, fmt_patch_suffix);\n \tfprintf(realstdout, \"%s\\n\", filename);\n-\tfreopen(filename, \"w\", stdout);\n+\tif (freopen(filename, \"w\", stdout) == NULL)\n+\t\treturn error(\"Cannot open patch file %s\",filename);\n+\treturn 0;\n+\t\n }\n \n static int get_patch_id(struct commit *commit, struct diff_options *options,\n@@ -573,7 +582,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\trev.message_id = message_id;\n \t\t}\n \t\tif (!use_stdout)\n-\t\t\treopen_stdout(commit, rev.nr, keep_subject);\n+\t\t\tif (reopen_stdout(commit, rev.nr, keep_subject))\n+\t\t\t\tdie(\"Failed to create output files\");\n \t\tshown = log_tree_commit(&rev, commit);\n \t\tfree(commit->buffer);\n \t\tcommit->buffer = NULL;\n"},{"id":"35351","messageId":"20070223222606.28660.46208.stgit@lathund.dewire.com","threadId":"6920","inReplyTo":"200702232239.19504.robin.rosenberg@dewire.com","subject":"[PATCH] Limit filename for format-patch","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2007-02-23T22:27:58Z","receivedAt":"2007-02-23T22:27:58Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"Badly formatted commits may have very long comments. This causes\ngit-format-patch to fail. To avoid that, truncate the filename\nto a value we believe will always work. \n\nErr out if the patch file cannot be created.\n\nSigned-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>\n---\n\n builtin-log.c |   22 ++++++++++++++++------\n 1 files changed, 16 insertions(+), 6 deletions(-)\n\nThird edition. A buffer overflow fixed and more\npeaceful sizeof syntax.\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex ad1e8c0..9054fd4 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -264,15 +264,17 @@ static int git_format_config(const char *var, const char *value)\n static FILE *realstdout = NULL;\n static const char *output_directory = NULL;\n \n-static void reopen_stdout(struct commit *commit, int nr, int keep_subject)\n+static int reopen_stdout(struct commit *commit, int nr, int keep_subject)\n {\n-\tchar filename[1024];\n+\tchar filename[PATH_MAX];\n \tchar *sol;\n \tint len = 0;\n-\tint suffix_len = strlen(fmt_patch_suffix) + 10; /* ., NUL and slop */\n+\tint suffix_len = strlen(fmt_patch_suffix) + 1;\n \n \tif (output_directory) {\n-\t\tstrlcpy(filename, output_directory, 1000);\n+\t\tif (strlen(output_directory) >= sizeof(filename) - 64 - suffix_len)\n+\t\t\treturn error(\"name of output directory is too long\");\n+\t\tstrlcpy(filename, output_directory, sizeof(filename) - suffix_len);\n \t\tlen = strlen(filename);\n \t\tif (filename[len - 1] != '/')\n \t\t\tfilename[len++] = '/';\n@@ -297,6 +299,7 @@ static void reopen_stdout(struct commit *commit, int nr, int keep_subject)\n \t\t}\n \n \t\tfor (j = 0;\n+\t\t     j< 64-suffix_len-5 && \n \t\t     len < sizeof(filename) - suffix_len &&\n \t\t\t     sol[j] && sol[j] != '\\n';\n \t\t     j++) {\n@@ -314,10 +317,16 @@ static void reopen_stdout(struct commit *commit, int nr, int keep_subject)\n \t\t}\n \t\twhile (filename[len - 1] == '.' || filename[len - 1] == '-')\n \t\t\tlen--;\n+\t\tfilename[len] = 0;\n \t}\n+\tif (len + suffix_len >= sizeof(filename))\n+\t\treturn error(\"Patch pathname too long\");\n \tstrcpy(filename + len, fmt_patch_suffix);\n \tfprintf(realstdout, \"%s\\n\", filename);\n-\tfreopen(filename, \"w\", stdout);\n+\tif (freopen(filename, \"w\", stdout) == NULL)\n+\t\treturn error(\"Cannot open patch file %s\",filename);\n+\treturn 0;\n+\t\n }\n \n static int get_patch_id(struct commit *commit, struct diff_options *options,\n@@ -573,7 +582,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\trev.message_id = message_id;\n \t\t}\n \t\tif (!use_stdout)\n-\t\t\treopen_stdout(commit, rev.nr, keep_subject);\n+\t\t\tif (reopen_stdout(commit, rev.nr, keep_subject))\n+\t\t\t\tdie(\"Failed to create output files\");\n \t\tshown = log_tree_commit(&rev, commit);\n \t\tfree(commit->buffer);\n \t\tcommit->buffer = NULL;\n"},{"id":"35360","messageId":"Pine.LNX.4.63.0702240227150.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6920","inReplyTo":"20070223222606.28660.46208.stgit@lathund.dewire.com","subject":"Re: [PATCH] Limit filename for format-patch","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-02-24T01:27:52Z","receivedAt":"2007-02-24T01:27:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 23 Feb 2007, Robin Rosenberg wrote:\n\n> Badly formatted commits may have very long comments. This causes \n> git-format-patch to fail. To avoid that, truncate the filename to a \n> value we believe will always work.\n> \n> Err out if the patch file cannot be created.\n> \n> Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>\n\nIn only read the patch, but it looks sane enough. Acked-by: me.\n\nCiao,\nDscho\n"},{"id":"35363","messageId":"7vr6sg1gr4.fsf@assigned-by-dhcp.cox.net","threadId":"6920","inReplyTo":"20070223222606.28660.46208.stgit@lathund.dewire.com","subject":"Re: [PATCH] Limit filename for format-patch","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-24T01:33:35Z","receivedAt":"2007-02-24T01:33:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"},{"id":"35386","messageId":"7vtzxcym3t.fsf@assigned-by-dhcp.cox.net","threadId":"6920","inReplyTo":"20070223222606.28660.46208.stgit@lathund.dewire.com","subject":"Re: [PATCH] Limit filename for format-patch","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-24T08:51:34Z","receivedAt":"2007-02-24T08:51:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robin Rosenberg <robin.rosenberg@dewire.com> writes:\n\n> -static void reopen_stdout(struct commit *commit, int nr, int keep_subject)\n> +static int reopen_stdout(struct commit *commit, int nr, int keep_subject)\n>  {\n> -\tchar filename[1024];\n> +\tchar filename[PATH_MAX];\n>  \tchar *sol;\n>  \tint len = 0;\n> -\tint suffix_len = strlen(fmt_patch_suffix) + 10; /* ., NUL and slop */\n> +\tint suffix_len = strlen(fmt_patch_suffix) + 1;\n>  \n>  \tif (output_directory) {\n> -\t\tstrlcpy(filename, output_directory, 1000);\n> +\t\tif (strlen(output_directory) >= sizeof(filename) - 64 - suffix_len)\n> +\t\t\treturn error(\"name of output directory is too long\");\n> +\t\tstrlcpy(filename, output_directory, sizeof(filename) - suffix_len);\n\nSorry for a late doubt, but I started wondering if we should use\nNAME_MAX instead of hardcoded 64.  Purists might argue for using\npathconf() but I think it is an overkill.\n\nNAME_MAX is 255 on Linux, POSIX says it should be 14 at least\n(and further says if the platform supports only smaller max,\nNAME_MAX should not be defined -- heh).  I do not know how\nuniversal NAME_MAX is defined, and I hate dealing with header\nincompatibility across different systems, so I am tempted to\njust do something like:\n\n#define FORMAT_PATCH_NAME_MAX 64\n\nlocally, and use that instead.\n"},{"id":"35393","messageId":"200702241224.41208.robin.rosenberg@dewire.com","threadId":"6920","inReplyTo":"7vtzxcym3t.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Limit filename for format-patch","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2007-02-24T11:24:40Z","receivedAt":"2007-02-24T11:24:40Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"lördag 24 februari 2007 09:51 skrev Junio C Hamano:\n> Robin Rosenberg <robin.rosenberg@dewire.com> writes:\n> \n> > -static void reopen_stdout(struct commit *commit, int nr, int keep_subject)\n> > +static int reopen_stdout(struct commit *commit, int nr, int keep_subject)\n> >  {\n> > -\tchar filename[1024];\n> > +\tchar filename[PATH_MAX];\n> >  \tchar *sol;\n> >  \tint len = 0;\n> > -\tint suffix_len = strlen(fmt_patch_suffix) + 10; /* ., NUL and slop */\n> > +\tint suffix_len = strlen(fmt_patch_suffix) + 1;\n> >  \n> >  \tif (output_directory) {\n> > -\t\tstrlcpy(filename, output_directory, 1000);\n> > +\t\tif (strlen(output_directory) >= sizeof(filename) - 64 - suffix_len)\n> > +\t\t\treturn error(\"name of output directory is too long\");\n> > +\t\tstrlcpy(filename, output_directory, sizeof(filename) - suffix_len);\n> \n> Sorry for a late doubt, but I started wondering if we should use\n> NAME_MAX instead of hardcoded 64.  Purists might argue for using\n> pathconf() but I think it is an overkill.\nI think it'd sane to keep the name much shorter than what is actually possible. The\npatches will have a counter to keep the names unique anyway.\n\n> \n> NAME_MAX is 255 on Linux, POSIX says it should be 14 at least\n> (and further says if the platform supports only smaller max,\n> NAME_MAX should not be defined -- heh).  I do not know how\n> universal NAME_MAX is defined, and I hate dealing with header\n> incompatibility across different systems, so I am tempted to\n> just do something like:\n> \n> #define FORMAT_PATCH_NAME_MAX 64\n> \n> locally, and use that instead.\nOk with me.\n\n-- robin\n"}]}