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