core: do not crash setting the log level before the process table exists#4121
Open
Lt-Flash wants to merge 1 commit into
Open
core: do not crash setting the log level before the process table exists#4121Lt-Flash wants to merge 1 commit into
Lt-Flash wants to merge 1 commit into
Conversation
set_proc_log_level() and reset_proc_log_level() segfault when called from a module's mod_init(). init_modules() runs several steps before init_multi_proc_support(), so pt is still NULL, and __set_proc_log_level() writes pt[proc_idx].log_level unconditionally. reset_proc_log_level() fails the same way one level down: default_log_level is a static pointer that init_log_level() has not filled in yet, so it dereferences NULL. This is easy to hit. Temporarily lowering the log level around a call that is expected to fail is the natural way for a module to keep an expected error out of its startup output, and the API gives no hint that it is unusable at the point most modules would reach for it. Guards each of the affected setters. Where the intent can still be honoured it is, rather than silently dropping the request: log_level already points at the static holder before init_log_level() runs, and there is only one process at that stage, so __set_proc_log_level() sets the level through it and reset_proc_log_level() restores the value logging started with. __set_proc_default_log_level() updates that same holder. set_global_log_level() additionally guards log_level_global, which init_log_level() allocates - before that, counted_max_processes is 0 so its loop is a no-op and the shared value does not exist yet, so it applies the level to the current process instead. suppress_proc_log_event() and reset_proc_log_event() return early: the event consumer they guard is not running yet either. Verified by calling set_proc_log_level() followed by reset_proc_log_level() from a module mod_init(): the same module segfaults on an unpatched core and starts normally on a patched one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
set_proc_log_level()andreset_proc_log_level()segfault when called from a module'smod_init().Root cause
init_modules()runs several steps beforeinit_multi_proc_support(), so the process table is still NULL while every module'smod_init()executes — and the setter writes through it unconditionally:reset_proc_log_level()fails the same way one level down:default_log_levelis a static pointer thatinit_log_level()has not filled in yet, so it dereferences NULL.Why it is worth guarding
Temporarily lowering the log level around a call that is expected to fail is the natural way for a module to keep a benign startup error out of its output — and the API gives no hint that it is unusable at exactly the point most modules would reach for it. The failure is a hard crash at startup, not a diagnostic.
The fix
Guards each affected setter. Where the intent can still be honoured it is, rather than silently dropping the request:
__set_proc_log_level()—log_levelalready points at the static holder beforeinit_log_level()runs, and there is only one process at that stage, so the level is applied through it.reset_proc_log_level()— restores the value logging started with (log_level_holder).__set_proc_default_log_level()— updates that same holder.set_global_log_level()— additionally guardslog_level_global, whichinit_log_level()allocates. Before that,counted_max_processesis 0 so its loop is a no-op and the shared value does not exist, so it applies the level to the current process instead.suppress_proc_log_event()/reset_proc_log_event()— return early; the event consumer they guard is not running yet either.No behaviour changes once the process table exists — every guard is a NULL check on state that is non-NULL from
init_multi_proc_support()onward.Testing
A/B tested with a module calling
set_proc_log_level()followed byreset_proc_log_level()from itsmod_init(): the same module segfaults on an unpatched core and starts normally on a patched one.