Fixed kernel memory leaks reported by Jin Jiang along with some
authorJames Carlson <carlsonj@workingcode.com>
Mon, 27 Jun 2005 00:59:57 +0000 (00:59 +0000)
committerJames Carlson <carlsonj@workingcode.com>
Mon, 27 Jun 2005 00:59:57 +0000 (00:59 +0000)
readability and commenting problems that led to the leaks.

solaris/ppp.c
solaris/ppp_ahdlc.c

index 85d7fc70643b78f4b30f075d76b95755a9fc9d4d..676bc2644f1a88113d1a3c16f2b0408814ca6926 100644 (file)
@@ -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) */
index 50b914a799ab21d181ab868800cb245ffa5ed6eb..28920f716a6fffd1c23f0e7358f7c885eadd2cb1 100644 (file)
@@ -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 $
  */
 
 /*
 #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