On Sun, Mar 08, 2009 at 09:52:26AM +1100, Max Matveev wrote:
> On Sat, 7 Mar 2009 12:18:07 -0500, Martin Hicks wrote:
>
> mort> On Sun, Mar 08, 2009 at 12:50:39AM +1100, Max Matveev wrote:
> >> On Fri, 6 Mar 2009 12:27:31 -0600, Martin Hicks wrote:
> >>
> >> Strictly speaking you cannot rely on umad_release_port() not to stuff
> >> ca_name and portnum, the fact that it does not do it now, just like
> >> the fact that it didn't do any allocation at all in 1.2 timeframe,
> >> does not mean you're not going to get into some weird use-after-free
> >> next time OFED people change the library.
>
> mort> I realize that I'm exploiting a particular behaviour in the ibumad
> mort> library. I've also filed a bug against the ibumad library and
> hopefully
> mort> someone will fix that in a future release.
> You can add ca_name and portnum into the local_port_t and make release/get
> independent of the shenanigans of libibumad.
>
One thing that concerned me while fixing this up is that madrpc_init()
is never called, but the mad_rpc_open_port() is used. The PMDA seems to
do most of what madrpc_init() does except for registering the RPC
mad_register_client() stuff.
Here's the changes you requested. Does this look better?
mh
diff --git a/src/pmdas/ib/ib.c b/src/pmdas/ib/ib.c
index cec3694..ccfb079 100644
--- a/src/pmdas/ib/ib.c
+++ b/src/pmdas/ib/ib.c
@@ -38,6 +38,16 @@
#define IBPMDA_MAX_HCAS (16)
typedef struct local_port_s {
+ /*
+ * Cache the ca_name and portnum to avoid a bug in libibumad that
+ * leaks memory when umad_port_get() is called over and over.
+ * With ca_name and portnum we can safely do umad_port_release()
+ * first and then umad_port_get() without fear that some future
+ * version of release() will deallocate port->ca_name and
+ * port->portnum.
+ */
+ char ca_name[UMAD_CA_NAME_LEN];
+ int portnum;
umad_port_t *ump;
void * hndl;
int needsupdate;
@@ -172,15 +182,18 @@ static void
openumadport (hca_state_t *hst, umad_port_t *port, void *arg)
{
void *hndl = arg;
+ local_port_t *lp;
if ((hndl = mad_rpc_open_port(port->ca_name, port->portnum, mgmt_classes,
ARRAYSZ(mgmt_classes))) == NULL) {
__pmNotifyErr(LOG_ERR, "Cannot open port handle for %s:%d\n",
port->ca_name, port->portnum);
}
-
- hst->lports[port->portnum].hndl = hndl;
- hst->lports[port->portnum].ump = port;
+ lp = &hst->lports[port->portnum];
+ strcpy(lp->ca_name, port->ca_name);
+ lp->portnum = port->portnum;
+ lp->ump = port;
+ lp->hndl = hndl;
}
int
@@ -504,9 +517,8 @@ ib_fetch_val(pmdaMetric *mdesc, unsigned int inst,
pmAtomValue *atom)
/* The state of the local port used for queries is checked
* once per fetch request */
if (lp->needsupdate) {
- umad_release_port(lp->ump);
- if (umad_get_port (lp->ump->ca_name, lp->ump->portnum,
- lp->ump) != 0) {
+ umad_release_port(lp->ump);
+ if (umad_get_port(lp->ca_name, lp->portnum, lp->ump) != 0) {
__pmNotifyErr (LOG_ERR,
"Cannot get state of the port %s:%d\n",
lp->ump->ca_name, lp->ump->portnum);
|