Skip to content

Commit

Permalink
A couple of fixes:
Browse files Browse the repository at this point in the history
* Properly remove a module from context->modules map as soon as module_deregister is call
* Avoid sending a MODULE_STOPPED sys message with a sender that points to module internal reference when a module is getting deregistered.
Module would be soon freed thus leading to a garbage pointer.
  • Loading branch information
FedeDP committed Aug 7, 2019
1 parent b64f368 commit 973e426
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 6 deletions.
24 changes: 20 additions & 4 deletions Lib/module.c
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,8 @@ module_ret_code stop(mod_t *mod, const bool stop) {

GET_CTX_PRIV((&mod->self));

const bool was_running = _module_is(mod, RUNNING);

int ret = manage_fds(mod, c, RM, stop);
MOD_ASSERT(!ret, errors[stop], MOD_ERR);

Expand All @@ -234,9 +236,23 @@ module_ret_code stop(mod_t *mod, const bool stop) {
}

MODULE_DEBUG("%s '%s'.\n", stop ? "Stopped" : "Paused", mod->name);
tell_system_pubsub_msg(NULL, c, MODULE_STOPPED, &mod->self, NULL);

c->running_mods--;
/*
* Check that we are not deregistering; this is needed to
* avoid sending a &mod->self that will be freed right after
* (thus pointing to freed data).
* When deregistering, module has already been removed from c->modules map.
*/
self_t *self = NULL;
if (map_has_key(c->modules, mod->name)) {
self = &mod->self;
}
tell_system_pubsub_msg(NULL, c, MODULE_STOPPED, self, NULL);

/* Increment only if we were previously RUNNING */
if (was_running) {
c->running_mods--;
}
return MOD_OK;
}

Expand Down Expand Up @@ -307,10 +323,10 @@ module_ret_code module_deregister(self_t **self) {
GET_MOD(*self);
MODULE_DEBUG("Deregistering module '%s'.\n", mod->name);

stop(mod, true);

/* Remove the module from the context */
map_remove((*self)->ctx->modules, mod->name);

stop(mod, true);

/* Remove context without modules */
if (map_length((*self)->ctx->modules) == 0) {
Expand Down
3 changes: 2 additions & 1 deletion TODO.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ conversely to module_stop that should stop module right away freeing all its enq
- [x] Avoid exposing in modules.h main() and modules_pre_start() functions: they're not part of libmodule's API and cannot be called as functions.
- [x] Rename module to mod_t and m_context to ctx_t
- [x] Rename module_poll_t to fd_priv_t
- [ ] Automatically poisonpill a module if init()/receive() return false?
- [x] FIX: avoid sending with MODULE_STOPPED pubsub message a not-exishtent reference to mod->self when deregistering, as module gets freed right after. Send NULL.
- [x] FIX: when deregistering, remove module from context before stopping it

### Doc
- [x] module_dump
Expand Down
2 changes: 1 addition & 1 deletion docs/src/pubsub.rst
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ Beside USER messages (ps_msg_t.type), there are some system messages, respective
These pubsub messages are automatically sent by libmodule when matching functions are called, eg: |br|
* LOOP_STARTED(STOPPED) is sent whenever a loop starts(stops) looping. It is useful to actually start(stop) your pubsub messagging (eg: one module waits on LOOP_STARTED to send a pubsub message to another module, and so on...). It won't have any valued fields, except for type. |br|
* MODULE_STARTED(STOPPED) is sent whenever a module starts/resumes(stops/pauses). Again this is useful to inform other modules that a new module is available. |br|
It will have valued type and sender fields; sender will be set to started(stopped) module.
It will have valued type and sender fields; sender will be set to started(stopped) module; it will be NULL though when a module is stopped by deregistering it.

Finally, note that system messages with valued sender won't be sent to modules that actually generated the message.

Expand Down

0 comments on commit 973e426

Please sign in to comment.