check sandbox pointers in virtcontainers internal functions

[bergwolf]

From @jodh-intel:

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

IMHO, to make such a change, we need to clarify a few things:

1. What is the guideline for internal function parameter sanity check? Do we want to check sandbox pointers in every function?
2. What to do about other parameters? Do we want to make sandbox pointers a special case?

@jodh-intel @sboeuf @egernst please comment.

[sboeuf]

Thanks for opening this, I have assigned P1 priority as it might be good to address it asap.

[bergwolf]

@sboeuf Why is it a P1 issue? AFAICT, it is not a bug and does not block any functionality. I am not even convinced we should do it. A P1 issue means it must be fixed ASAP but I see otherwise on this one.

And please comment on the two things I listed above so that we can get consensus on internal function parameter sanity checking before introducing random checks here and there.

[sboeuf]

I have assigned P1 to make sure we don't forget it, otherwise, I have the feeling that nobody will ever look at it if it is P2 or P3...

1. What is the guideline for internal function parameter sanity check? Do we want to check sandbox pointers in every function?

We have two options, either we check this for every single function (which seems a bit overkill), or we define what we consider as high level functions where we should check this.

2. What to do about other parameters? Do we want to make sandbox pointers a special case?

I think we might need to do the same for other parameters, but this should be handled in a different issue then.

[egernst]

I don't think this is a P1, as there really isn't an severity, but agree we need to come to agreement on this. Let's get feedback from rest of the committee on this if we can't get quick alignment between @jodh-intel and @bergwolf

I think that for the two items you made, Peng, we should have the same guidance. For example, we should be consistent on parameter checking; sandbox shouldn't be a special case. Is there a reason you think it would be? Because it comes from a well defined source? Or?

I think that doing parameter checking at the API level makes sense 100% for sure, and agree about the ROI of doing the check as it gets passed down through multiple function calls. IF we have test coverage for each of the functions, in the chain already, I'm not sure if it is necessary.

Having said that, I don't think this is a top priority/release gating item and we have a pretty big backlog right now for 1.0. This is definitely something we should get agreement on so we can be consistent on implementation and review (the most important thing here, imo).

[grahamwhaley]

You may not get feedback from @jodh-intel this week. I strongly suspect he'd vote for checking all interface parameters all the time, at all levels... but, let's wait for his input on that.

Personally, I do think that whilst that leads to better/safer software, it is possibly overkill from the performance pov. I'd vote we check all parameters at all boundaries (API and module interfaces etc.) at a minimum (but I think we all agree on that already).

[bergwolf]

@egernst I asked about sandbox pointer specifically because I was asked to check it but not other parameters. I think we should not make it a special case but need to confirm with you guys.

I guess we all likely agree on checking all parameters at API and module boundaries. Let's wait @jodh-intel's input when he comes back.