[PATCH] diff: avoid stack-buffer-read-overrun for very long name

Subsystems: the rest

DORMANTno replies

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

[PATCH] diff: avoid stack-buffer-read-overrun for very long name

From: Jim Meyering <hidden>
Date: 2016-06-15 22:53:36

Due to the use of strncpy without explicit NUL termination,
we could end up passing names n1 or n2 that are not NUL-terminated
to queue_diff, which requires NUL-terminated strings.
Ensure that each is NUL terminated.

Signed-off-by: Jim Meyering <redacted>
---
After finding strncpy problems in other projects, I audited
git for the same and found only these two.

 diff-no-index.c |    2 ++
 1 file changed, 2 insertions(+)
diff --git a/diff-no-index.c b/diff-no-index.c
index 3a36144..5cd3ff5 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -109,6 +109,7 @@ static int queue_diff(struct diff_options *o,
 				n1 = buffer1;
 				strncpy(buffer1 + len1, p1.items[i1++].string,
 						PATH_MAX - len1);
+				buffer1[PATH_MAX-1] = 0;
 			}

 			if (comp < 0)
@@ -117,6 +118,7 @@ static int queue_diff(struct diff_options *o,
 				n2 = buffer2;
 				strncpy(buffer2 + len2, p2.items[i2++].string,
 						PATH_MAX - len2);
+				buffer2[PATH_MAX-1] = 0;
 			}

 			ret = queue_diff(o, n1, n2);
--
1.7.10.169.g146fe

Re: [PATCH] diff: avoid stack-buffer-read-overrun for very long name

From: Marcus Karlsson <hidden>
Date: 2016-06-15 22:53:36

On Mon, Apr 16, 2012 at 05:20:02PM +0200, Jim Meyering wrote:
quoted hunk
Due to the use of strncpy without explicit NUL termination,
we could end up passing names n1 or n2 that are not NUL-terminated
to queue_diff, which requires NUL-terminated strings.
Ensure that each is NUL terminated.

Signed-off-by: Jim Meyering <redacted>
---
After finding strncpy problems in other projects, I audited
git for the same and found only these two.

 diff-no-index.c |    2 ++
 1 file changed, 2 insertions(+)
diff --git a/diff-no-index.c b/diff-no-index.c
index 3a36144..5cd3ff5 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -109,6 +109,7 @@ static int queue_diff(struct diff_options *o,
 				n1 = buffer1;
 				strncpy(buffer1 + len1, p1.items[i1++].string,
 						PATH_MAX - len1);
+				buffer1[PATH_MAX-1] = 0;
 			}

 			if (comp < 0)
@@ -117,6 +118,7 @@ static int queue_diff(struct diff_options *o,
 				n2 = buffer2;
 				strncpy(buffer2 + len2, p2.items[i2++].string,
 						PATH_MAX - len2);
+				buffer2[PATH_MAX-1] = 0;
 			}

 			ret = queue_diff(o, n1, n2);
--
1.7.10.169.g146fe
Are there any guarantees that len1 and len2 does not exceed PATH_MAX?
Because if there aren't any then that function looks like it could need
even more improvements.

	Marcus
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help