Worktree creation race
Hello, I am writing to report a bug. The original report is from my colleague, I am also providing his suggestions. There is insufficient locking for worktree addition. Adding worktree may fail. The problem is that git reads the directory entries in $GIT_DIR/worktrees, finds a worktree name that does not exist, tries to create it, and if an error is returned adding the worktree fails. When multiple git processes do this in parallel only one adds a worktree and the others fail. Git should reread the directory and find a new name that does not exist when creating the worktree directory fails because another git process already created it. I suppose adding PID in the tree name would mitigate the issue to the point it will be very unlikely to encounter. I need more than the tree in the temporary directory so using the temporary directory directly as a tree is out of question. to test: cd /dev/shm mkdir gittest cd gittest git init gitrepo cd gitrepo git commit --allow-empty -m Empty for n in $(seq 1) ; do ( tmp=$(mktemp -d /dev/shm/gittest/test.XXX) ; mkdir $tmp/test ; git worktree add --detach $tmp/test ; ) & done (you should see many messages like: fatal: could not create directory of '.git/worktrees/test284': File exists) Greetings, Marketa signature.asc Description: OpenPGP digital signature
Re: Git bomb still present (at least in SUSE)?
Hi, Jonathan Nieder wrote: > Jeff King wrote: >> On Tue, Jan 15, 2019 at 02:35:29PM +0100, Marketa Calabkova wrote: >>> meggy@irbis:/tmp/test> /usr/bin/time git clone >>> https://github.com/Katee/git-bomb.git >>> Cloning into 'git-bomb'... >>> remote: Enumerating objects: 18, done. >>> remote: Total 18 (delta 0), reused 0 (delta 0), pack-reused 18 >>> Unpacking objects: 100% (18/18), done. >>> ^Cwarning: Clone succeeded, but checkout failed. > [...] >> git clone --bare https://github.com/Katee/git-bomb.git >> cd git-bomb.git >> git read-tree HEAD ;# yikes! >> >> So I don't think there's a bug per se. It is possible that Git could >> have better protections against maliciously gigantic repositories, but I >> don't think anybody is actively working on such a feature (and it would >> involve much more than this case; it's pretty easy to generate trees >> with pessimal diffs, etc). Thanks for your answer, now I finally understand what the fix is about. > One thing I think interested people could do is introduce some kind of > "limits" subsystem into Git, so that a person could configure Git to > refuse to even try when it notices that an operation is going to be > sufficiently expensive. I.e. something similar to what rlimits (or > other limits e.g. enforced in cgroups) provide in an operating system. > > That said, as alluded to in the last paragraph, there's also some > protection possible at the operating system level. > > So my feeling is that there's some real potential for improvement > here, and I'm happy to help mentor anyone working on it if it is their > itch (because of the "can handle at another level" thing, it is not > mine). Thank you, I am interested. Mostly for educational purpose, I have to say, I would like to contribute on such a big project as git is. I would be happy if you help me with that. How can git know how big the repository is before it tries to checkout? Or do you think it is OK to start the operation, notice it is already too expensive and kill it? And, well, how to easily track how expensive the operation is (e. g. not in the particular operation)? Greetings, Marketa signature.asc Description: OpenPGP digital signature
Re: Worktree creation race
Hi, you have probably overseen my email :) . Please, I would like to get it fixed. Does anyone has a suggestion what to do with this bug? It looks like a one-line fix probably in builtin/worktree.c, but I have no idea how to do it. Sorry. Thank you, Marketa On 15/01/2019 15:03, Marketa Calabkova wrote: > Hello, > > I am writing to report a bug. The original report is from my colleague, I am > also providing his suggestions. > > There is insufficient locking for worktree addition. Adding worktree may fail. > > The problem is that git reads the directory entries in $GIT_DIR/worktrees, > finds a worktree name that does not exist, tries to create it, and if an > error is returned adding the worktree fails. When multiple git processes > do this in parallel only one adds a worktree and the others fail. Git should > reread the directory and find a new name that does not exist when creating > the worktree directory fails because another git process already created it. > > I suppose adding PID in the tree name would mitigate the issue to the point > it will be very unlikely to encounter. > > I need more than the tree in the temporary directory so using the temporary > directory directly as a tree is out of question. > > to test: > > cd /dev/shm > mkdir gittest > cd gittest > git init gitrepo > cd gitrepo > git commit --allow-empty -m Empty > for n in $(seq 1) ; do ( tmp=$(mktemp -d > /dev/shm/gittest/test.XXX) ; mkdir $tmp/test ; git worktree add > --detach $tmp/test ; ) & done > > (you should see many messages like: > fatal: could not create directory of '.git/worktrees/test284': File exists) > > Greetings, > Marketa > > signature.asc Description: OpenPGP digital signature
