sibling() is copying a sync.Mutex

[ktarplee]

The copylocks linter (via golangci-lint) pointed out that the code below, in repository.go makes a copy of the sync.Mutex in Repository.

func (s *blobStore) sibling(otherRepoName string) *blobStore {
	otherRepo := *s.repo
	otherRepo.Reference.Repository = otherRepoName
	return &blobStore{
		repo: &otherRepo,
	}
}

The first line in this function make a copy of Repository and the Repository struct contains a field with type sync.Mutex so it is copied as well.

The documentation for sync.Mutex states clearly that making a copy after first use is not allowed so this does seem like a bug in ORAS.

It seems like this is an easy fix for someone who knows what the intent of sibling() is in the code base.

[Wwwsylvia]

@ktarplee Thanks for reporting this bug!

Just to share some context:
The sibling() function is a private function invoked by Repository.Mount at here.
The referrersPingLock mutex being accidentally copied is used on manifest push and manifest delete.
The copied repository is used for fetching blob content, so it does not actually use the copied mutex.

But this is still a coding bug. @ktarplee Would you be willing to contribute a fix? If not, me or someone else in the community can fix it.

[ktarplee]

It looks like referrersMergePool (member of Repository) also contains a sync.Mutex (via the syncutil.Pool) that would be copied by sibling(). The linter is not catching that one likely due to not knowing about generics well enough yet.

I would be happy to fix it but I am not 100% clear on the approach to take. I am just now porting code to use ORAS so I am no expert in how ORAS works yet. It seems me that you would want to preserve the ability to copy (or at least clone) Respository. So in that case referrersPingLock and referrersMergePool need to be pointers. I think that breaks the way you do initialization (lazy defaults) so it gets a little tricky. Making them pointers with lazy initialization introduces a race condition so that won't work. So we need to create the mutex in NewRepository() and then maybe provide a Clone() method to use sibling(). Does that sound like a reasonable approach. It is OK if you tell me I am completely off base. @Wwwsylvia

[Wwwsylvia]

It looks like referrersMergePool (member of Repository) also contains a sync.Mutex (via the syncutil.Pool) that would be copied by sibling(). The linter is not catching that one likely due to not knowing about generics well enough yet.

I would be happy to fix it but I am not 100% clear on the approach to take. I am just now porting code to use ORAS so I am no expert in how ORAS works yet. It seems me that you would want to preserve the ability to copy (or at least clone) Respository. So in that case referrersPingLock and referrersMergePool need to be pointers. I think that breaks the way you do initialization (lazy defaults) so it gets a little tricky. Making them pointers with lazy initialization introduces a race condition so that won't work. So we need to create the mutex in NewRepository() and then maybe provide a Clone() method to use sibling(). Does that sound like a reasonable approach. It is OK if you tell me I am completely off base. @Wwwsylvia

Yeah I think we should provide a method to clone the Repository struct. It could be similar to an old PR resolving a similar issue: #364.

[ktarplee]

I'll work on this.

[Wwwsylvia]

@ktarplee Thank you! I've assigned this to you.