ng_bridge(4) has an easily exploitable memory leak

[Julian Elischer]

looks good but:

+               if (destLink == firstLink) { 
+                       /*
+                        * If we've sent all the others, send the
original
+                        * on the first link we found.
+                        */
+                       NG_SEND_DATA(error, destLink->hook, m, meta);
+                       break; /* always done last - not really
needed. */
+               } else {
+                       NG_SEND_DATA(error, destLink->hook, m2, meta2);
+               }


couldn't this be avoided by previously doing:


+               if (linkNum == priv->numLinks) {
+                       /* If we never saw a good link, leave. */
+                       if (firstLink == NULL) {
+                               NG_FREE_DATA(m, meta);
+                               return (0);
+                       }       
+                       destLink = firstLink;
--->                    m2 = m;
--->                    meta2 = meta;
--->                    m=NULL;
--->                    meta=NULL;
+               }


I leave it up to you to decide which you prefer, (but remember that
NG_SEND_DATA is a macro and expads somewhat.

specifically, to (sorry about linewrap):
#define NG_SEND_DATA(error, hook, m, meta)    \
        do {\
                item_p _item;                 \
                if ((_item = ng_package_data((m), (meta)))) {\
                        NG_FWD_ITEM_HOOK(error, _item, hook);    \
                } else { \
                        (error) = ENOMEM;     \   
                }\   
                (m) = NULL;   \
                (meta) = NULL;      \
        } while (0)

where NG_FWD_ITEM_HOOK
itself expands to:
#define NG_FWD_ITEM_HOOK(error, item, hook)   \
        do { \
                (error) = \
                    ng_address_hook(NULL, (item), (hook), 0);   \
                if (error == 0) { \
			SAVE_LINE(item);      \
                        (error) = ng_snd_item((item), 0);   \ 
                } \
                (item) = NULL;         \ 
        } while (0)
 
so only having one of those saves a bit of code.


On Thu, 8 Apr 2004, Ruslan Ermilov wrote:

> On Wed, Apr 07, 2004 at 12:28:38PM -0700, Julian Elischer wrote:
> > On Wed, 7 Apr 2004, Ruslan Ermilov wrote:
> > 
> > > On RELENG_4, ng_bridge(4) has an easily exploitable memory leak,
> > > and may quickly run system out of mbufs.  It's enough to just
> > > have only one link connected to the bridge, e.g., the "upper"
> > > hook of the ng_ether(4) with IP address assigned, and pinging
> > > the broadcast IP address on the interface.  The bug is more
> > > real when constructing a bridge, or, like we experienced it,
> > > by shutting down all except one bridge's link.  The following
> > > patch fixes it:
> > > 
> [snipped]
> 
> > > An alternate solution is to MFC most of ng_bridge.c,v 1.8.  Julian?
> > 
> > what does an MFC diff look like?
> > (bridge is one of archies's nodes)
> > 
> But it was _you_ who masked this bugfix (which would equally apply
> to RELENG_4 as well) under the "SMP preparation" emblem.  ;)
> 
> Anyway, the patch for RELENG_4 is attached.  I've only compile tested
> it.
> 
> 
> Cheers,
> -- 
> Ruslan Ermilov
> ru at FreeBSD.org
> FreeBSD committer
>