Don't we now need check on all these params now they are pointers?:

var errNeedSandbox = errors.New("Sandbox cannot be empty")

    :

func (p *ccProxy) start(sandbox *Sandbox, params proxyParams) (int, string, error) {
    if sandbox == nil {
        return -1, "", errNeedSandbox
    }

    :
}

These are all internal functions. I think we should trust internal callers because otherwise we do not trust ourselves. That said, even if we do not trust internal callers, the check should be added as a separate PR since it is an existing issue for places where sandbox is already passed as pointer.

Call me paranoid, but I'd honestly take the opposite approach and not trust anything! 😄

imho we need to "fail fast" since if we start trusting ourselves, we get into horrible situations where func1 calls func2 calls funcN, and funcN explodes because func1 forgot to check if sandbox == nil. That makes debugging very difficult. This is not theoretical - we've been in that situation before in the very early days of vc which prompted me to raise PRs such as containers/virtcontainers#163.

We also have precedent for checking all parameters, for example:

• https://github.com/kata-containers/runtime/blob/master/virtcontainers/container.go#L480..L483

How about adding such checks on a new commit on this PR to ensure those checks get applied and the issue doesn't just get lost in the backlog?

@jodh-intel Do you want to check every parameter in every function? Since that's what not trust anything means to me. From my point of view, I think we should check all parameters in all of the virtcontainers APIs but that's all. We are already doing this and it ensures that we fail fast.

OTOH, if we do not trust ourselves and check parameter in every function, we waste CPU time and memory on checking the same thing over and over again. Please create an issue for it if you think that's really necessary. I don't mind having such a discussion but it does not need to block this PR since it's a different/existing issue.

A few examples of where *Sandbox is not checked before being dereferenced in master:

• https://github.com/kata-containers/runtime/blob/master/virtcontainers/kata_agent.go#L132
• https://github.com/kata-containers/runtime/blob/master/virtcontainers/kata_agent.go#L183
• https://github.com/kata-containers/runtime/blob/master/virtcontainers/shim.go#L175
• https://github.com/kata-containers/runtime/blob/master/virtcontainers/container.go#L404

There a more of them for sure. And if we want to take precaution in all these functions, it should also apply to other parameters as well, not just the sandbox pointers.