On Thu, 24 May 2012 16:16:02 +0200, Kenneth R Westerback
<kwesterb...@rogers.com> wrote:
On Thu, May 24, 2012 at 01:54:36PM +0200, Gerhard Roth wrote:
Hi,
with the OPENBSD-CARP-MIB a memory leak was introduced to snmpd(8):
RCS file: mib.c,v
retrieving revision 1.52
diff -u -p -r1.52 mib.c
--- mib.c 2012/03/20 03:01:26 1.52
+++ mib.c 2012/05/24 12:53:35
@@ -2713,7 +2713,7 @@ mib_carpiftable(struct oid *oid, struct ber_oid
*o,
st
/* Get and verify the current row index */
idx = o->bo_id[OIDIDX_carpIfEntry];
- if ((cif = mib_carpifget(cif, idx)) == NULL) {
+ if (mib_carpifget(cif, idx) == NULL) {
free(cif);
return (1);
}
Gerhard
Calling mib_carpget() seems a tad over complex. Wouldn't the diff
below make it cleaner? Untested except by gcc.
And doesn't the socket 's' leak too, or does SIOCGVH returning -1
mean 's' was closed?
.... Ken
Index: mib.c
===================================================================
RCS file: /cvs/src/usr.sbin/snmpd/mib.c,v
retrieving revision 1.52
diff -u -p -r1.52 mib.c
--- mib.c 20 Mar 2012 03:01:26 -0000 1.52
+++ mib.c 24 May 2012 14:11:22 -0000
@@ -1360,7 +1360,7 @@ int mib_carpstats(struct oid *, struct
int mib_carpiftable(struct oid *, struct ber_oid *, struct ber_element
**);
int mib_carpifnum(struct oid *, struct ber_oid *, struct ber_element
**);
struct carpif
- *mib_carpifget(struct carpif *, u_int);
+ *mib_carpifget(u_int);
int mib_memiftable(struct oid *, struct ber_oid *, struct ber_element
**);
static struct oid openbsd_mib[] = {
@@ -2647,9 +2647,10 @@ mib_carpifnum(struct oid *oid, struct be
}
struct carpif *
-mib_carpifget(struct carpif *cif, u_int idx)
+mib_carpifget(u_int idx)
{
struct kif *kif;
+ struct carpif *cif;
int s;
struct ifreq ifr;
struct carpreq carpr;
@@ -2692,9 +2693,12 @@ mib_carpifget(struct carpif *cif, u_int
if (ioctl(s, SIOCGVH, (caddr_t)&ifr) == -1)
return (NULL);
- memset(cif, 0, sizeof(struct carpif));
- memcpy(&cif->carpr, &carpr, sizeof(struct carpreq));
- memcpy(&cif->kif, kif, sizeof(struct kif));
+ cif = malloc(sizeof(struct carpif));
+ if (cif != NULL) {
+ memset(cif, 0, sizeof(struct carpif));
+ memcpy(&cif->carpr, &carpr, sizeof(struct carpreq));
+ memcpy(&cif->kif, kif, sizeof(struct kif));
+ }
close(s);
@@ -2707,16 +2711,11 @@ mib_carpiftable(struct oid *oid, struct
u_int32_t idx;
struct carpif *cif;
- if ((cif = malloc(sizeof(struct carpif))) == NULL)
- return (1);
-
/* Get and verify the current row index */
idx = o->bo_id[OIDIDX_carpIfEntry];
- if ((cif = mib_carpifget(cif, idx)) == NULL) {
- free(cif);
+ if ((cif = mib_carpifget(idx)) == NULL)
return (1);
- }
/* Tables need to prepend the OID on their own */
o->bo_id[OIDIDX_carpIfEntry] = cif->kif.if_index;
Hi Ken,
I like your patch; it's better than mine.
And you are right about the socket fd leak. If an ioctl()
would close the filedes passed in that would be a very
awkward API.
Gerhard
--
GeNUA, Gesellschaft fC<r Netzwerk- und Unix-Administration mbH
Domagkstrasse 7, 85551 Kirchheim bei Muenchen
tel +49 89 991950-0, fax -999, www.genua.de
Geschaeftsfuehrer: Dr. Magnus Harlander, Dr. Michaela Harlander,
Bernhard Schneck. Amtsgericht Muenchen HRB 98238
