From 32c99c7b0a58545b273fa713d81d6bbfb3b8ebe6 Mon Sep 17 00:00:00 2001 From: bones_was_here Date: Mon, 5 Feb 2024 02:18:03 +1000 Subject: [PATCH] cvar: fix many bugs in gamedir, loadconfig, unset, cvar_resettodefaults* commands Fixes a segfault when Cvar_RestoreInitState() deleted cvars (null ptr deref). Fixes an infinite loop in Host_LoadConfig_f() due to calling Cbuf_Execute() from inside Cbuf_Execute() via Host_AddConfigText(). Fixes `unset` being able to delete autocvars while they're in use by QC. Fixes Cvar_RestoreInitState() not updating autocvars when necessary. Fixes Cvar_RestoreInitState() not calling cvar callbacks. Fixes Cvar_RestoreInitState() sometimes restoring such that .string doesn't match .value, or such that .defstring is wrong. This was caused by Cvar_SaveInitState() saving only string pointers and not strings. Fixes CF_READONLY cvars like gl_info_* getting restored/reset. It doesn't make sense for any of these to be changed by code outside their own subsystems. Fixes Xonotic menu QC errors caused by Host_LoadConfig_f deleting cvars before calling QC's m_shutdown(). Deduplicates and simplifies cvar deletion code, see also 05a5ed884366d3a1c8e211168efc19b61867cfca Saves a little memory by storing only minimal init state data for Cvar_RestoreInitState(), and by relaxing alignment requirements in _Mem_strdup(). Improves some warns and docs and tidies up a little. Removes `vid_restart` from `gamedir`, it usually isn't needed because current DP applies many vid_ cvar changes without it, and because of the ubiquity of flat panel displays. Fixes https://gitlab.com/xonotic/darkplaces/-/issues/354 Signed-off-by: bones_was_here --- cmd.h | 2 +- cvar.c | 242 ++++++++++++++++++++++++--------------------------------- cvar.h | 3 +- fs.c | 4 +- host.c | 26 +++---- menu.c | 6 +- zone.c | 6 +- zone.h | 4 +- 8 files changed, 129 insertions(+), 164 deletions(-) diff --git a/cmd.h b/cmd.h index ada9769c..97925042 100644 --- a/cmd.h +++ b/cmd.h @@ -51,7 +51,7 @@ struct cmd_state_s; #define CF_SERVER_FROM_CLIENT (1u<<3) ///< command the client is allowed to execute on the server as a stringcmd #define CF_CHEAT (1u<<4) ///< command or cvar that gives an unfair advantage over other players and is blocked unless sv_cheats is 1 #define CF_ARCHIVE (1u<<5) ///< cvar should have its set value saved to config.cfg and persist across sessions -#define CF_READONLY (1u<<6) ///< cvar cannot be changed from the console or the command buffer +#define CF_READONLY (1u<<6) ///< cvar cannot be changed from the console or the command buffer, and is considered CF_PERSISTENT #define CF_NOTIFY (1u<<7) ///< cvar should trigger a chat notification to all connected clients when changed #define CF_SERVERINFO (1u<<8) ///< command or cvar relevant to serverinfo string handling #define CF_USERINFO (1u<<9) ///< command or cvar used to communicate userinfo to the server diff --git a/cvar.c b/cvar.c index b1d7d91a..ce8d89ed 100644 --- a/cvar.c +++ b/cvar.c @@ -78,7 +78,7 @@ cvar_t *Cvar_FindVarAfter(cvar_state_t *cvars, const char *prev_var_name, unsign * Returns a pointer to the pointer stored in hashtable[] (or the one it links to) * because we'll need to update that when deleting a cvar as other cvar(s) may share its hashindex. */ -static cvar_hash_t **Cvar_FindVarLink(cvar_state_t *cvars, const char *var_name, cvar_t **parent, cvar_t ***link, cvar_t **prev_alpha, unsigned neededflags) +static cvar_hash_t **Cvar_FindVarLink(cvar_state_t *cvars, const char *var_name, cvar_t **prev_alpha, unsigned neededflags) { unsigned hashindex; cvar_t *cvar; @@ -86,9 +86,7 @@ static cvar_hash_t **Cvar_FindVarLink(cvar_state_t *cvars, const char *var_name, // use hash lookup to minimize search time hashindex = CRC_Block((const unsigned char *)var_name, strlen(var_name)) % CVAR_HASHSIZE; - if(parent) *parent = NULL; if(prev_alpha) *prev_alpha = NULL; - if(link) *link = &cvars->hashtable[hashindex]->cvar; hashlinkptr = &cvars->hashtable[hashindex]; for (hashlinkptr = &cvars->hashtable[hashindex]; *hashlinkptr; hashlinkptr = &(*hashlinkptr)->next) { @@ -99,13 +97,12 @@ static cvar_hash_t **Cvar_FindVarLink(cvar_state_t *cvars, const char *var_name, for (char **alias = cvar->aliases; alias && *alias; alias++) if (!strcmp (var_name, *alias) && (cvar->flags & neededflags)) goto match; - if(parent) *parent = cvar; } return NULL; + match: if(!prev_alpha || cvar == cvars->vars) return hashlinkptr; - *prev_alpha = cvars->vars; // if prev_alpha happens to become NULL then there has been some inconsistency elsewhere // already - should I still insert '*prev_alpha &&' in the loop? @@ -304,8 +301,9 @@ void Cvar_CompleteCvarPrint(cvar_state_t *cvars, const char *partial, unsigned n } -// check if a cvar is held by some progs -static qbool Cvar_IsAutoCvar(cvar_t *var) +/// Check if a cvar is held by some progs +/// in which case its name is returned, otherwise NULL. +static const char *Cvar_IsAutoCvar(cvar_t *var) { int i; prvm_prog_t *prog; @@ -313,12 +311,12 @@ static qbool Cvar_IsAutoCvar(cvar_t *var) { prog = &prvm_prog_list[i]; if (prog->loaded && var->globaldefindex[i] >= 0) - return true; + return prog->name; } - return false; + return NULL; } -// we assume that prog is already set to the target progs +/// we assume that prog is already set to the target progs static void Cvar_UpdateAutoCvar(cvar_t *var) { int i; @@ -363,7 +361,7 @@ static void Cvar_UpdateAutoCvar(cvar_t *var) } } -// called after loading a savegame +/// called after loading a savegame void Cvar_UpdateAllAutoCvars(cvar_state_t *cvars) { cvar_t *var; @@ -549,7 +547,7 @@ void Cvar_RegisterVirtual(cvar_t *variable, const char *name ) // Also if aliases is NULL this allocates fresh for the correct size, so it's fine to just do this. variable->aliases = (char **)Z_Realloc(variable->aliases, sizeof(char *) * (variable->aliases_size + 2)); // Add the new alias, and increment the number of aliases in the list - variable->aliases[variable->aliases_size++] = (char *)Z_strdup(name); + variable->aliases[variable->aliases_size++] = Z_strdup(name); // link to head of list in this hash table index hash = (cvar_hash_t *)Z_Malloc(sizeof(cvar_hash_t)); @@ -668,14 +666,14 @@ void Cvar_RegisterVariable (cvar_t *variable) } // copy the value off, because future sets will Z_Free it - variable->name = (char *)Mem_strdup(zonemempool, variable->name); - variable->string = (char *)Mem_strdup(zonemempool, variable->string); - variable->defstring = (char *)Mem_strdup(zonemempool, variable->string); + variable->name = Z_strdup(variable->name); + variable->string = Z_strdup(variable->string); + variable->defstring = Z_strdup(variable->string); variable->value = atof (variable->string); variable->integer = (int) variable->value; variable->aliases = NULL; variable->aliases_size = 0; - variable->initstate = NULL; + variable->initstring = NULL; // Mark it as not an autocvar. for (i = 0;i < PRVM_PROG_MAX;i++) @@ -714,7 +712,7 @@ cvar_t *Cvar_Get(cvar_state_t *cvars, const char *name, const char *value, unsig Z_Free((char *)cvar->description); if(*newdescription) - cvar->description = (char *)Mem_strdup(zonemempool, newdescription); + cvar->description = Z_strdup(newdescription); else cvar->description = cvar_dummy_description; } @@ -740,17 +738,17 @@ cvar_t *Cvar_Get(cvar_state_t *cvars, const char *name, const char *value, unsig // FIXME: these never get Z_Free'd cvar = (cvar_t *)Z_Malloc(sizeof(cvar_t)); cvar->flags = flags | CF_ALLOCATED; - cvar->name = (char *)Mem_strdup(zonemempool, name); - cvar->string = (char *)Mem_strdup(zonemempool, value); - cvar->defstring = (char *)Mem_strdup(zonemempool, value); + cvar->name = Z_strdup(name); + cvar->string = Z_strdup(value); + cvar->defstring = Z_strdup(value); cvar->value = atof (cvar->string); cvar->integer = (int) cvar->value; cvar->aliases = NULL; cvar->aliases_size = 0; - cvar->initstate = NULL; + cvar->initstring = NULL; if(newdescription && *newdescription) - cvar->description = (char *)Mem_strdup(zonemempool, newdescription); + cvar->description = Z_strdup(newdescription); else cvar->description = cvar_dummy_description; // actually checked by VM_cvar_type @@ -763,6 +761,55 @@ cvar_t *Cvar_Get(cvar_state_t *cvars, const char *name, const char *value, unsig return cvar; } +/// For "quiet" mode pass NULL as the callername. +/// Returns true if the cvar was deleted. +static qbool Cvar_Delete(cvar_state_t *cvars, const char *name, const char *callername) +{ + cvar_t *cvar, *prev; + cvar_hash_t **hashlinkptr, *oldhashlink; + const char *progname; + + hashlinkptr = Cvar_FindVarLink(cvars, name, &prev, ~0); + if(!hashlinkptr) + { + if (callername) + Con_Printf("%s: cvar \"%s\" is not defined.\n", callername, name); + return false; + } + cvar = (*hashlinkptr)->cvar; + + if(!(cvar->flags & CF_ALLOCATED)) + { + if (callername) + Con_Printf(CON_WARN "%s: engine cvar \"%s\" cannot be deleted!\n", callername, cvar->name); + return false; + } + if ((progname = Cvar_IsAutoCvar(cvar))) + { + if (callername) + Con_Printf(CON_WARN "%s: unable to delete cvar \"%s\", it is an autocvar used by running %s progs!\n", callername, cvar->name, progname); + return false; + } + + if(cvar == cvars->vars) + cvars->vars = cvar->next; + else + prev->next = cvar->next; + + if(cvar->description != cvar_dummy_description) + Z_Free((char *)cvar->description); + Z_Free((char *)cvar->name); + Z_Free((char *)cvar->string); + Z_Free((char *)cvar->defstring); + Z_Free(cvar); + + oldhashlink = *hashlinkptr; + *hashlinkptr = (*hashlinkptr)->next; + Z_Free(oldhashlink); + + return true; +} + qbool Cvar_Readonly (cvar_t *var, const char *cmd_name) { if (var->flags & CF_READONLY) @@ -845,86 +892,48 @@ void Cvar_LockDefaults_f(cmd_state_t *cmd) void Cvar_SaveInitState(cvar_state_t *cvars) { cvar_t *c; + for (c = cvars->vars;c;c = c->next) - { - c->initstate = (cvar_t *)Z_Malloc(sizeof(cvar_t)); - memcpy(c->initstate, c, sizeof(cvar_t)); - } + if ((c->flags & (CF_PERSISTENT | CF_READONLY)) == 0) + c->initstring = Z_strdup(c->string); } void Cvar_RestoreInitState(cvar_state_t *cvars) { - unsigned hashindex; - cvar_t *c, **cp; - cvar_t *c2, **cp2; - for (cp = &cvars->vars;(c = *cp);) + cvar_t *c, *cp; + int i = 0; + + for (cp = cvars->vars; (c = cp);++i) { - if (c->initstate) + cp = c->next; // get next cvar now in case we delete this cvar + + if (c->initstring) { // restore this cvar, it existed at init - if (((c->flags ^ c->initstate->flags) & CF_MAXFLAGSVAL) - || strcmp(c->defstring ? c->defstring : "", c->initstate->defstring ? c->initstate->defstring : "") - || strcmp(c->string ? c->string : "", c->initstate->string ? c->initstate->string : "")) - { - Con_DPrintf("Cvar_RestoreInitState: Restoring cvar \"%s\"\n", c->name); - if (c->defstring) - Z_Free((char *)c->defstring); - c->defstring = Mem_strdup(zonemempool, c->initstate->defstring); - if (c->string) - Z_Free((char *)c->string); - c->string = Mem_strdup(zonemempool, c->initstate->string); - } - c->flags = c->initstate->flags; - c->value = c->initstate->value; - c->integer = c->initstate->integer; - VectorCopy(c->initstate->vector, c->vector); - cp = &c->next; + Con_DPrintf("Cvar_RestoreInitState: Restoring cvar \"%s\"\n", c->name); + + /* bones_was_here: intentionally NOT restoring defstring in this function. + * The only callsite, Host_LoadConfig_f, will re-exec quake.rc, default.cfg, etc. + * Defaults are unlocked here, so Cvar_LockDefaults_f will reset the defstring. + * This is more correct than restoring the defstring here + * because a gamedir change could load configs that should change defaults. + */ + + Cvar_SetQuick(c, c->initstring); + c->flags &= ~CF_DEFAULTSET; } - else + else if (!(c->flags & (CF_PERSISTENT | CF_READONLY))) // cvars with those flags have no initstring AND may not be deleted or reset { - if (!(c->flags & CF_ALLOCATED)) - { - Con_DPrintf("Cvar_RestoreInitState: Unable to destroy cvar \"%s\", it was registered after init!\n", c->name); - // In this case, at least reset it to the default. - if((c->flags & CF_PERSISTENT) == 0) - Cvar_SetQuick(c, c->defstring); - cp = &c->next; - continue; - } - if (Cvar_IsAutoCvar(c)) - { - Con_DPrintf("Cvar_RestoreInitState: Unable to destroy cvar \"%s\", it is an autocvar used by running progs!\n", c->name); - // In this case, at least reset it to the default. - if((c->flags & CF_PERSISTENT) == 0) - Cvar_SetQuick(c, c->defstring); - cp = &c->next; - continue; - } // remove this cvar, it did not exist at init - Con_DPrintf("Cvar_RestoreInitState: Destroying cvar \"%s\"\n", c->name); - // unlink struct from hash - hashindex = CRC_Block((const unsigned char *)c->name, strlen(c->name)) % CVAR_HASHSIZE; - for (cp2 = &cvars->hashtable[hashindex]->cvar;(c2 = *cp2);) + Con_DPrintf("Cvar_RestoreInitState: cvar \"%s\"", c->name); + if (Cvar_Delete(cvars, c->name, NULL)) + Con_DPrint("deleted.\n"); + else { - if (c2 == c) - { - *cp2 = cvars->hashtable[hashindex]->next->cvar; - break; - } - else - cp2 = &cvars->hashtable[hashindex]->next->cvar; + // In this case, at least reset it to the default. + Con_DPrint("reset.\n"); + Cvar_SetQuick(c, c->defstring); } - // unlink struct from main list - *cp = c->next; - // free strings - if (c->defstring) - Z_Free((char *)c->defstring); - if (c->string) - Z_Free((char *)c->string); - if (c->description && c->description != cvar_dummy_description) - Z_Free((char *)c->description); - // free struct - Z_Free(c); } } } @@ -936,7 +945,7 @@ void Cvar_ResetToDefaults_All_f(cmd_state_t *cmd) // restore the default values of all cvars for (var = cvars->vars ; var ; var = var->next) { - if((var->flags & CF_PERSISTENT) == 0) + if((var->flags & (CF_PERSISTENT | CF_READONLY)) == 0) Cvar_SetQuick(var, var->defstring); } } @@ -948,7 +957,7 @@ void Cvar_ResetToDefaults_NoSaveOnly_f(cmd_state_t *cmd) // restore the default values of all cvars for (var = cvars->vars ; var ; var = var->next) { - if ((var->flags & (CF_PERSISTENT | CF_ARCHIVE)) == 0) + if ((var->flags & (CF_PERSISTENT | CF_READONLY | CF_ARCHIVE)) == 0) Cvar_SetQuick(var, var->defstring); } } @@ -961,7 +970,7 @@ void Cvar_ResetToDefaults_SaveOnly_f(cmd_state_t *cmd) // restore the default values of all cvars for (var = cvars->vars ; var ; var = var->next) { - if ((var->flags & (CF_PERSISTENT | CF_ARCHIVE)) == CF_ARCHIVE) + if ((var->flags & (CF_PERSISTENT | CF_READONLY | CF_ARCHIVE)) == CF_ARCHIVE) Cvar_SetQuick(var, var->defstring); } } @@ -1103,9 +1112,6 @@ void Cvar_Del_f(cmd_state_t *cmd) { cvar_state_t *cvars = cmd->cvars; int i; - cvar_t *parent, **link; - cvar_t *cvar, *prev; - cvar_hash_t **hashlinkptr, *oldhashlink; if(Cmd_Argc(cmd) < 2) { @@ -1113,51 +1119,7 @@ void Cvar_Del_f(cmd_state_t *cmd) return; } for(i = 1; i < Cmd_Argc(cmd); ++i) - { - hashlinkptr = Cvar_FindVarLink(cvars, Cmd_Argv(cmd, i), &parent, &link, &prev, ~0); - - if(!hashlinkptr) - { - Con_Printf("%s: %s is not defined\n", Cmd_Argv(cmd, 0), Cmd_Argv(cmd, i)); - continue; - } - cvar = (*hashlinkptr)->cvar; - - if(Cvar_Readonly(cvar, Cmd_Argv(cmd, 0))) - continue; - if(!(cvar->flags & CF_ALLOCATED)) - { - Con_Printf(CON_WARN "%s: %s is static and cannot be deleted\n", Cmd_Argv(cmd, 0), cvar->name); - continue; - } - - if(cvar == cvars->vars) - { - cvars->vars = cvar->next; - } - else - { - // in this case, prev must be set, otherwise there has been some inconsistensy - // elsewhere already... should I still check for prev != NULL? - prev->next = cvar->next; - } - - if(parent) - parent->next = cvar->next; - else if(link) - *link = cvar->next; - if(cvar->description != cvar_dummy_description) - Z_Free((char *)cvar->description); - - Z_Free((char *)cvar->name); - Z_Free((char *)cvar->string); - Z_Free((char *)cvar->defstring); - Z_Free(cvar); - - oldhashlink = *hashlinkptr; - *hashlinkptr = (*hashlinkptr)->next; - Z_Free(oldhashlink); - } + Cvar_Delete(cvars, Cmd_Argv(cmd, i), Cmd_Argv(cmd, 0)); } #ifdef FILLALLCVARSWITHRUBBISH diff --git a/cvar.h b/cvar.h index a489e1dc..a0a87167 100644 --- a/cvar.h +++ b/cvar.h @@ -81,7 +81,8 @@ typedef struct cvar_s char **aliases; int aliases_size; - struct cvar_s *initstate; // snapshot of cvar during init + // this is sufficient for Cvar_RestoreInitState() + const char *initstring; int globaldefindex[3]; int globaldefindex_stringno[3]; diff --git a/fs.c b/fs.c index c76a1467..e1b22c46 100644 --- a/fs.c +++ b/fs.c @@ -1713,8 +1713,8 @@ qbool FS_ChangeGameDirs(int numgamedirs, char gamedirs[][MAX_QPATH], qbool compl // unload all sounds so they will be reloaded from the new files as needed S_UnloadAllSounds_f(cmd_local); - // restart the video subsystem after the config is executed - Cbuf_InsertText(cmd_local, "\nloadconfig\nvid_restart\n\n"); + // reset everything that can be and reload configs + Cbuf_InsertText(cmd_local, "\nloadconfig\n"); return true; } diff --git a/host.c b/host.c index a2c43d85..de40ce7f 100644 --- a/host.c +++ b/host.c @@ -245,7 +245,10 @@ static void Host_AddConfigText(cmd_state_t *cmd) Cbuf_InsertText(cmd, "alias startmap_sp \"map start\"\nalias startmap_dm \"map start\"\nexec teu.rc\n"); else Cbuf_InsertText(cmd, "alias startmap_sp \"map start\"\nalias startmap_dm \"map start\"\nexec " STARTCONFIGFILENAME "\n"); - Cbuf_Execute(cmd->cbuf); + + // if quake.rc is missing, use default + if (!FS_FileExists(STARTCONFIGFILENAME)) + Cbuf_InsertText(cmd, "exec default.cfg\nexec " CONFIGFILENAME "\nexec autoexec.cfg\n"); } /* @@ -257,14 +260,17 @@ Resets key bindings and cvars to defaults and then reloads scripts */ static void Host_LoadConfig_f(cmd_state_t *cmd) { - // reset all cvars, commands and aliases to init values - Cmd_RestoreInitState(); #ifdef CONFIG_MENU - // prepend a menu restart command to execute after the config - Cbuf_InsertText(cmd_local, "\nmenu_restart\n"); + // Xonotic QC complains/breaks if its cvars are deleted before its m_shutdown() is called + if(MR_Shutdown) + MR_Shutdown(); + // append a menu restart command to execute after the config + Cbuf_AddText(cmd, "\nmenu_restart\n"); #endif + // reset all cvars, commands and aliases to init values + Cmd_RestoreInitState(); // reset cvars to their defaults, and then exec startup scripts again - Host_AddConfigText(cmd_local); + Host_AddConfigText(cmd); } /* @@ -494,13 +500,7 @@ static void Host_Init (void) // here comes the not so critical stuff Host_AddConfigText(cmd_local); - - // if quake.rc is missing, use default - if (!FS_FileExists("quake.rc")) - { - Cbuf_AddText(cmd_local, "exec default.cfg\nexec " CONFIGFILENAME "\nexec autoexec.cfg\n"); - Cbuf_Execute(cmd_local->cbuf); - } + Cbuf_Execute(cmd_local->cbuf); // cannot be in Host_AddConfigText as that would cause Host_LoadConfig_f to loop! host.state = host_active; diff --git a/menu.c b/menu.c index e26e74d7..891ebe1e 100644 --- a/menu.c +++ b/menu.c @@ -5306,10 +5306,12 @@ static void MP_KeyEvent (int key, int ascii, qbool downevent) static void MP_Draw (void) { prvm_prog_t *prog = MVM_prog; - // declarations that are needed right now - float oldquality; + // don't crash if we draw a frame between prog shutdown and restart, see Host_LoadConfig_f + if (!prog->loaded) + return; + R_SelectScene( RST_MENU ); // reset the temp entities each frame diff --git a/zone.c b/zone.c index f2888895..1fb9da39 100644 --- a/zone.c +++ b/zone.c @@ -650,7 +650,7 @@ void _Mem_CheckSentinelsGlobal(const char *filename, int fileline) #endif } -qbool Mem_IsAllocated(mempool_t *pool, void *data) +qbool Mem_IsAllocated(mempool_t *pool, const void *data) { memheader_t *header; memheader_t *target; @@ -863,14 +863,14 @@ static void MemStats_f(cmd_state_t *cmd) } -char* _Mem_strdup (mempool_t *pool, const char* s, const char *filename, int fileline) +char *_Mem_strdup(mempool_t *pool, const char *s, const char *filename, int fileline) { char* p; size_t sz; if (s == NULL) return NULL; sz = strlen (s) + 1; - p = (char*)_Mem_Alloc (pool, NULL, sz, 16, filename, fileline); + p = (char*)_Mem_Alloc (pool, NULL, sz, alignof(char), filename, fileline); dp_strlcpy (p, s, sz); return p; } diff --git a/zone.h b/zone.h index 12216f96..42644a19 100644 --- a/zone.h +++ b/zone.h @@ -107,9 +107,9 @@ void _Mem_EmptyPool(mempool_t *pool, const char *filename, int fileline); void _Mem_CheckSentinels(void *data, const char *filename, int fileline); void _Mem_CheckSentinelsGlobal(const char *filename, int fileline); // if pool is NULL this searches ALL pools for the allocation -qbool Mem_IsAllocated(mempool_t *pool, void *data); +qbool Mem_IsAllocated(mempool_t *pool, const void *data); -char* _Mem_strdup (mempool_t *pool, const char* s, const char *filename, int fileline); +char *_Mem_strdup(mempool_t *pool, const char *s, const char *filename, int fileline); typedef struct memexpandablearray_array_s { -- 2.39.2