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 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
>
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;
