enumerate and resolve unhandled heap allocation errors

[wjhun]

There are numerous areas in the kernel where a heap allocation failure is being silently ignored, merely asserted against or otherwise mishandled. Assertions may suffice for allocations on initialization (where system bringup would otherwise be impossible), but allocation failures which may reasonably happen during runtime must be addressed.

See, for example, #1199; all the various calls to these vector functions should now check for success.

This is an open-ended ticket - we can enumerate the cases here which may have many instances.

Review:

• calls to vector_set

• calls to buffer_extend

• calls to extend_total

• calls to allocate, allocate_u64 and allocate_zero

[estherlyoon]

Hi, I'm a student from The University of Texas at Austin. Myself and my group are looking to contribute to Nanos as part of a class we're taking on virtualization. Could we take on this issue?

[wjhun]

Hello Esther. Sorry for the delay in response, and thank you for offering to contribute to the project. Yes, we could use help in cleaning up some of our error checking. I suggest prioritizing the calls where an error return path already exists, either via function return (e.g. boolean pass/fail or error code) or a status handler (e.g. applying an error tuple created with "timm" to a completion).

I wouldn't worry about trying to propagate errors up to callers if there isn't a path to do so and there isn't a straightforward way to add one. An assertion would be better than no check at all.

[estherlyoon]

Thanks, Will! No worries. We'll get started on this and let you know if we have additional questions.

[estherlyoon]

Hi @wjhun, sorry for the delay, but I started working on this issue and have a few questions.

When error checking vector_set, should I also deallocate_uint64 as is done in the linked example #1199 ? Or is either asserting or returning the boolean sufficient?

Additionally, when error checking allocate, should I always be checking if the allocated object pointer = INVALID_PHYSICAL for allocate_u64, and INVALID_ADDRESS for allocate and allocate_zero with an assert? This is what other areas of the code seem to do.

Thanks!

[wjhun]

@estherlyoon

When error checking vector_set, should I also deallocate_uint64 as is done in the linked example #1199 ? Or is either asserting or returning the boolean sufficient?

In that particular case, the deallocate is reverting the preceding fd allocation due to the error. On a case by case basis, you would also want to similarly unwind any state that was changed within the failing procedure. But, if you are not sure what to do, an assert or passing an error upstream (or to a completion) is preferable to doing nothing.

Additionally, when error checking allocate, should I always be checking if the allocated object pointer = INVALID_PHYSICAL for allocate_u64, and INVALID_ADDRESS for allocate and allocate_zero with an assert? This is what other areas of the code seem to do.

Yes, that's the right way to check for allocation errors. Whether or not to assert depends on the case; if it's a recoverable failure or an error can be return or thrown in some way, then it's better to do so rather than assert. But many allocation failures (particularly on initialization) aren't really recoverable and could just be asserted without any path to recovery.

Again, when in doubt, an assert would still give more useful context than a page fault or other undesirable behavior stemming from a bad pointer propagating downstream.