Subject: va_list reuse in ap_hook.c

From: Jonathan Matthew <[email-masked]>
Date: Wed, 24 Sep 2003 19:39:17 +1000

Hi,

While working on a particularly nasty module, I discovered a few bugs in
ap_hook.c relating to reuse of va_lists.  On some platforms, va_lists
cannot be reused in mulitple function calls.  For example, this:

void e(va_list f, int g);

void a(int b, ...)
{
    va_list c;
    int d;
    va_start(c, b);
     
    for (d=0; d<4; d++) {
        e(c, d);
    }
    va_end(c);
}

is unsafe - on some platforms, calls to va_arg in the second call to e()
will not return arguments starting with the argument after 'b'.
Instead, va_arg will return garbage.  The only such platform I know of 
at the moment is s390 Linux, but there are probably others.

Anyway, here's a patch to mod_ssl 2.8.15 that fixes ap_hook_call() and
ap_hook_use().  I've simply moved all the code that calls va_ functions 
inside the for(hooks) loops in those functions.  I haven't checked for
other instances of va_list reuse in mod_ssl, but my guess is someone
would have noticed by now if they were anywhere less obscure than these.

You probably don't want to know what I'm doing to make this problem
show up.

-jonathan.

--- pkg.eapi/ap_hook.c  2003-09-24 17:23:59.000000000 +1000
+++ pkg.eapi/ap_hook.c  2003-09-24 18:15:32.000000000 +1000
@@ -314,23 +314,6 @@
     va_list ap;
     int rc;
 
-    va_start(ap, modeid);
-
-    if (modeid == AP_HOOK_MODE_DECLINE || modeid == AP_HOOK_MODE_DECLTMP) {
-        if (AP_HOOK_SIG_HAS(sig, RC, char))
-            modeval.v_char = va_arg(ap, va_type(char));
-        else if (AP_HOOK_SIG_HAS(sig, RC, int))
-            modeval.v_int = va_arg(ap, va_type(int));
-        else if (AP_HOOK_SIG_HAS(sig, RC, long))
-            modeval.v_long = va_arg(ap, va_type(long));
-        else if (AP_HOOK_SIG_HAS(sig, RC, float))
-            modeval.v_float = va_arg(ap, va_type(float));
-        else if (AP_HOOK_SIG_HAS(sig, RC, double))
-            modeval.v_double = va_arg(ap, va_type(double));
-        else if (AP_HOOK_SIG_HAS(sig, RC, ptr))
-            modeval.v_ptr = va_arg(ap, va_type(ptr));
-    }
-
     if ((he = ap_hook_create(hook)) == NULL)
         return FALSE;
 
@@ -341,9 +324,29 @@
         he->he_modeval = modeval;
     }
 
-    for (i = 0; he->he_func[i] != NULL; i++)
-        if (ap_hook_call_func(ap, he, he->he_func[i]))
+    for (i = 0; he->he_func[i] != NULL; i++) {
+        va_start(ap, modeid);
+
+        if (modeid == AP_HOOK_MODE_DECLINE || modeid == AP_HOOK_MODE_DECLTMP) {
+            if (AP_HOOK_SIG_HAS(sig, RC, char))
+                modeval.v_char = va_arg(ap, va_type(char));
+            else if (AP_HOOK_SIG_HAS(sig, RC, int))
+                modeval.v_int = va_arg(ap, va_type(int));
+            else if (AP_HOOK_SIG_HAS(sig, RC, long))
+                modeval.v_long = va_arg(ap, va_type(long));
+            else if (AP_HOOK_SIG_HAS(sig, RC, float))
+                modeval.v_float = va_arg(ap, va_type(float));
+            else if (AP_HOOK_SIG_HAS(sig, RC, double))
+                modeval.v_double = va_arg(ap, va_type(double));
+            else if (AP_HOOK_SIG_HAS(sig, RC, ptr))
+                modeval.v_ptr = va_arg(ap, va_type(ptr));
+        }
+        if (ap_hook_call_func(ap, he, he->he_func[i])) {
+            va_end(ap);
             break;
+        }
+        va_end(ap);
+    }
 
     if (i > 0 && he->he_modeid == AP_HOOK_MODE_ALL)
         rc = TRUE;
@@ -352,7 +355,6 @@
     else
         rc = TRUE;
 
-    va_end(ap);
     return rc;
 }
 
@@ -366,21 +368,23 @@
     va_list ap;
     int rc;
     
-    va_start(ap, hook);
-
     if ((he = ap_hook_find(hook)) == NULL) {
-        va_end(ap);
         return FALSE;
     }
     if (   he->he_sig == AP_HOOK_SIG_UNKNOWN
         || he->he_modeid == AP_HOOK_MODE_UNKNOWN) {
-        va_end(ap);
         return FALSE;
     }
 
-    for (i = 0; he->he_func[i] != NULL; i++)
-        if (ap_hook_call_func(ap, he, he->he_func[i]))
+    for (i = 0; he->he_func[i] != NULL; i++) {
+        va_start(ap, hook);
+        if (ap_hook_call_func(ap, he, he->he_func[i])) {
+            va_end(ap);
             break;
+        }
+        va_end(ap);
+    }
+
 
     if (i > 0 && he->he_modeid == AP_HOOK_MODE_ALL)
         rc = TRUE;
@@ -389,7 +393,6 @@
     else
         rc = TRUE;
 
-    va_end(ap);
     return rc;
 }

______________________________________________________________________
Apache Interface to OpenSSL (mod_ssl)                   www.modssl.org
User Support Mailing List                      [email-masked]
Automated List Manager                            [email-masked]