mheily / jobd

A job management framework
Other
143 stars 15 forks source link

libnv doesn't check malloc()'s return value #82

Closed Crest closed 8 years ago

Crest commented 8 years ago

Libnv wraps memory allocation with macros to work both inside the FreeBSD kernel and in userspace. In the kernel it uses WAITOK which guarantees a successful memory allocation on return from malloc(), but in userspace malloc() can fail without taking down the whole system. As such an anything replacing init has to deal with memory allocation failure in a sane way. Just crashing is unacceptable behaviour for a nested process supervisor as some subtrees might run with resource limits which in turn make it far more probable get a NULL pointer from malloc().

mheily commented 8 years ago

Interesting info, and a fair point about how jobd crashing should not cause the entire system to go down. I am planning to support restarting jobd after a crash, and having it resume supervision of existing process trees. How this would work if the system is OOM entirely, that is another question.

mheily commented 8 years ago

85 describes a supervisor/repear layer between jobd and the jobs that should help.

As to checking malloc() return for NULL, that's not going to happen. The default behavior for most systems is to overcommit memory, so malloc() never returns NULL. It's better to mark jobd as immune from the OOM killer, and make sure that it doesn't leak memory.

Crest commented 8 years ago

I just looked at the libnv code in FreeBSD 11.0 ALPHA4. The code now checks the return value of all nv_malloc() calls I found with a quick grep -R nv_malloc. By default FreeBSD doesn't overcommit memory. Check yourself with sysctl vm.overcommit. IIRC overcommitting has been disabled by default in FreeBSD since FreeBSD 8.0.

You can use procctl(2) to protect jobd and its supervisors from the OOM killer.

In FreeBSD 11 pdfork(2) + kevent(2) with EVFILT_PROCDESC are probably better suited to monitoring direct child processes from a unified event loop than signal handling. From the looks of it a dedicated reaper process is still required to supervise indirect child processes e.g. double forking daemons.