Re: [PATCH 24/30] merge-recursive: Add computation of collisions due to dir rename & merging
- From
Jeff King <peff@peff.net>
- Date
- Jun 10, 2018, 20:44 UTC
- Message-ID
- <20180610204419.GA11273@sigill.intra.peff.net>
- In-Reply-To
- <7d1237c7-5a83-d766-7d93-5f0d59166067@web.de>
On Sun, Jun 10, 2018 at 12:56:31PM +0200, René Scharfe wrote:
Show 8 quoted lines
> The value of PATH_MAX is platform-dependent, so it's easy to exceed when > doing cross-platform development. It's also not a hard limit on most > operating systems, not even on Windows. Further reading: > > https://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html > > So using a fixed buffer is not a good idea, and writing to it without > checking is dangerous. Here's a fix:
Even on platforms where it _is_ a hard-limit, we are quite often dealing with paths that come from tree objects (so even if the OS would eventually complain about our path, it is small consolation when we smash the stack before we get there).
Your patch looks good to me, and we definitely should address this before v2.18-final.
Show 7 quoted lines
> - char temp[PATH_MAX]; > + char *temp = xstrdup(path); > char *end; > - struct dir_rename_entry *entry; > + struct dir_rename_entry *entry = NULL;; > > - strcpy(temp, path);
I'm sad that this strcpy() wasn't caught in review. IMHO we should avoid that function altogether, even when we _think_ it can't trigger an overflow. That's easier to reason about (and makes auditing easier).
It looks like another one has crept in recently, too.
-- >8 -- Subject: [PATCH] blame: prefer xsnprintf to strcpy for colors
Our color buffers are all COLOR_MAXLEN, which fits the largest possible color. So we can never overflow the buffer by copying an existing color. However, using strcpy() makes it harder to audit the code-base for calls that _are_ problems. We should use something like xsnprintf(), which shows the reader that we expect this never to fail (and provides a run-time assertion if it does, just in case).
Signed-off-by: Jeff King <peff@peff.net> --- Another option would just be color_parse(repeated_meta_color, "cyan"). The run-time cost is slightly higher, but it probably doesn't matter here, and perhaps it's more readable.
This one is less critical for v2.18.
builtin/blame.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/blame.c b/builtin/blame.c index 4202584f97..45770c5a8c 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -1068,7 +1068,9 @@ int cmd_blame(int argc, const char **argv, const char *prefix) find_alignment(&sb, &output_option); if (!*repeated_meta_color && (output_option & OUTPUT_COLOR_LINE)) - strcpy(repeated_meta_color, GIT_COLOR_CYAN); + xsnprintf(repeated_meta_color, + sizeof(repeated_meta_color), + "%s", GIT_COLOR_CYAN); } if (output_option & OUTPUT_ANNOTATE_COMPAT) output_option &= ~(OUTPUT_COLOR_LINE | OUTPUT_SHOW_AGE_WITH_COLOR);
-- 2.18.0.rc1.446.g4486251e51