[PATCH] Limit filename for format-patch

[PATCH] Limit filename for format-patch

[Robin Rosenberg]

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 <robin.rosenberg@dewire.com>
---

 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;

Re: [PATCH] Limit filename for format-patch

[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...

>  		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

Re: [PATCH] Limit filename for format-patch

[Robin Rosenberg]

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

[PATCH] Limit filename for format-patch

[Robin Rosenberg]

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 <robin.rosenberg@dewire.com>
---

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;

[PATCH] Limit filename for format-patch

[Robin Rosenberg]

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 <robin.rosenberg@dewire.com>
---

 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;

Re: [PATCH] Limit filename for format-patch

[Johannes Schindelin]

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 <robin.rosenberg@dewire.com>

In only read the patch, but it looks sane enough. Acked-by: me.

Ciao,
Dscho

Re: [PATCH] Limit filename for format-patch

[Junio C Hamano]

Thanks.

Re: [PATCH] Limit filename for format-patch

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg@dewire.com> 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.

Re: [PATCH] Limit filename for format-patch

[Robin Rosenberg]

lördag 24 februari 2007 09:51 skrev Junio C Hamano:
> Robin Rosenberg <robin.rosenberg@dewire.com> 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