David CARLIER (2015-11-21 18:24 +0100):
> As a first message, I wanted to send a little patch which is all about
> use-after-free if fine with you first in mta???s part when a route might be
> totally discarded when disabled, the other changes are queue/tree element
> removals rearrangements.
> Index: bounce.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v
> retrieving revision 1.67
> diff -u -p -r1.67 bounce.c
> --- bounce.c 7 Oct 2015 19:25:42 -0000 1.67
> +++ bounce.c 20 Nov 2015 16:23:14 -0000
> @@ -536,13 +536,14 @@ bounce_next(struct bounce_session *s)
> static void
> bounce_delivery(struct bounce_message *msg, int delivery, const char *status)
> {
> - struct bounce_envelope *be;
> + struct bounce_envelope *be, *sbe;
> struct envelope evp;
> size_t n;
> const char *f;
>
> n = 0;
> - while ((be = TAILQ_FIRST(&msg->envelopes))) {
> + be = TAILQ_FIRST(&msg->envelopes);
> + while (be) {
> if (delivery == IMSG_QUEUE_DELIVERY_TEMPFAIL) {
> if (queue_envelope_load(be->id, &evp) == 0) {
> fatalx("could not reload envelope!");
> @@ -560,9 +561,11 @@ bounce_delivery(struct bounce_message *m
> m_close(p_scheduler);
> queue_envelope_delete(be->id);
> }
> + sbe = TAILQ_NEXT(be, entry);
> TAILQ_REMOVE(&msg->envelopes, be, entry);
> free(be->report);
> free(be);
> + be = sbe;
> n += 1;
> }
There is no use-after-free here. This is what happens essentially:
while ((elem = TAILQ_FIRST(&list_head)) != NULL) {
TAILQ_REMOVE(&list_head, elem, entry);
free(elem);
}
The loop requests the first element and removes it. TAILQ_REMOVE() will
ensure that the second element in the list becomes the new first
element. Then the loop requests the new first element and removes it,
etc.
This also applies to the other list/tree removals in your diff.
The queue and tree APIs are described here:
http://www.openbsd.org/cgi-bin/man.cgi?query=queue
http://www.openbsd.org/cgi-bin/man.cgi?query=tree
> Index: expand.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/expand.c,v
> retrieving revision 1.28
> diff -u -p -r1.28 expand.c
> --- expand.c 20 Jan 2015 17:37:54 -0000 1.28
> +++ expand.c 20 Nov 2015 16:23:14 -0000
> @@ -100,16 +100,24 @@ expand_insert(struct expand *expand, str
> void
> expand_clear(struct expand *expand)
> {
> - struct expandnode *xn;
> + struct expandnode *xn, *xnb;
>
> log_trace(TRACE_EXPAND, "expand: %p: clearing expand tree", expand);
> - if (expand->queue)
> - while ((xn = TAILQ_FIRST(expand->queue)))
> + if (expand->queue) {
> + xn = TAILQ_FIRST(expand->queue);
> + while (xn) {
> + xnb = TAILQ_NEXT(xn, tq_entry);
> TAILQ_REMOVE(expand->queue, xn, tq_entry);
> + xn = xnb;
> + }
> + }
>
> - while ((xn = RB_ROOT(&expand->tree)) != NULL) {
> + xn = RB_ROOT(&expand->tree);
> + while (xn != NULL) {
> + xnb = RB_NEXT(expandtree, &expand->tree, xn);
> RB_REMOVE(expandtree, &expand->tree, xn);
> free(xn);
> + xn = xnb;
> }
> }
>
> Index: mta.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/mta.c,v
> retrieving revision 1.192
> diff -u -p -r1.192 mta.c
> --- mta.c 14 Oct 2015 22:01:43 -0000 1.192
> +++ mta.c 20 Nov 2015 16:23:14 -0000
> @@ -117,7 +117,7 @@ static const char *mta_connector_to_text
> SPLAY_HEAD(mta_route_tree, mta_route);
> static struct mta_route *mta_route(struct mta_source *, struct mta_host *);
> static void mta_route_ref(struct mta_route *);
> -static void mta_route_unref(struct mta_route *);
> +static int mta_route_unref(struct mta_route *);
> static const char *mta_route_to_text(struct mta_route *);
> static int mta_route_cmp(const struct mta_route *, const struct mta_route *);
> SPLAY_PROTOTYPE(mta_route_tree, mta_route, entry, mta_route_cmp);
> @@ -1248,7 +1248,8 @@ mta_route_disable(struct mta_route *rout
>
> if (route->flags & ROUTE_DISABLED) {
> runq_cancel(runq_route, NULL, route);
> - mta_route_unref(route); /* from last call to here */
> + if (mta_route_unref(route) == 1) /* from last call to here */
> + route = calloc(1, sizeof(*route));
> }
> route->flags |= reason & ROUTE_DISABLED;
> runq_schedule(runq_route, time(NULL) + delay, NULL, route);
> @@ -2253,14 +2254,14 @@ mta_route_ref(struct mta_route *r)
> r->refcount++;
> }
>
> -static void
> +static int
> mta_route_unref(struct mta_route *r)
> {
> time_t sched, now;
> int delay;
>
> if (--r->refcount)
> - return;
> + return 0;
>
> /*
> * Nothing references this route, but we might want to keep it alive
> @@ -2296,7 +2297,7 @@ mta_route_unref(struct mta_route *r)
> r->flags |= ROUTE_RUNQ;
> runq_schedule(runq_route, sched, NULL, r);
> r->refcount++;
> - return;
> + return 0;
> }
>
> log_debug("debug: mta: mta_route_unref(): really discarding route %s",
> @@ -2307,6 +2308,7 @@ mta_route_unref(struct mta_route *r)
> mta_host_unref(r->dst); /* from constructor */
> free(r);
> stat_decrement("mta.route", 1);
> + return 1;
> }
That won't work I'm afraid. I think the following diff does, but I'll
have to defer this to the OpenSMTPD developers.
Index: mta.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/mta.c,v
retrieving revision 1.192
diff -p -u -r1.192 mta.c
--- mta.c 14 Oct 2015 22:01:43 -0000 1.192
+++ mta.c 22 Nov 2015 19:48:24 -0000
@@ -1246,13 +1246,13 @@ mta_route_disable(struct mta_route *rout
log_info("smtp-out: Disabling route %s for %llus",
mta_route_to_text(route), delay);
- if (route->flags & ROUTE_DISABLED) {
+ if (!(route->flags & ROUTE_DISABLED))
+ mta_route_ref(route);
+ else
runq_cancel(runq_route, NULL, route);
- mta_route_unref(route); /* from last call to here */
- }
+
route->flags |= reason & ROUTE_DISABLED;
runq_schedule(runq_route, time(NULL) + delay, NULL, route);
- mta_route_ref(route);
}
static void
>
> static const char *
> Index: rfc2822.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/smtpd/rfc2822.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 rfc2822.c
> --- rfc2822.c 5 Nov 2015 08:55:09 -0000 1.5
> +++ rfc2822.c 20 Nov 2015 16:23:14 -0000
> @@ -71,12 +71,15 @@ end:
> static void
> missing_headers_callback(struct rfc2822_parser *rp)
> {
> - struct rfc2822_hdr_miss_cb *hdr_miss_cb;
> + struct rfc2822_hdr_miss_cb *hdr_miss_cb, *hdr_miss_cbs;
>
> - while ((hdr_miss_cb = TAILQ_FIRST(&rp->hdr_miss_cb))) {
> + hdr_miss_cb = TAILQ_FIRST(&rp->hdr_miss_cb);
> + while (hdr_miss_cb) {
> hdr_miss_cb->func(hdr_miss_cb->name, hdr_miss_cb->arg);
> + hdr_miss_cbs = TAILQ_NEXT(hdr_miss_cb, next);
> TAILQ_REMOVE(&rp->hdr_miss_cb, hdr_miss_cb, next);
> free(hdr_miss_cb);
> + hdr_miss_cb = hdr_miss_cbs;
> }
> }
--
You received this mail because you are subscribed to [email protected]
To unsubscribe, send a mail to: [email protected]
