nerdctl is leaking host files when a container fails to start

[apostasie]

Description

While investigating #2992, I noticed that AllocHostsFile is called before we verify that the requested --name for the container to be started is actually available.

If the name is not free, we bail out, but we leave the allocated host file in place.

Steps to reproduce the issue

# Just repeatedly run containers with the exact same name
sudo ./nerdctl-patch run -d --name whatevername debian bash
sudo ./nerdctl-patch run -d --name whatevername debian bash
sudo ./nerdctl-patch run -d --name whatevername debian bash

Describe the results you received and expected

Now, /var/lib/nerdctl/XXXX/etchosts/default will start filling up with orphaned files.

This is also true for /var/lib/nerdctl/1935db59/containers/default.

Looking at create.go shows why.

What version of nerdctl are you using?

1.7.6

Are you using a variant of nerdctl? (e.g., Rancher Desktop)

None

Host information

No response

[apostasie]

Seems to me like the general issue here is that we just do not clean-up anything if the container fails to create.
Solving this is likely more involved than just shuffling things around.

[haytok]

Hi, @apostasie @AkihiroSuda

The reproduction procedure written by apostasie has been executed on my env.

When a container with the same name already exists and an error occurs, an error returns at the following point.

• https://github.com/containerd/nerdctl/blob/main/pkg/cmd/container/create.go#L281-L283

// Create will create a container.
func Create(ctx context.Context, client *containerd.Client, args []string, netManager containerutil.NetworkOptionsManager, options types.ContainerCreateOptions) (containerd.Container, func(), error) {
...
		if err := containerNameStore.Acquire(options.Name, id); err != nil {
			return nil, nil, err
		}

At the moment no gc function is passed to the second return value (fuc()) of Create().

Therefore, it is assumed that this issue can be solved by specifying a function in the second return value that removes the following.

• /var/lib/nerdctl/1935db59/containers/<namespace>/<container id>
• /var/lib/nerdctl/1935db59/etchosts/<namespace>/<container id>

Can I create a PR with this policy?
Please let me know if there are other considerations to take into account.

[apostasie]

Hey @haytok

Yeah, I would say, just go for it and let's discuss details on your PR.

Generally, I would love to see a solution that takes care of all the places where we error in that function - not just this specific location.

[apostasie]

Therefore, it is assumed that this issue can be solved by specifying a function in the second return value that removes the following.

Be sure to use existing abstractions to do the cleanup (hostsstore.DeallocHostsFile) and do not manipulate the filesystem directly.
The point is: we need to make sure this stays safe wrt concurrency.

[haytok]

Thanks for comments !!!

Generally, I would love to see a solution that takes care of all the places where we error in that function - not just this specific location.

Sure, the second return value of most of this function is nil and does not return a gc, which should also be corrected.

I will create a PR as soon as possible.