Re: [ANNOUNCE] Git v2.2.1 (and updates to older maintenance tracks)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Max,

On Fri, 19 Dec 2014, Max Kirillov wrote:

> Hello. Thank you for the fix.

You are welcome.

> Would it be more reliable to compare inode of directory in question and
> ".git"? (there is [*] for windows). So that any unspotted name
> equivalence is prevented to cause any harm.
> 
> *) http://stackoverflow.com/questions/7162164/does-windows-have-inode-numbers-like-linux

We were considering this, really. The biggest problem we had with this
approach is that the most important aspect of releasing a new version was
to let hosting sites protect their users via receive.fsckObjects = true
(which all hosting sites should activate anyway) by rejecting the
ill-intentioned file names in hand-crafted tree objects.

This turned out to be much more important than the user-facing aspect:
many users will not update (in many cases, users will be prevented by
administrators from updating their software themselves, even).

Therefore, we really tried very hard to get the checks right without
having to write out non-bare repositories on Windows (or for that matter,
on MacOSX).

Having said that, I would welcome additional safeguards on the users'
side, such as what you suggested; I had written out the code in a mail
thread between the Git security squad (in addition to nFileIndexHigh and
nFileIndexLow, we need to check dwVolumeSerialNumber, too), maybe you want
to take this further?

-- snip --
Untested code snippet to illustrate what I have in mind:

	static int is_git_directory(const char *path) {
		static int initialized;
		static DWORD dwVolumeSerialNumber, nFileSizeHigh, nFileSizeLow;

		HANDLE handle;
		BY_HANDLE_FILE_INFORMATION info;
		int result = 0;

		if (!initialized) {
			handle = CreateFile(get_git_dir(), 0,
				FILE_SHARE_READ, 0, OPEN_EXISTING,
				FILE_ATTRIBUTE_NORMAL);

			if (handle != INVALID_HANDLE_VALUE) {
				if (GetFileInformationByHandle(handle, &info)) {
					dwVolumeSerialNumber =
						info.dwVolumeSerialNumber;
					nFileSizeHigh = info.nFileSizeHigh;
					nFileSizeLow = info.nFileSizeLow;
				}
				CloseHandle(handle);
			}
			initialized = 1;
		}

		handle = CreateFile(path, 0, FILE_SHARE_READ, 0, OPEN_EXISTING,
			FILE_ATTRIBUTE_NORMAL);

		if (handle != INVALID_HANDLE_VALUE) {
			if (GetFileInformationByHandle(handle, &info)) {
				result = dwVolumeSerialNumber ==
					info.dwVolumeSerialNumber &&
					nFileSizeHigh == info.nFileSizeHigh &&
					nFileSizeLow == info.nFileSizeLow;
			}
			CloseHandle(handle);
		}
		return result;
	}

	static inline int is_in_git_directory(const char *path) {
		char *copy = xstrdup(path);
		while (PathRemoveFileSpec(path)) {
			if (is_git_directory(path)) {
				free(copy);
				return 1;
			}
		}
		free(copy);
		return 0;
	}

This *should* be good enough.

I fear that this will have a performance impact that might be alleviated
by caching results.

What do you think?
-- snap --

Please note that this code has been written inside a mail program and has
never seen a compiler.

This function – `is_in_git_directory()` – would be called at the start of
`verify_path()`, protected by an `if (protect_ntfs)` guard, and of course
we would need to have a POSIX version using inodes, too, because – believe
it or not – there are Linux users using Git on NTFS volumes.

The problems I see with this are:

1) it really might affect performance very negatively, as it has to
   perform recursive checks for *all* the paths about to be checked out,

2) we would really need to make sure that the function cannot be called
   *before* initializing the .git/ directory in `git init` and `git clone`,
   and

3) it would not protect submodules' .git/ directories (before we switched
   to writing out .git files redirecting to the super project's .git/
   directory, Git initialized .git/ directories inside the submodules'
   working directories).

Ciao,
Johannes

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]