Re: svn commit: r1883708 - /httpd/httpd/trunk/server/core.c

[Joe Orton <jo...@redhat.com>]

On Sun, Nov 22, 2020 at 01:06:11AM -0000, ylavic@apache.org wrote:
> Author: ylavic
> Date: Sun Nov 22 01:06:11 2020
> New Revision: 1883708
> 
> URL: http://svn.apache.org/viewvc?rev=1883708&view=rev
> Log:
> core: reset ap_runtime_dir to NULL after AP_SQ_MS_DESTROY_CONFIG.
> 
> ap_runtime_dir_relative() might reuse ap_runtime_dir from previously cleared
> pconf otherwise.

There was the same bug with state_dir handling at one point too, can we 
unify this to maybe avoid future similar mistakes?  Is this sufficient 
for whatever problem you saw as well?

Index: server/config.c
===================================================================
--- server/config.c	(revision 1884093)
+++ server/config.c	(working copy)
@@ -59,7 +59,6 @@
 
 AP_DECLARE_DATA const char *ap_server_argv0 = NULL;
 AP_DECLARE_DATA const char *ap_server_root = NULL;
-AP_DECLARE_DATA const char *ap_runtime_dir = NULL;
 AP_DECLARE_DATA server_rec *ap_server_conf = NULL;
 AP_DECLARE_DATA apr_pool_t *ap_pglobal = NULL;
 
Index: server/core.c
===================================================================
--- server/core.c	(revision 1884093)
+++ server/core.c	(working copy)
@@ -127,15 +127,18 @@
 /* magic pointer for ErrorDocument xxx "default" */
 static char errordocument_default;
 
+/* Global state allocated out of pconf: variables here MUST be
+ * cleared/reset in reset_config(), a pconf cleanup, to avoid the
+ * variable getting reused after the pool is cleared. */
 static apr_array_header_t *saved_server_config_defines = NULL;
 static apr_table_t *server_config_defined_vars = NULL;
+AP_DECLARE_DATA const char *ap_runtime_dir = NULL;
+static const char *core_state_dir;
 
 AP_DECLARE_DATA int ap_main_state = AP_SQ_MS_INITIAL_STARTUP;
 AP_DECLARE_DATA int ap_run_mode = AP_SQ_RM_UNKNOWN;
 AP_DECLARE_DATA int ap_config_generation = 0;
 
-static const char *core_state_dir;
-
 typedef struct {
     apr_ipsubnet_t *subnet;
     struct ap_logconf log;
@@ -1489,6 +1492,7 @@
     saved_server_config_defines = NULL;
     server_config_defined_vars = NULL;
     core_state_dir = NULL;
+    ap_runtime_dir = NULL;
 
     return APR_SUCCESS;
 }
@@ -5863,7 +5867,6 @@
 
 static void register_hooks(apr_pool_t *p)
 {
-    ap_runtime_dir = NULL;
     errorlog_hash = apr_hash_make(p);
     ap_register_log_hooks(p);
     ap_register_config_hooks(p);

Re: svn commit: r1883708 - /httpd/httpd/trunk/server/core.c

[Yann Ylavic <yl...@gmail.com>]

On Fri, Dec 4, 2020 at 5:39 PM Joe Orton <jo...@redhat.com> wrote:
>
> On Sun, Nov 22, 2020 at 01:06:11AM -0000, ylavic@apache.org wrote:
> > Author: ylavic
> > Date: Sun Nov 22 01:06:11 2020
> > New Revision: 1883708
> >
> > URL: http://svn.apache.org/viewvc?rev=1883708&view=rev
> > Log:
> > core: reset ap_runtime_dir to NULL after AP_SQ_MS_DESTROY_CONFIG.
> >
> > ap_runtime_dir_relative() might reuse ap_runtime_dir from previously cleared
> > pconf otherwise.
>
> There was the same bug with state_dir handling at one point too, can we
> unify this to maybe avoid future similar mistakes?  Is this sufficient
> for whatever problem you saw as well?

Yes, good idea, ASan didn't detect other "reload" issues than
ap_runtime_dir so we should be good (for now :).
I'll try running the tests suite with --enable-mods-static=reallyall
(i.e. no DSOs unload/reload) so that there is no static
re-initialization in modules.
This could help detect more issues like this..

Regards;
Yann.