Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Nov 16, 2011, 09:31 UTC
Message-ID: <CACsJy8CYj_s92zG-LnBKtHxV2uaG8-rq-VNJiQYwNJXGKbFeDw@mail.gmail.com>
In-Reply-To: <20111116085944.GA18781@elie.hsd1.il.comcast.net>

On Wed, Nov 16, 2011 at 3:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 12 quoted lines

> Nguyen Thai Ngoc Duy wrote:
>
>> Or perhaps
> [...]
>>  - git_path(const char *path) maintains a small hash table to keep
>> track of all returned strings based with "path" as key.
>>
>> Out of 142 git_path() calls in my tree, 97 of them are in form
>> git_path("some static string").
>
> The main bit I dislike about patch 3/3 is that constructs like
> 'unlink(git_path("MERGE_HEAD"));' are not actually unsafe

Well, we can create wrappers (e.g. repo_unlink(const char *) that calls git_path internally). According to grep/sed these functions are used in form xxx(git_path(xxx))

     16 unlink
      8 file_exists
      7 stat
      6 fopen
      5 rename
      5 open
      4 unlink_or_warn
      3 safe_create_dir
      3 adjust_shared_perm
      3 access
      2 xstrdup
      2 safe_create_leading_directories
      2 rmdir
      2 remove_empty_directories
      2 opendir
      1 unable_to_lock_error
      1 read_attr_from_file
      1 mkdir
      1 lstat
      1 launch_editor
      1 add_excludes_from_file_to_list

By creating wrappers for unlink, file_exists, stat, fopen, rename and open we can safely avoid git_pathdup()/free() in 42 places.

-- 
Duy