From d2ccba80c5a06d9bfcd9dead4089a1a4f9bd110d Mon Sep 17 00:00:00 2001 From: Bram Matthys Date: Fri, 10 Nov 2023 10:01:05 +0100 Subject: [PATCH] Moddata fixes: LoadPersistent*()/SavePersistent*() and removing mdata. The LoadPersistent*()/SavePersistent*() functions caused moddata to be tagged with ->unloaded=1. Though it seems it caused no real issues this is not good... we now properly tag them as 0 and the like. Also did a code cleanup / overhaul on that system as well. For other ModData we now handle the case where a module is loaded with with a newer version and that newer version is no longer having certain moddata, eg the name changed or it no longer needs it. KNOWN ISSUE: Unfortunately we cannot call the free function for the old moddata that is no longer being handled by the newer version of the module, since the module is already unloaded. So this will result in a memory leak, but not in a crash. KNOWN ISSUE: Similarly, for SavePersistentPointer() there is a free function, again this is called just fine if the module is permanently unloaded but NOT if the module is reloaded with the same name and no longer is interested in the persistent pointer object. Again, here too, that would result in a memory leak but not in a crash. Fortunately the "known issues" are rare. Fixing these is impossible with the current module API because modules are unloaded after MOD_TEST and before MOD_INIT, and only after MOD_INIT we know which moddata is handled by the new version of the module. To change that we would need to keep the old module around until after MOD_INIT of the new module (so we can call free functions in the old module), but that means delaying the MOD_UNLOAD for the old modules until after MOD_INIT of the new modules, which changes the sequence too much that i don't dare to do that. For example, it would mean a database save routine in the old module would only be called after MOD_INIT finished in the new module, which may be unexpected since right now MOD_UNLOAD is called before MOD_INIT and maybe the db loading is done in MOD_INIT, which would need to be moved to MOD_LOAD. That's just one example, there may be others. I think such a change can only be done on a major UnrealIRCd version change, so we will have to live this for now. As said, fortunately it is a corner case. --- include/modules.h | 8 +-- src/api-moddata.c | 21 +++++++- src/conf.c | 2 +- src/modules.c | 133 +++++++++++++++------------------------------- 4 files changed, 66 insertions(+), 98 deletions(-) diff --git a/include/modules.h b/include/modules.h index 644674157..7eaabc91b 100644 --- a/include/modules.h +++ b/include/modules.h @@ -996,22 +996,22 @@ extern ModData *moddata_client_get_raw(Client *client, const char *varname); extern int moddata_local_client_set(Client *acptr, const char *varname, const char *value); extern const char *moddata_local_client_get(Client *acptr, const char *varname); -extern int LoadPersistentPointerX(ModuleInfo *modinfo, const char *varshortname, void **var, void (*free_variable)(ModData *m)); +extern void LoadPersistentPointerX(ModuleInfo *modinfo, const char *varshortname, void **var, void (*free_variable)(ModData *m)); #define LoadPersistentPointer(modinfo, var, free_variable) LoadPersistentPointerX(modinfo, #var, (void **)&var, free_variable) extern void SavePersistentPointerX(ModuleInfo *modinfo, const char *varshortname, void *var); #define SavePersistentPointer(modinfo, var) SavePersistentPointerX(modinfo, #var, var) -extern int LoadPersistentIntX(ModuleInfo *modinfo, const char *varshortname, int *var); +extern void LoadPersistentIntX(ModuleInfo *modinfo, const char *varshortname, int *var); #define LoadPersistentInt(modinfo, var) LoadPersistentIntX(modinfo, #var, &var) extern void SavePersistentIntX(ModuleInfo *modinfo, const char *varshortname, int var); #define SavePersistentInt(modinfo, var) SavePersistentIntX(modinfo, #var, var) -extern int LoadPersistentLongX(ModuleInfo *modinfo, const char *varshortname, long *var); +extern void LoadPersistentLongX(ModuleInfo *modinfo, const char *varshortname, long *var); #define LoadPersistentLong(modinfo, var) LoadPersistentLongX(modinfo, #var, &var) extern void SavePersistentLongX(ModuleInfo *modinfo, const char *varshortname, long var); #define SavePersistentLong(modinfo, var) SavePersistentLongX(modinfo, #var, var) -extern int LoadPersistentLongLongX(ModuleInfo *modinfo, const char *varshortname, long long *var); +extern void LoadPersistentLongLongX(ModuleInfo *modinfo, const char *varshortname, long long *var); #define LoadPersistentLongLong(modinfo, var) LoadPersistentLongLongX(modinfo, #var, &var) extern void SavePersistentLongLongX(ModuleInfo *modinfo, const char *varshortname, long long var); #define SavePersistentLongLong(modinfo, var) SavePersistentLongLongX(modinfo, #var, var) diff --git a/src/api-moddata.c b/src/api-moddata.c index aa0767142..31c84c892 100644 --- a/src/api-moddata.c +++ b/src/api-moddata.c @@ -285,24 +285,41 @@ void ModDataDel(ModDataInfo *md) break; } } + /* Module is unloading, so owner will be gone: */ md->owner = NULL; } if (loop.rehashing) + { + /* Since the module is unloaded, these are not safe anymore either: */ + md->free = NULL; + md->serialize = NULL; + md->unserialize = NULL; md->unloaded = 1; - else + } else { + /* We need the free function and the like, + * and then completely destroy 'md'. + */ unload_moddata_commit(md); + } } void unload_all_unused_moddata(void) { -ModDataInfo *md, *md_next; + ModDataInfo *md, *md_next; for (md = MDInfo; md; md = md_next) { md_next = md->next; if (md->unloaded) + { + //config_status("UNLOADING: md %s (owner %p, type %d, slot %d)", + // md->name, md->owner, md->type, md->slot); unload_moddata_commit(md); + } else { + //config_status("loaded: md %s (owner %p, type %d, slot %d)", + // md->name, md->owner, md->type, md->slot); + } } } diff --git a/src/conf.c b/src/conf.c index 1ecec29fe..1c7cb4db3 100644 --- a/src/conf.c +++ b/src/conf.c @@ -11152,7 +11152,7 @@ int rehash_internal(Client *client) unload_all_unused_caps(); unload_all_unused_history_backends(); unload_all_unused_rpc_handlers(); - // unload_all_unused_moddata(); -- this will crash + unload_all_unused_moddata(); clicap_check_for_changes(); umodes_check_for_changes(); charsys_check_for_changes(); diff --git a/src/modules.c b/src/modules.c index 6b6ae2d71..20e6f4874 100644 --- a/src/modules.c +++ b/src/modules.c @@ -1191,7 +1191,7 @@ EVENT(e_unload_module_delayed) if (i == 1) { unreal_log(ULOG_INFO, "module", "MODULE_UNLOADING_DELAYED", NULL, - "Unloading module $module_name (was delayed earlier)", + "Unloaded module $module_name (was delayed earlier)", log_data_string("module_name", name)); } safe_free(name); @@ -1364,16 +1364,24 @@ static const char *mod_var_name(ModuleInfo *modinfo, const char *varshortname) return fullname; } -int LoadPersistentPointerX(ModuleInfo *modinfo, const char *varshortname, void **var, void (*free_variable)(ModData *m)) +ModDataInfo *persistent_var_generic(ModuleInfo *modinfo, const char *varshortname, void (*free_variable)(ModData *m)) { ModDataInfo *m; const char *fullname = mod_var_name(modinfo, varshortname); + if (!modinfo) + abort(); + m = findmoddata_byname(fullname, MODDATATYPE_LOCAL_VARIABLE); if (m) { - *var = moddata_local_variable(m).ptr; - return 1; + if (loop.config_status != CONFIG_STATUS_TEST) + { + /* Claim ownership */ + m->owner = modinfo->handle; + m->unloaded = 0; + } + return m; } else { ModDataInfo mreq; memset(&mreq, 0, sizeof(mreq)); @@ -1381,114 +1389,57 @@ int LoadPersistentPointerX(ModuleInfo *modinfo, const char *varshortname, void * mreq.name = strdup(fullname); mreq.free = free_variable; m = ModDataAdd(modinfo->handle, mreq); - moddata_local_variable(m).ptr = NULL; safe_free(mreq.name); - return 0; + memset(&moddata_local_variable(m), 0, sizeof(ModData)); + return m; } } +void LoadPersistentPointerX(ModuleInfo *modinfo, const char *varshortname, void **var, void (*free_variable)(ModData *m)) +{ + ModDataInfo *m = persistent_var_generic(modinfo, varshortname, free_variable); + *var = moddata_local_variable(m).ptr; +} + +void LoadPersistentIntX(ModuleInfo *modinfo, const char *varshortname, int *var) +{ + ModDataInfo *m = persistent_var_generic(modinfo, varshortname, NULL); + *var = moddata_local_variable(m).i; +} + +void LoadPersistentLongX(ModuleInfo *modinfo, const char *varshortname, long *var) +{ + ModDataInfo *m = persistent_var_generic(modinfo, varshortname, NULL); + *var = moddata_local_variable(m).l; +} + +void LoadPersistentLongLongX(ModuleInfo *modinfo, const char *varshortname, long long *var) +{ + ModDataInfo *m = persistent_var_generic(modinfo, varshortname, NULL); + *var = moddata_local_variable(m).ll; +} + void SavePersistentPointerX(ModuleInfo *modinfo, const char *varshortname, void *var) { - ModDataInfo *m; - const char *fullname = mod_var_name(modinfo, varshortname); - - m = findmoddata_byname(fullname, MODDATATYPE_LOCAL_VARIABLE); + ModDataInfo *m = persistent_var_generic(modinfo, varshortname, NULL); moddata_local_variable(m).ptr = var; } -int LoadPersistentIntX(ModuleInfo *modinfo, const char *varshortname, int *var) -{ - ModDataInfo *m; - const char *fullname = mod_var_name(modinfo, varshortname); - - m = findmoddata_byname(fullname, MODDATATYPE_LOCAL_VARIABLE); - if (m) - { - *var = moddata_local_variable(m).i; - return 1; - } else { - ModDataInfo mreq; - memset(&mreq, 0, sizeof(mreq)); - mreq.type = MODDATATYPE_LOCAL_VARIABLE; - mreq.name = strdup(fullname); - mreq.free = NULL; - m = ModDataAdd(modinfo->handle, mreq); - moddata_local_variable(m).i = 0; - safe_free(mreq.name); - return 0; - } -} - void SavePersistentIntX(ModuleInfo *modinfo, const char *varshortname, int var) { - ModDataInfo *m; - const char *fullname = mod_var_name(modinfo, varshortname); - - m = findmoddata_byname(fullname, MODDATATYPE_LOCAL_VARIABLE); + ModDataInfo *m = persistent_var_generic(modinfo, varshortname, NULL); moddata_local_variable(m).i = var; } -int LoadPersistentLongX(ModuleInfo *modinfo, const char *varshortname, long *var) -{ - ModDataInfo *m; - const char *fullname = mod_var_name(modinfo, varshortname); - - m = findmoddata_byname(fullname, MODDATATYPE_LOCAL_VARIABLE); - if (m) - { - *var = moddata_local_variable(m).l; - return 1; - } else { - ModDataInfo mreq; - memset(&mreq, 0, sizeof(mreq)); - mreq.type = MODDATATYPE_LOCAL_VARIABLE; - mreq.name = strdup(fullname); - mreq.free = NULL; - m = ModDataAdd(modinfo->handle, mreq); - moddata_local_variable(m).l = 0; - safe_free(mreq.name); - return 0; - } -} - void SavePersistentLongX(ModuleInfo *modinfo, const char *varshortname, long var) { - ModDataInfo *m; - const char *fullname = mod_var_name(modinfo, varshortname); - - m = findmoddata_byname(fullname, MODDATATYPE_LOCAL_VARIABLE); + ModDataInfo *m = persistent_var_generic(modinfo, varshortname, NULL); moddata_local_variable(m).l = var; } -int LoadPersistentLongLongX(ModuleInfo *modinfo, const char *varshortname, long long *var) -{ - ModDataInfo *m; - const char *fullname = mod_var_name(modinfo, varshortname); - - m = findmoddata_byname(fullname, MODDATATYPE_LOCAL_VARIABLE); - if (m) - { - *var = moddata_local_variable(m).ll; - return 1; - } else { - ModDataInfo mreq; - memset(&mreq, 0, sizeof(mreq)); - mreq.type = MODDATATYPE_LOCAL_VARIABLE; - mreq.name = strdup(fullname); - mreq.free = NULL; - m = ModDataAdd(modinfo->handle, mreq); - moddata_local_variable(m).ll = 0; - safe_free(mreq.name); - return 0; - } -} - void SavePersistentLongLongX(ModuleInfo *modinfo, const char *varshortname, long long var) { - ModDataInfo *m; - const char *fullname = mod_var_name(modinfo, varshortname); - - m = findmoddata_byname(fullname, MODDATATYPE_LOCAL_VARIABLE); + ModDataInfo *m = persistent_var_generic(modinfo, varshortname, NULL); moddata_local_variable(m).ll = var; }