Re: [PATCH 24/30] merge-recursive: Add computation of collisions due to dir rename & merging

[Elijah Newren <newren@gmail.com>]

On Sun, Jun 10, 2018 at 3:56 AM, René Scharfe <l.s.r@web.de> 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 <l.s.r@web.de>
> ---
>  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 <newren@gmail.com>