Re: [PATCH 24/30] merge-recursive: Add computation of collisions due to dir rename & merging
René Scharfe writes:
> 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:
>
> -- >8 --
> Subject: [PATCH] merge-recursive: use xstrdup() instead of fixed buffer
>
> Paths can be longer than PATH_MAX. Avoid a buffer overrun in
> check_dir_renamed() by using xstrdup() to make a private copy safely.
>
> Signed-off-by: Rene Scharfe
> ---
Thanks. Makes sense.
> merge-recursive.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index ac27abbd4c..db708176c5 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2211,18 +2211,18 @@ static struct hashmap *get_directory_renames(struct
> diff_queue_struct *pairs,
> static struct dir_rename_entry *check_dir_renamed(const char *path,
> struct hashmap *dir_renames)
> {
> - char temp[PATH_MAX];
> + char *temp = xstrdup(path);
> char *end;
> - struct dir_rename_entry *entry;
> + struct dir_rename_entry *entry = NULL;;
>
> - strcpy(temp, path);
> while ((end = strrchr(temp, '/'))) {
> *end = '\0';
> entry = dir_rename_find_entry(dir_renames, temp);
> if (entry)
> - return entry;
> + break;
> }
> - return NULL;
> + free(temp);
> + return entry;
> }
>
> static void compute_collisions(struct hashmap *collisions,
Re: [PATCH 24/30] merge-recursive: Add computation of collisions due to dir rename & merging
On Sun, Jun 10, 2018 at 3:56 AM, René Scharfe wrote:
> Am 10.11.2017 um 20:05 schrieb Elijah Newren:
>> +static struct dir_rename_entry *check_dir_renamed(const char *path,
>> + struct hashmap *dir_renames)
>> {
>> + char temp[PATH_MAX];
>> + char *end;
>> + struct dir_rename_entry *entry;
>> +
>> + strcpy(temp, path);
>> + while ((end = strrchr(temp, '/'))) {
>> + *end = '\0';
>> + entry = dir_rename_find_entry(dir_renames, temp);
>> + if (entry)
>> + return entry;
>> + }
>> + return NULL;
>> +}
>
> 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:
Thanks for the pointers, and for providing a fix.
> -- >8 --
> Subject: [PATCH] merge-recursive: use xstrdup() instead of fixed buffer
>
> Paths can be longer than PATH_MAX. Avoid a buffer overrun in
> check_dir_renamed() by using xstrdup() to make a private copy safely.
>
> Signed-off-by: Rene Scharfe
> ---
> merge-recursive.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index ac27abbd4c..db708176c5 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2211,18 +2211,18 @@ static struct hashmap *get_directory_renames(struct
> diff_queue_struct *pairs,
> static struct dir_rename_entry *check_dir_renamed(const char *path,
> struct hashmap *dir_renames)
> {
> - char temp[PATH_MAX];
> + char *temp = xstrdup(path);
> char *end;
> - struct dir_rename_entry *entry;
> + struct dir_rename_entry *entry = NULL;;
>
> - strcpy(temp, path);
> while ((end = strrchr(temp, '/'))) {
> *end = '\0';
> entry = dir_rename_find_entry(dir_renames, temp);
> if (entry)
> - return entry;
> + break;
> }
> - return NULL;
> + free(temp);
> + return entry;
> }
>
> static void compute_collisions(struct hashmap *collisions,
> --
> 2.17.1
Reviewed-by: Elijah Newren
Re: [PATCH 24/30] merge-recursive: Add computation of collisions due to dir rename & merging
On Sun, Jun 10, 2018 at 12:56:31PM +0200, René Scharfe wrote: > 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. > - 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 --- 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
Re: [PATCH 24/30] merge-recursive: Add computation of collisions due to dir rename & merging
Am 10.06.2018 um 12:56 schrieb René Scharfe:
> Am 10.11.2017 um 20:05 schrieb Elijah Newren:
>> +static struct dir_rename_entry *check_dir_renamed(const char *path,
>> + struct hashmap *dir_renames) {
>> +char temp[PATH_MAX];
>> +char *end;
>> +struct dir_rename_entry *entry;
>> +
>> +strcpy(temp, path);
>> +while ((end = strrchr(temp, '/'))) {
>> +*end = '\0';
>> +entry = dir_rename_find_entry(dir_renames, temp);
>> +if (entry)
>> +return entry;
>> +}
>> +return NULL;
>> +}
>
> 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:
Argh, I meant to reply to v10 of that patch, i.e. this:
https://public-inbox.org/git/20180419175823.7946-21-new...@gmail.com/
The cited code wasn't changed and is in current master, though, so both
that part and my patch are still relevant.
René
Re: [PATCH 24/30] merge-recursive: Add computation of collisions due to dir rename & merging
Am 10.11.2017 um 20:05 schrieb Elijah Newren:
> +static struct dir_rename_entry *check_dir_renamed(const char *path,
> + struct hashmap *dir_renames) {
> + char temp[PATH_MAX];
> + char *end;
> + struct dir_rename_entry *entry;
> +
> + strcpy(temp, path);
> + while ((end = strrchr(temp, '/'))) {
> + *end = '\0';
> + entry = dir_rename_find_entry(dir_renames, temp);
> + if (entry)
> + return entry;
> + }
> + return NULL;
> +}
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:
-- >8 --
Subject: [PATCH] merge-recursive: use xstrdup() instead of fixed buffer
Paths can be longer than PATH_MAX. Avoid a buffer overrun in
check_dir_renamed() by using xstrdup() to make a private copy safely.
Signed-off-by: Rene Scharfe
---
merge-recursive.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index ac27abbd4c..db708176c5 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2211,18 +2211,18 @@ static struct hashmap *get_directory_renames(struct
diff_queue_struct *pairs,
static struct dir_rename_entry *check_dir_renamed(const char *path,
struct hashmap *dir_renames)
{
- char temp[PATH_MAX];
+ char *temp = xstrdup(path);
char *end;
- struct dir_rename_entry *entry;
+ struct dir_rename_entry *entry = NULL;;
- strcpy(temp, path);
while ((end = strrchr(temp, '/'))) {
*end = '\0';
entry = dir_rename_find_entry(dir_renames, temp);
if (entry)
- return entry;
+ break;
}
- return NULL;
+ free(temp);
+ return entry;
}
static void compute_collisions(struct hashmap *collisions,
--
2.17.1
