Recursive pinning can lose existing direct pin

[ghost]

Version information: 0.4.14-dev

Type: bug

Description:

This bit looks like it'll lose the existing direct pin if the attempt to add the recursive pin fails. The direct pin will have been removed, and the expected recursive pin won't have succeeded: https://github.com/ipfs/go-ipfs/blob/864c960aa641bff2820b59b5eeb9d93be5d0998d/pin/pin.go#L213-L228

directPin.Remove() should probably happen after recursePin.Add()

[ghost]

And we probably want to guard against blocks being removed between FetchGraph() and recursePin.Add(), however unlikely it is to occur.

[schomatis]

@lgierth Is someone working on this? I'd like to make a try, it will help me to learn more about the pinning system.

[ghost]

@schomatis thanks, please go for it :)

[Kubuxu]

@lgierth pinner is locked during this time and any access to pins does require locking (or at least it should, not true about: https://github.com/ipfs/go-ipfs/blob/864c960aa641bff2820b59b5eeb9d93be5d0998d/pin/pin.go#L491-L498), so there should not be any inconsistency.

The fact that those two functions do not require locking will lead to concurrent map modification/read panic and should be fixed.

[schomatis]

@lgierth

directPin.Remove() should probably happen after recursePin.Add()

I agree. Maybe a function could be added to add recursive pins (e.g., addRecursePin) that would abstract the requirement that a pin can only be of only one type (e.g., direct or recursive) performing the atomic check p.directPin.Has(c) inside the function.

And we probably want to guard against blocks being removed between FetchGraph() and recursePin.Add(), however unlikely it is to occur.

As @Kubuxu commented the Pin function is protected by a lock and, for example, a direct attempt to remove the block being pinned with ipfs block rm will check for pins and block when it tries to call CheckIfPinned. However, if the garbage collection is run while adding the pin (ipfs repo gc), as RecursiveKeys is not protected, there is a chance that the block is removed before it is pinned (i.e., added to recursePin).

So both DirectKeys and RecursiveKeys (and probably any other exported function that manipulates the pin sets) should be locked to prevent similar issues (although they may be quite rare). Locking those getter pin functions may have some performance impact over other parts of the code that will stall while the pinner is working (from what I'm seeing adding a recursive pin is an expensive operation).

[schomatis]

Looking more carefully into the issue I have to correct myself, ipfs pin add locks the GC so the above mentioned race condition should not happen, I still think DirectKeys and RecursiveKeys should be locked, as there may be other code paths that could lead to a similar situation.