From 344653ffb7e6cc0dd0263192a117e5bfc3fdc9b9 Mon Sep 17 00:00:00 2001 From: James Carlson Date: Mon, 27 Jun 2005 00:59:57 +0000 Subject: [PATCH] Fixed kernel memory leaks reported by Jin Jiang along with some readability and commenting problems that led to the leaks. --- solaris/ppp.c | 140 ++++++++++++++++++++++++++------------------ solaris/ppp_ahdlc.c | 103 ++++++++++---------------------- 2 files changed, 113 insertions(+), 130 deletions(-) diff --git a/solaris/ppp.c b/solaris/ppp.c index 85d7fc7..676bc26 100644 --- a/solaris/ppp.c +++ b/solaris/ppp.c @@ -32,7 +32,7 @@ * AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING * OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. * - * $Id: ppp.c,v 1.3 2004/11/15 00:57:54 carlsonj Exp $ + * $Id: ppp.c,v 1.4 2005/06/27 00:59:57 carlsonj Exp $ */ /* @@ -276,7 +276,9 @@ static void dlpi_ok __P((queue_t *, int)); static int send_data __P((mblk_t *, upperstr_t *)); static void new_ppa __P((queue_t *, mblk_t *)); static void attach_ppa __P((queue_t *, mblk_t *)); +#ifndef NO_DLPI static void detach_ppa __P((queue_t *, mblk_t *)); +#endif static void detach_lower __P((queue_t *, mblk_t *)); static void debug_dump __P((queue_t *, mblk_t *)); static upperstr_t *find_dest __P((upperstr_t *, int)); @@ -645,11 +647,12 @@ pppuwput(q, mp) break; } #ifdef NO_DLPI + /* pass_packet frees the packet on returning 0 */ if ((us->flags & US_CONTROL) == 0 && !pass_packet(us, mp, 1)) break; #endif - if (!send_data(mp, us)) - putq(q, mp); + if (!send_data(mp, us) && !putq(q, mp)) + freemsg(mp); break; case M_IOCTL: @@ -722,6 +725,7 @@ pppuwput(q, mp) #endif iop->ioc_count = 0; qwriter(q, mp, detach_lower, PERIM_OUTER); + /* mp is now gone */ error = -1; break; @@ -748,6 +752,7 @@ pppuwput(q, mp) iop->ioc_count = sizeof(int); mq->b_wptr = mq->b_rptr + sizeof(int); qwriter(q, mp, new_ppa, PERIM_OUTER); + /* mp is now gone */ error = -1; break; @@ -769,6 +774,7 @@ pppuwput(q, mp) us->ppa = ppa; iop->ioc_count = 0; qwriter(q, mp, attach_ppa, PERIM_OUTER); + /* mp is now gone */ error = -1; break; @@ -851,8 +857,8 @@ pppuwput(q, mp) } n = *(int *)mp->b_cont->b_rptr; if (n == PPPDBG_DUMP + PPPDBG_DRIVER) { - qwriter(q, NULL, debug_dump, PERIM_OUTER); - iop->ioc_count = 0; + qwriter(q, mp, debug_dump, PERIM_OUTER); + /* mp is now gone */ error = -1; } else if (n == PPPDBG_LOG + PPPDBG_DRIVER) { DPRINT1("ppp/%d: debug log enabled\n", us->mn); @@ -863,6 +869,7 @@ pppuwput(q, mp) if (us->ppa == 0 || us->ppa->lowerq == 0) break; putnext(us->ppa->lowerq, mp); + /* mp is now gone */ error = -1; } break; @@ -1010,7 +1017,8 @@ pppuwput(q, mp) ((union DL_primitives *)mq->b_rptr)->dl_primitive = DL_INFO_REQ; mq->b_wptr = mq->b_rptr + sizeof(dl_info_req_t); dlpi_request(q, mq, us); - error = 0; + /* mp is now gone */ + error = -1; break; case SIOCGIFNETMASK: @@ -1345,9 +1353,10 @@ dlpi_request(q, mp, us) mp->b_rptr[1] = PPP_UI; mp->b_rptr[2] = us->sap >> 8; mp->b_rptr[3] = us->sap; + /* pass_packet frees the packet on returning 0 */ if (pass_packet(us, mp, 1)) { - if (!send_data(mp, us)) - putq(q, mp); + if (!send_data(mp, us) && !putq(q, mp)) + freemsg(mp); } return; @@ -1484,6 +1493,9 @@ dlpi_ok(q, prim) } #endif /* NO_DLPI */ +/* + * If return value is 0, then the packet has already been freed. + */ static int pass_packet(us, mp, outbound) upperstr_t *us; @@ -1676,9 +1688,11 @@ attach_ppa(q, mp) #ifndef NO_DLPI dlpi_ok(q, DL_ATTACH_REQ); #endif + freemsg(mp); } } +#ifndef NO_DLPI static void detach_ppa(q, mp) queue_t *q; @@ -1699,11 +1713,11 @@ detach_ppa(q, mp) } us->next = 0; us->ppa = 0; -#ifndef NO_DLPI us->state = DL_UNATTACHED; dlpi_ok(q, DL_DETACH_REQ); -#endif + freemsg(mp); } +#endif /* * We call this with qwriter in order to give the upper queue procedures @@ -1873,53 +1887,54 @@ pppurput(q, mp) putnext(ppa->q, mp); break; - default: - if (mp->b_datap->db_type == M_DATA) { - len = msgdsize(mp); - if (mp->b_wptr - mp->b_rptr < PPP_HDRLEN) { - PULLUP(mp, PPP_HDRLEN); - if (mp == 0) { - DPRINT1("ppp_urput: msgpullup failed (len=%d)\n", len); - break; - } + case M_DATA: + len = msgdsize(mp); + if (mp->b_wptr - mp->b_rptr < PPP_HDRLEN) { + PULLUP(mp, PPP_HDRLEN); + if (mp == 0) { + DPRINT1("ppp_urput: msgpullup failed (len=%d)\n", len); + break; } - MT_ENTER(&ppa->stats_lock); - ppa->stats.ppp_ipackets++; - ppa->stats.ppp_ibytes += len; + } + MT_ENTER(&ppa->stats_lock); + ppa->stats.ppp_ipackets++; + ppa->stats.ppp_ibytes += len; #ifdef INCR_IPACKETS - INCR_IPACKETS(ppa); + INCR_IPACKETS(ppa); #endif - MT_EXIT(&ppa->stats_lock); + MT_EXIT(&ppa->stats_lock); - proto = PPP_PROTOCOL(mp->b_rptr); + proto = PPP_PROTOCOL(mp->b_rptr); #if defined(SOL2) - /* - * Should there be any promiscuous stream(s), send the data - * up for each promiscuous stream that we recognize. - */ - promisc_sendup(ppa, mp, proto, 1); + /* + * Should there be any promiscuous stream(s), send the data + * up for each promiscuous stream that we recognize. + */ + promisc_sendup(ppa, mp, proto, 1); #endif /* defined(SOL2) */ - if (proto < 0x8000 && (us = find_dest(ppa, proto)) != 0) { - /* - * A data packet for some network protocol. - * Queue it on the upper stream for that protocol. - * XXX could we just putnext it? (would require thought) - * The rblocked flag is there to ensure that we keep - * messages in order for each network protocol. - */ - if (!pass_packet(us, mp, 0)) - break; - if (!us->rblocked && !canput(us->q)) - us->rblocked = 1; - if (!us->rblocked) - putq(us->q, mp); - else - putq(q, mp); + if (proto < 0x8000 && (us = find_dest(ppa, proto)) != 0) { + /* + * A data packet for some network protocol. + * Queue it on the upper stream for that protocol. + * XXX could we just putnext it? (would require thought) + * The rblocked flag is there to ensure that we keep + * messages in order for each network protocol. + */ + /* pass_packet frees the packet on returning 0 */ + if (!pass_packet(us, mp, 0)) break; - } + if (!us->rblocked && !canput(us->q)) + us->rblocked = 1; + if (!putq(us->rblocked ? q : us->q, mp)) + freemsg(mp); + break; } + + /* FALLTHROUGH */ + + default: /* * A control frame, a frame for an unknown protocol, * or some other message type. @@ -1927,8 +1942,8 @@ pppurput(q, mp) */ if (queclass(mp) == QPCTL || canputnext(ppa->q)) putnext(ppa->q, mp); - else - putq(q, mp); + else if (!putq(q, mp)) + freemsg(mp); break; } @@ -1980,7 +1995,8 @@ pppursrv(q) if (proto < 0x8000 && (as = find_dest(us, proto)) != 0) { if (!canput(as->q)) break; - putq(as->q, mp); + if (!putq(as->q, mp)) + freemsg(mp); } else { if (!canputnext(q)) break; @@ -2187,11 +2203,12 @@ promisc_sendup(ppa, mp, proto, skip) if (canputnext(prus->q)) { if (prus->flags & US_RAWDATA) { dup_dup_mp = prepend_ether(prus, dup_dup_mp, proto); - putnext(prus->q, dup_dup_mp); } else { dup_dup_mp = prepend_udind(prus, dup_dup_mp, proto); - putnext(prus->q, dup_dup_mp); } + if (dup_dup_mp == 0) + continue; + putnext(prus->q, dup_dup_mp); } else { DPRINT("ppp_urput: data to promisc q dropped\n"); freemsg(dup_dup_mp); @@ -2202,11 +2219,11 @@ promisc_sendup(ppa, mp, proto, skip) if (canputnext(prus->q)) { if (prus->flags & US_RAWDATA) { dup_mp = prepend_ether(prus, dup_mp, proto); - putnext(prus->q, dup_mp); } else { dup_mp = prepend_udind(prus, dup_mp, proto); - putnext(prus->q, dup_mp); } + if (dup_mp != 0) + putnext(prus->q, dup_mp); } else { DPRINT("ppp_urput: data to promisc q dropped\n"); freemsg(dup_mp); @@ -2253,7 +2270,8 @@ ppplrput(q, mp) * rather than blocking, to avoid the possibility of deadlock. */ if (!TRYLOCK_LOWER_R) { - putq(q, mp); + if (!putq(q, mp)) + freemsg(mp); return 0; } @@ -2276,8 +2294,8 @@ ppplrput(q, mp) */ if (queclass(mp) == QPCTL || (qsize(q) == 0 && canput(uq))) put(uq, mp); - else - putq(q, mp); + else if (!putq(q, mp)) + freemsg(mp); UNLOCK_LOWER; return 0; @@ -2411,6 +2429,9 @@ static struct pktfilt_tab { }; +/* + * Packet has already been freed if return value is 0. + */ static int ip_hard_filter(us, mp, outbound) upperstr_t *us; @@ -2484,7 +2505,10 @@ ip_hard_filter(us, mp, outbound) us->mn, pft->proto, pft->port); /* Discard if not connected, or if not pass_with_link_up */ /* else, if link is up let go by, but don't update time */ - return pft->ok_if_link_up? -1: 0; + if (pft->ok_if_link_up) + return -1; + freemsg(mp); + return 0; } break; } /* end switch (proto) */ diff --git a/solaris/ppp_ahdlc.c b/solaris/ppp_ahdlc.c index 50b914a..28920f7 100644 --- a/solaris/ppp_ahdlc.c +++ b/solaris/ppp_ahdlc.c @@ -49,7 +49,7 @@ * AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING * OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. * - * $Id: ppp_ahdlc.c,v 1.4 2004/11/15 00:57:54 carlsonj Exp $ + * $Id: ppp_ahdlc.c,v 1.5 2005/06/27 00:59:57 carlsonj Exp $ */ /* @@ -84,6 +84,14 @@ #define USE_MUTEX #endif /* SOL2 */ +#ifdef USE_MUTEX +#define MUTEX_ENTER(x) mutex_enter(x) +#define MUTEX_EXIT(x) mutex_exit(x) +#else +#define MUTEX_ENTER(x) +#define MUTEX_EXIT(x) +#endif + /* * intpointer_t and uintpointer_t are signed and unsigned integer types * large enough to hold any data pointer; that is, data pointers can be @@ -258,7 +266,7 @@ MOD_OPEN(ahdlc_open) * This can only be opened as a module */ if (sflag != MODOPEN) { - return 0; + OPEN_ERROR(EINVAL); } state = (ahdlc_state_t *) ALLOC_NOSLEEP(sizeof(ahdlc_state_t)); @@ -271,7 +279,6 @@ MOD_OPEN(ahdlc_open) #if defined(USE_MUTEX) mutex_init(&state->lock, NULL, MUTEX_DEFAULT, NULL); - mutex_enter(&state->lock); #endif /* USE_MUTEX */ state->xaccm[0] = ~0; /* escape 0x00 through 0x1f */ @@ -281,10 +288,6 @@ MOD_OPEN(ahdlc_open) state->flag_time = drv_usectohz(FLAG_TIME); #endif /* SOL2 */ -#if defined(USE_MUTEX) - mutex_exit(&state->lock); -#endif /* USE_MUTEX */ - #if defined(SUNOS4) ppp_ahdlc_count++; #endif /* SUNOS4 */ @@ -316,17 +319,12 @@ MOD_CLOSE(ahdlc_close) return 0; } -#if defined(USE_MUTEX) - mutex_enter(&state->lock); -#endif /* USE_MUTEX */ - if (state->rx_buf != 0) { freemsg(state->rx_buf); state->rx_buf = 0; } #if defined(USE_MUTEX) - mutex_exit(&state->lock); mutex_destroy(&state->lock); #endif /* USE_MUTEX */ @@ -386,16 +384,12 @@ ahdlc_wput(q, mp) DPRINT1("ahdlc_wput/%d: PPPIO_XACCM b_cont = 0!\n", state->unit); break; } -#if defined(USE_MUTEX) - mutex_enter(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_ENTER(&state->lock); bcopy((caddr_t)mp->b_cont->b_rptr, (caddr_t)state->xaccm, iop->ioc_count); state->xaccm[2] &= ~0x40000000; /* don't escape 0x5e */ state->xaccm[3] |= 0x60000000; /* do escape 0x7d, 0x7e */ -#if defined(USE_MUTEX) - mutex_exit(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_EXIT(&state->lock); iop->ioc_count = 0; error = 0; break; @@ -407,14 +401,10 @@ ahdlc_wput(q, mp) DPRINT1("ahdlc_wput/%d: PPPIO_RACCM b_cont = 0!\n", state->unit); break; } -#if defined(USE_MUTEX) - mutex_enter(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_ENTER(&state->lock); bcopy((caddr_t)mp->b_cont->b_rptr, (caddr_t)&state->raccm, sizeof(u_int32_t)); -#if defined(USE_MUTEX) - mutex_exit(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_EXIT(&state->lock); iop->ioc_count = 0; error = 0; break; @@ -428,13 +418,9 @@ ahdlc_wput(q, mp) if (mp->b_cont != 0) freemsg(mp->b_cont); mp->b_cont = np; -#if defined(USE_MUTEX) - mutex_enter(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_ENTER(&state->lock); *(int *)np->b_wptr = state->flags & RCV_FLAGS; -#if defined(USE_MUTEX) - mutex_exit(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_EXIT(&state->lock); np->b_wptr += sizeof(int); iop->ioc_count = sizeof(int); error = 0; @@ -483,37 +469,25 @@ ahdlc_wput(q, mp) case M_CTL: switch (*mp->b_rptr) { case PPPCTL_MTU: -#if defined(USE_MUTEX) - mutex_enter(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_ENTER(&state->lock); state->mtu = ((unsigned short *)mp->b_rptr)[1]; -#if defined(USE_MUTEX) - mutex_exit(&state->lock); -#endif /* USE_MUTEX */ - freemsg(mp); + MUTEX_EXIT(&state->lock); break; case PPPCTL_MRU: -#if defined(USE_MUTEX) - mutex_enter(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_ENTER(&state->lock); state->mru = ((unsigned short *)mp->b_rptr)[1]; -#if defined(USE_MUTEX) - mutex_exit(&state->lock); -#endif /* USE_MUTEX */ - freemsg(mp); + MUTEX_EXIT(&state->lock); break; case PPPCTL_UNIT: -#if defined(USE_MUTEX) - mutex_enter(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_ENTER(&state->lock); state->unit = mp->b_rptr[1]; -#if defined(USE_MUTEX) - mutex_exit(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_EXIT(&state->lock); break; default: putnext(q, mp); + return 0; } + freemsg(mp); break; default: @@ -546,18 +520,14 @@ ahdlc_rput(q, mp) break; case M_HANGUP: -#if defined(USE_MUTEX) - mutex_enter(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_ENTER(&state->lock); if (state->rx_buf != 0) { /* XXX would like to send this up for debugging */ freemsg(state->rx_buf); state->rx_buf = 0; } state->flags = IFLUSH; -#if defined(USE_MUTEX) - mutex_exit(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_EXIT(&state->lock); putnext(q, mp); break; @@ -593,9 +563,7 @@ ahdlc_encode(q, mp) } state = (ahdlc_state_t *)q->q_ptr; -#if defined(USE_MUTEX) - mutex_enter(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_ENTER(&state->lock); /* * Allocate an output buffer large enough to handle a case where all @@ -608,9 +576,7 @@ ahdlc_encode(q, mp) outmp = allocb(outmp_len, BPRI_MED); if (outmp == NULL) { state->stats.ppp_oerrors++; -#if defined(USE_MUTEX) - mutex_exit(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_EXIT(&state->lock); putctl1(RD(q)->q_next, M_CTL, PPPCTL_OERROR); return; } @@ -704,12 +670,9 @@ ahdlc_encode(q, mp) state->stats.ppp_obytes += msgdsize(outmp); state->stats.ppp_opackets++; -#if defined(USE_MUTEX) - mutex_exit(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_EXIT(&state->lock); putnext(q, outmp); - return; } /* @@ -733,9 +696,7 @@ ahdlc_decode(q, mp) state = (ahdlc_state_t *) q->q_ptr; -#if defined(USE_MUTEX) - mutex_enter(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_ENTER(&state->lock); state->stats.ppp_ibytes += msgdsize(mp); @@ -856,9 +817,7 @@ ahdlc_decode(q, mp) } } -#if defined(USE_MUTEX) - mutex_exit(&state->lock); -#endif /* USE_MUTEX */ + MUTEX_EXIT(&state->lock); } static int -- 2.39.2