Re: Memory leak in snmpd(8)
On Thu, May 24, 2012 at 8:16 AM, Kenneth R Westerback
kwesterb...@rogers.com wrote:
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,
Your diff looks good to me. I also found a few spots that leak in
pf.c. All combined I've come up with this diff. Ok?
Index: src/usr.sbin/snmpd/mib.c
===
RCS file: /cvs/src/usr.sbin/snmpd/mib.c,v
retrieving revision 1.52
diff -p -u -r1.52 mib.c
--- src/usr.sbin/snmpd/mib.c20 Mar 2012 03:01:26 - 1.52
+++ src/usr.sbin/snmpd/mib.c26 May 2012 05:14:36 -
@@ -1360,7 +1360,7 @@ intmib_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;
@@ -2689,12 +2690,17 @@ mib_carpifget(struct carpif *cif, u_int
memset((char *)carpr, 0, sizeof(carpr));
ifr.ifr_data = (caddr_t)carpr;
- if (ioctl(s, SIOCGVH, (caddr_t)ifr) == -1)
+ if (ioctl(s, SIOCGVH, (caddr_t)ifr) == -1) {
+ close(s);
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 +2713,11 @@ mib_carpiftable(struct oid *oid, struct
u_int32_tidx;
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;
Index: src/usr.sbin/snmpd/pf.c
===
RCS file: /cvs/src/usr.sbin/snmpd/pf.c,v
retrieving revision 1.2
diff -p -u -r1.2 pf.c
--- src/usr.sbin/snmpd/pf.c 14 May 2012 00:02:33 - 1.2
+++ src/usr.sbin/snmpd/pf.c 26 May 2012 05:22:04 -
@@ -228,8 +228,10 @@ pfi_count(void)
struct pfi_kif *p;
int c = 0;
- if (pfi_get(b, NULL))
+ if (pfi_get(b, NULL)) {
+ free(b.pfrb_caddr);
return (-1);
+ }
PFRB_FOREACH(p, b)
c++;
@@ -245,8 +247,10 @@ pfi_get_if(struct pfi_kif *rp, int idx)
struct pfi_kif *p;
int i = 1;
- if (pfi_get(b, NULL))
+ if (pfi_get(b, NULL)) {
+ free(b.pfrb_caddr);
return (-1);
+ }
PFRB_FOREACH(p, b) {
if (i == idx)
@@ -290,9 +294,11 @@ pft_get_table(struct pfr_tstats *rts, in
struct pfr_tstats *ts;
int i = 1;
- if (pft_get(b, NULL))
+ if (pft_get(b, NULL)) {
+ free(b.pfrb_caddr);
return (-1);
-
+ }
+
PFRB_FOREACH(ts, b) {
if (!(ts-pfrts_flags PFR_TFLAG_ACTIVE))
continue;
@@ -319,8 +325,10 @@ pft_count(void)
struct pfr_tstats *ts;
int c = 0;
- if (pft_get(b, NULL))
+ if (pft_get(b, NULL)) {
+ free(b.pfrb_caddr);
return (-1);
+ }
PFRB_FOREACH(ts, b) {
if (!(ts-pfrts_flags PFR_TFLAG_ACTIVE))
Memory leak in snmpd(8)
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
--
GeNUA, Gesellschaft fCr 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
Re: Memory leak in snmpd(8)
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
--
GeNUA, Gesellschaft fCr 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
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 - 1.52
+++ mib.c 24 May 2012 14:11:22 -
@@ -1360,7 +1360,7 @@ intmib_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_tidx;
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;
Re: Memory leak in snmpd(8)
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 - 1.52
+++ mib.c 24 May 2012 14:11:22 -
@@ -1360,7 +1360,7 @@ intmib_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_tidx;
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 fCr 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
