va_list reuse in ap_hook.c

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 modssl-users@modssl.org
Automated List Manager majordomo@modssl.org