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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 16, 2011, 08:59 UTC
Message-ID: <20111116085944.GA18781@elie.hsd1.il.comcast.net>
In-Reply-To: <CACsJy8Di3ZrPdXh1Jf=PbLYRWwx-TEV78NzUukwaxA0xW=rSNg@mail.gmail.com>

Nguyen Thai Ngoc Duy wrote:

> Or perhaps

[...]

Show 5 quoted lines

>  - 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, unless they happen to sit in the middle of an unsafe

	const char *filename = git_path(foo);
	int fd;

	call_a_function_i_dont_control();
	fd = open(filename, O_CREAT|O_WRONLY|O_TRUNC, 0600);

sequence. Lacks that feeling of truth in advertising. And on the other hand that this doesn't help with thread-safety at all.

I think if I ran the world, the fundamental operation would be strbuf_addpath(). Unlike git_pathdup(), this lets callers avoid some allocation churn if they are in the middle of a loop.