Fix segfault in host-ctr

[larvacea]

Bottlerocket invokes the host-ctr executable with an optional "--registry-config" parameter. If this parameter was omitted (for instance, in customer code), host-ctr would segfault. Now it will behave as it would for an empty registry-config file.

Issue number:

Closes #4059

Description of changes:

Protect two locations where we tried to dereference the registryConfig pointer. The pointer is nil if host-ctr is invoked
without the optional --registry-config parameter.

Testing done:

Built an image and demonstrated that the command in issue #4059 could load a container from a public ECR repository without segfaulting.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

[larvacea]

The linter complains about seven locations with deprecated function calls. Six are calls to log.L or log.G, and one to a deprecated method in containerd.

[yeazelm]

Isn't the right behavior here to just return the defaultResolver if registryConfig is nil?

[larvacea]

This case branch is "this is the public ECR," and (if possible) the code here is trying to get an auth token for the public ECR. So this if statement at the head of the case branch says "but hey, if registryConfig has something to say about the public ECR, well, never mind, we will go with what registryConfig says." Thus if registryConfig is nil, I am treating it as registryConfig does not have anything to say, and we should attempt to get the token.

[larvacea]

Perhaps a comment is in order. Assuming that my version of "what this thing does" is correct.

[yeazelm]

Ah that makes sense. The diff didn't help here because the top of the function is what creates this defaultResolver. After reading more of the context I see how this is the right way to solve this.

[yeazelm]

Haven't you created a new problem that endpoints can now be unset? I think we probably want the else clause to be for the first if statement:

Suggested change

	if registryConfig != nil {
	if _, ok := registryConfig.Mirrors[host]; ok {
	endpoints = registryConfig.Mirrors[host].Endpoints
	} else {
	endpoints = registryConfig.Mirrors["*"].Endpoints
	}
	if registryConfig != nil {
	if _, ok := registryConfig.Mirrors[host]; ok {
	endpoints = registryConfig.Mirrors[host].Endpoints
	}
	} else {
	endpoints = registryConfig.Mirrors["*"].Endpoints

[larvacea]

Likewise, another good candidate for a comment. We start with an empty array of endpoints. Then we may replace that empty array with a non-empty one if registryConfig has an override. Then we append docker.defaultHost(host) to the array. The endpoints array always has at least that one docker.defaultHost() entry.

[larvacea]

I have added comments, hoping to make the code a little less opaque.

[yeazelm]

I think I got myself confused a bit reviewing this code so I went digging, I wonder if the right fix is to ensure https://github.com/bottlerocket-os/bottlerocket-core-kit/blob/develop/sources/host-ctr/cmd/host-ctr/main.go#L1025 doesn't proceed as an uninitialized variable if registryConfigPath is empty. This seems like a validation of arguments issue. I think the defensive if statements you have added make sense, but we should also fix this issue of creating a variable and only creating the right struct if a different variable. Can we look at creating an empty registryConfig one isn't passed? This would allow all the passing of this variable downstream to be safer.

[larvacea]

Yes, I think it makes sense to build an empty registryConfig if --registry-config is missing. This is the net effect of what we do today (almost always). We supply --registry-config /etc/host-containers/host-ctr.toml in both systemd units where we call host-ctr. The file ends up being a single blank line (at least on a default aws-k8s instance).

[larvacea]

Updated: added a NewBlankRegistryConfig method and use it when we have an empty registry config file path. With this change, registryConfig will not be nil when the caller omits --registry-config. The locations where we were dereferencing nil are still guarded.

Tested manually using the same host-ctr run command supplied by our customer in the original GitHub issue.

[yeazelm]

Ah that makes sense. The diff didn't help here because the top of the function is what creates this defaultResolver. After reading more of the context I see how this is the right way to solve this.