This case is covered by the new regress' backend_get_toofew and
backend_get_toomany tests. However, even with MALLOC_OPTIONS cranked
to the max it's really hard to trigger (I had to run
backend_get_wrongorder, backend_get_toofew, backend_get_toomany
sequentially in a tight loop killing snmpd between iterations for the
best chance).
When we receive an invalid varbindlist in a response we set the invalid
variable. This in turn calls appl_varbind_error(), but the avi_state
of the varbinds remains in APPL_VBSTATE_PENDING. Directly following
we call appl_request_downstream_free(), which sees that the varbinds
haven't been resolved, triggering a call to
appl_request_upstream_resolve(). This call in turn sees that the
error has been set and just sends out the error-response to the client
and frees the appl_request_upstream. From here we return back to
appl_response(), which also calls appl_request_upstream_resolve(),
resulting in a use after free.
The main tool for fixing this issue is making use of
appl_request_upstream's aru_locked member, which will cause
appl_request_upstream_resolve() to return instantly. The simplest fix is
to set aru_locked before calling appl_request_downstream_free() and
unsetting it directly afterwards inside appl_response().
The second one is the diff proposed below, which shrinks the code.
appl_request_upstream_free() is only called once from
appl_request_upstream_reply(). appl_request_upstream_reply() in turn
is only called by appl_request_upstream_resolve().
appl_request_upstream_resolve() is called in 3 places:
- appl_processpdu(): to kick things off
- appl_request_downstream_free(): For when a backend disappears with
outstanding requests
- appl_response(): To kickstart the next round of resolving.
Since appl_request_downstream_free() is always called from
appl_response(), we can leverage that function and make it call
appl_request_upstream_resolve() unconditionally.
appl_request_downstream_free() is called from the following locations:
- appl_close(): When a backend has disappeared.
- appl_request_upstream_free(): We send out a reply early, because an
error has been detected.
- appl_response(): We received a response
appl_request_upstream_free() can't reenter into
appl_request_upstream_resolve(), or it would potentially trigger new
appl_request_downstreams. This can be prevented by setting aru_locked
before calling appl_request_downstream_free().
For all other cases we should rely on appl_request_upstream_resolve()'s
logic to handle varbinds in any state, so there's no reason make calls
from other contexts conditional.
OK?
martijn@
Index: application.c
===================================================================
RCS file: /cvs/src/usr.sbin/snmpd/application.c,v
retrieving revision 1.24
diff -u -p -r1.24 application.c
--- application.c 24 Oct 2023 14:21:58 -0000 1.24
+++ application.c 26 Oct 2023 09:40:23 -0000
@@ -710,6 +710,7 @@ appl_request_upstream_free(struct appl_r
if (ureq == NULL)
return;
+ ureq->aru_locked = 1;
for (i = 0; i < ureq->aru_varbindlen && ureq->aru_vblist != NULL; i++) {
vb = &(ureq->aru_vblist[i]);
ober_free_elements(vb->avi_varbind.av_value);
@@ -726,7 +727,6 @@ void
appl_request_downstream_free(struct appl_request_downstream *dreq)
{
struct appl_varbind_internal *vb;
- int retry = 0;
if (dreq == NULL)
return;
@@ -736,14 +736,11 @@ appl_request_downstream_free(struct appl
for (vb = dreq->ard_vblist; vb != NULL; vb = vb->avi_next) {
vb->avi_request_downstream = NULL;
- if (vb->avi_state == APPL_VBSTATE_PENDING) {
+ if (vb->avi_state == APPL_VBSTATE_PENDING)
vb->avi_state = APPL_VBSTATE_NEW;
- retry = 1;
- }
}
- if (retry)
- appl_request_upstream_resolve(dreq->ard_request);
+ appl_request_upstream_resolve(dreq->ard_request);
free(dreq);
}
@@ -1172,9 +1169,6 @@ appl_response(struct appl_backend *backe
backend->ab_name);
backend->ab_fn->ab_close(backend, APPL_CLOSE_REASONPARSEERROR);
}
-
- if (ureq != NULL)
- appl_request_upstream_resolve(ureq);
}
int
