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
