From f95f67ac43c455993a84f03dbda6464edac0a7aa Mon Sep 17 00:00:00 2001 From: "Rob Swindell (on Windows)" <rob@synchro.net> Date: Mon, 3 Apr 2023 23:05:51 -0700 Subject: [PATCH] Fix double-free race condition with SBBSCTRL upon global recycle When multiple servers are recycling at the same time, (e.g. due to saved change in SCFG) they'd each call sbbs_read_ini() with a shared global_startup struct, which in turn calls sbbs_free_ini(), which would free all the allocated network interface lists (including the global_startup one) using iniFreeStringList (just a wrapper for strListFree), but iniFreeStringList() does NOT modify (NULLify) the freed-pointer, so your second or third server that called sbbs_read_ini(), with the shared MainForm->global structure, would *again* free the same global interface list. This bug actually has always existed because get_ini_globals() freed the global interface list in the same way, except it *immediately* re-allocated a new one by calling iniGetStringList(), so the time window (opportunity) for this race condition to occur was much smaller. Truly, SBBSCTRL should use a mutex or other mechanism to protect the shared global_startup struct, but this is a first step to a full fix: sbbs_free_ini() should (and now does) nullify the freed network interface pointers by using strListFree() directly. I haven't been able to reproduce the crash upon recycle in SBBSCTRL after making this change. --- src/sbbs3/sbbs_ini.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/sbbs3/sbbs_ini.c b/src/sbbs3/sbbs_ini.c index b7421ef1a9..da23ffa29c 100644 --- a/src/sbbs3/sbbs_ini.c +++ b/src/sbbs3/sbbs_ini.c @@ -308,28 +308,28 @@ void sbbs_free_ini( ) { if(global != NULL) { - iniFreeStringList(global->interfaces); + strListFree(&global->interfaces); } if(bbs != NULL) { - iniFreeStringList(bbs->telnet_interfaces); - iniFreeStringList(bbs->rlogin_interfaces); - iniFreeStringList(bbs->ssh_interfaces); + strListFree(&bbs->telnet_interfaces); + strListFree(&bbs->rlogin_interfaces); + strListFree(&bbs->ssh_interfaces); } if(web != NULL) { - iniFreeStringList(web->interfaces); - iniFreeStringList(web->tls_interfaces); - iniFreeStringList(web->index_file_name); - iniFreeStringList(web->cgi_ext); + strListFree(&web->interfaces); + strListFree(&web->tls_interfaces); + strListFree(&web->index_file_name); + strListFree(&web->cgi_ext); } if(ftp != NULL) { - iniFreeStringList(ftp->interfaces); + strListFree(&ftp->interfaces); } if(mail != NULL) { - iniFreeStringList(mail->interfaces); - iniFreeStringList(mail->pop3_interfaces); + strListFree(&mail->interfaces); + strListFree(&mail->pop3_interfaces); } if(services != NULL) { - iniFreeStringList(services->interfaces); + strListFree(&services->interfaces); } } -- GitLab