Re: net/wmnetload has a memory leak [patch]

2018-12-20 Thread Jeremie Courreges-Anglas 
On Wed, Dec 19 2018, Ed Hynan  wrote:
> On Wed, 19 Dec 2018, Jeremie Courreges-Anglas wrote:
>> On Wed, Dec 19 2018, Ed Hynan  wrote:
>> > net/wmnetload has a memory leak [patch]
>> >
>> > wmnetload (6.4 release) repeatedly calls getifaddrs(3)
>> > without freeifaddrs.
>> 
>> Indeed.
>> 
>> > The patch here fixes and would replace
>> > net/wmnetload/patches/patch-src_ifstat_openbsd_c
>> > not apply over it.
>> 
>> Thanks.  If you want to make it easier for people to pick up your
>> patches, please try to submit patches using make update-patches and cvs
>> diff.  For more information on how to use update-patches, see item 11:
>> 
>>   https://www.openbsd.org/faq/ports/guide.html
>> 
>> Since the fix changes what ends up in the package, it should include
>> a REVISION bump.
>
> OK.  I was concerned that I have 6.4 release and my ports are
> cvs checkout -P -rOPENBSD_6_4 ports -- is this what ports folks
> are working on?

Nope, except for security backports on -stable, development happens
on -current.

[...]

>> Could you please test this and report back?
>
> It's fine here.
>
> (I see your latest msg re. -w; pardon my late response.)

Your response was not late, I was able to move fast thanks to Stuart. :)

Thanks for confirming that it works for you.

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: net/wmnetload has a memory leak [patch]

2018-12-19 Thread Ed Hynan 
On Wed, 19 Dec 2018, Jeremie Courreges-Anglas wrote:
> On Wed, Dec 19 2018, Ed Hynan  wrote:
> > net/wmnetload has a memory leak [patch]
> >
> > wmnetload (6.4 release) repeatedly calls getifaddrs(3)
> > without freeifaddrs.
> 
> Indeed.
> 
> > The patch here fixes and would replace
> > net/wmnetload/patches/patch-src_ifstat_openbsd_c
> > not apply over it.
> 
> Thanks.  If you want to make it easier for people to pick up your
> patches, please try to submit patches using make update-patches and cvs
> diff.  For more information on how to use update-patches, see item 11:
> 
>   https://www.openbsd.org/faq/ports/guide.html
> 
> Since the fix changes what ends up in the package, it should include
> a REVISION bump.

OK.  I was concerned that I have 6.4 release and my ports are
cvs checkout -P -rOPENBSD_6_4 ports -- is this what ports folks
are working on? (probably wouldn't matter with this package, but...)

> 
> > Also fixes a compile warning from strcmp() w/o 
> 
> Yep.
> 
> Here's a reworked version, not directly based on your diff.  There's no
> need to keep around what getifaddrs(3) returns, we can always free it.
> This simplifies the memory management and appears to fix the memory leak
> in the current package.  I have no idea if wmnetload actually works, it
> seems to just draw an empty black window under my window manager.
> 
> Could you please test this and report back?

It's fine here.

(I see your latest msg re. -w; pardon my late response.)

-Ed Hynan

[snip quoted patch]

Re: net/wmnetload has a memory leak [patch]

2018-12-19 Thread Jeremie Courreges-Anglas 
On Wed, Dec 19 2018, Jeremie Courreges-Anglas  wrote:
> On Wed, Dec 19 2018, Ed Hynan  wrote:
>> net/wmnetload has a memory leak [patch]
>>
>> wmnetload (6.4 release) repeatedly calls getifaddrs(3)
>> without freeifaddrs.
>
> Indeed.
>
>> The patch here fixes and would replace
>> net/wmnetload/patches/patch-src_ifstat_openbsd_c
>> not apply over it.
>
> Thanks.  If you want to make it easier for people to pick up your
> patches, please try to submit patches using make update-patches and cvs
> diff.  For more information on how to use update-patches, see item 11:
>
>   https://www.openbsd.org/faq/ports/guide.html
>
> Since the fix changes what ends up in the package, it should include
> a REVISION bump.
>
>> Also fixes a compile warning from strcmp() w/o 
>
> Yep.
>
> Here's a reworked version, not directly based on your diff.  There's no
> need to keep around what getifaddrs(3) returns, we can always free it.
> This simplifies the memory management and appears to fix the memory leak
> in the current package.  I have no idea if wmnetload actually works, it
> seems to just draw an empty black window under my window manager.
>
> Could you please test this and report back?

Thanks to sthen@ I could test that the port still behaves properly (need
to use -w to run in windowed mode).  The fix has been committed.

Thanks,
-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: net/wmnetload has a memory leak [patch]

2018-12-19 Thread Jeremie Courreges-Anglas 
On Wed, Dec 19 2018, Ed Hynan  wrote:
> net/wmnetload has a memory leak [patch]
>
> wmnetload (6.4 release) repeatedly calls getifaddrs(3)
> without freeifaddrs.

Indeed.

> The patch here fixes and would replace
> net/wmnetload/patches/patch-src_ifstat_openbsd_c
> not apply over it.

Thanks.  If you want to make it easier for people to pick up your
patches, please try to submit patches using make update-patches and cvs
diff.  For more information on how to use update-patches, see item 11:

  https://www.openbsd.org/faq/ports/guide.html

Since the fix changes what ends up in the package, it should include
a REVISION bump.

> Also fixes a compile warning from strcmp() w/o 

Yep.

Here's a reworked version, not directly based on your diff.  There's no
need to keep around what getifaddrs(3) returns, we can always free it.
This simplifies the memory management and appears to fix the memory leak
in the current package.  I have no idea if wmnetload actually works, it
seems to just draw an empty black window under my window manager.

Could you please test this and report back?


Index: Makefile
===
RCS file: /cvs/ports/net/wmnetload/Makefile,v
retrieving revision 1.13
diff -u -p -r1.13 Makefile
--- Makefile7 Dec 2015 14:58:54 -   1.13
+++ Makefile19 Dec 2018 18:29:20 -
@@ -3,7 +3,7 @@
 COMMENT=   wm-dockapp; simple network interface monitoring tool
 
 DISTNAME=  wmnetload-1.3
-REVISION=  3
+REVISION=  4
 CATEGORIES=net x11 x11/windowmaker
 
 HOMEPAGE=  http://freshmeat.net/projects/wmnetload
Index: patches/patch-src_ifstat_openbsd_c
===
RCS file: /cvs/ports/net/wmnetload/patches/patch-src_ifstat_openbsd_c,v
retrieving revision 1.2
diff -u -p -r1.2 patch-src_ifstat_openbsd_c
--- patches/patch-src_ifstat_openbsd_c  7 Dec 2015 14:58:54 -   1.2
+++ patches/patch-src_ifstat_openbsd_c  19 Dec 2018 18:29:20 -
@@ -2,9 +2,10 @@ $OpenBSD: patch-src_ifstat_openbsd_c,v 1
 
 use getifaddrs(3) instead of libkvm.
 
 src/ifstat_openbsd.c.orig  Tue Jan 29 09:09:18 2002
-+++ src/ifstat_openbsd.c   Mon Dec  7 14:31:38 2015
-@@ -27,19 +27,14 @@
+Index: src/ifstat_openbsd.c
+--- src/ifstat_openbsd.c.orig
 src/ifstat_openbsd.c
+@@ -27,10 +27,7 @@
  #include 
  #include 
  #include 
@@ -14,19 +15,24 @@ use getifaddrs(3) instead of libkvm.
 -#include 
 +#include 
  #include 
--#include 
+ #include 
  
- #include "ifstat.h"
+@@ -38,21 +35,16 @@
  #include "utils.h"
  
  struct ifstatstate {
 -  void*ifnet_head;
 -  kvm_t   *kd;
-+  struct ifaddrs *ifap;
++  int unused;
  };
  
  /*
-@@ -51,8 +46,6 @@ ifstatstate_t *
+- * Do one-time setup stuff for accessing the interface statistics and store
+- * the gathered information in an interface statistics state structure.
+- * Return the state structure.
++ * Return a dummy state structure.
+  */
+ ifstatstate_t *
  if_statinit(void)
  {
ifstatstate_t   *statep;
@@ -35,7 +41,7 @@ use getifaddrs(3) instead of libkvm.
  
statep = malloc(sizeof (ifstatstate_t));
if (statep == NULL) {
-@@ -60,35 +53,8 @@ if_statinit(void)
+@@ -60,63 +52,44 @@ if_statinit(void)
return (NULL);
}
  
@@ -66,60 +72,67 @@ use getifaddrs(3) instead of libkvm.
 -  }
 -
return (statep);
- fail:
+-fail:
 -  (void) kvm_close(statep->kd);
-   free(statep);
-   return (NULL);
+-  free(statep);
+-  return (NULL);
  }
-@@ -100,22 +66,33 @@ fail:
+ 
+ /*
+- * Optionally using state stored in `statep', retrieve stats on interface
+- * `ifname', and store the statistics in `ifstatsp'.
++ * Retrieve stats on interface `ifname', and store the statistics in 
`ifstatsp'.
+  */
  int
  if_stats(const char *ifname, ifstatstate_t *statep, ifstats_t *ifstatsp)
  {
 -  void*ifnet_addr = statep->ifnet_head;
 -  struct ifnetifnet;
-+  struct ifaddrs *ifa;
++  struct ifaddrs  *ifa0, *ifa;
++  int  ret = 0;
  
 -  for (; ifnet_addr != NULL; ifnet_addr = TAILQ_NEXT(, if_list)) {
-+  if (getifaddrs(>ifap) != 0) {
-+  warn("failed to get interface addresses");
-+  return (0);
-+  }
++  (void)statep;
  
 -  if (kvm_read(statep->kd, (unsigned long)ifnet_addr, ,
 -  sizeof (struct ifnet)) != sizeof (struct ifnet))
 -  return (0);
-+  for (ifa = statep->ifap; ifa != NULL; ifa = ifa->ifa_next) {
-+  if (strcmp(ifname, ifa->ifa_name)) {
-+  continue;
-+  }
++  if (getifaddrs() != 0) {
++  warn("failed to get interface addresses");
++  return (0);
++  }
  
 -  if (strcmp(ifnet.

net/wmnetload has a memory leak [patch]

2018-12-19 Thread Ed Hynan 
net/wmnetload has a memory leak [patch]

wmnetload (6.4 release) repeatedly calls getifaddrs(3)
without freeifaddrs.

The patch here fixes and would replace
net/wmnetload/patches/patch-src_ifstat_openbsd_c
not apply over it.

Also fixes a compile warning from strcmp() w/o 

-Ed Hynan

# patch:
--- src/ifstat_openbsd.c.orig   Tue Jan 29 03:09:18 2002
+++ src/ifstat_openbsd.cTue Dec 18 13:38:23 2018
@@ -24,22 +24,18 @@
 #pragma ident "@(#)ifstat_netbsd.c 1.2 02/01/29 meem"
 
 #include 
+#include 
 #include 
 #include 
 #include 
-#include 
-#include 
-#include 
-#include 
+#include 
 #include 
-#include 
 
 #include "ifstat.h"
 #include "utils.h"
 
 struct ifstatstate {
-   void*ifnet_head;
-   kvm_t   *kd;
+   struct ifaddrs *ifap;
 };
 
 /*
@@ -51,8 +47,6 @@
 if_statinit(void)
 {
ifstatstate_t   *statep;
-   struct nlistifnet[] = { { "_ifnet" }, { NULL }};
-   charerrbuf[_POSIX2_LINE_MAX];
 
statep = malloc(sizeof (ifstatstate_t));
if (statep == NULL) {
@@ -60,37 +54,7 @@
return (NULL);
}
 
-   /*
-* Just for the duration of kmem_openfiles(), get privileges
-* needed to access kmem.
-*/
-   chpriv(PRIV_GAIN);
-   statep->kd = kvm_openfiles(NULL, NULL, NULL, O_RDONLY, errbuf);
-   chpriv(PRIV_DROP);
-   if (statep->kd == NULL) {
-   warn("cannot access raw kernel memory: %s\n", errbuf);
-   free(statep);
-   return (NULL);
-   }
-
-   if (kvm_nlist(statep->kd, ifnet) == -1) {
-   warn("cannot populate kernel namelist: %s\n",
-   kvm_geterr(statep->kd));
-   goto fail;
-   }
-
-   if (kvm_read(statep->kd, ifnet->n_value, >ifnet_head,
-   sizeof (ifnet->n_value)) != sizeof (ifnet->n_value)) {
-   warn("cannot find ifnet list head: %s\n",
-   kvm_geterr(statep->kd));
-   goto fail;
-   }
-
return (statep);
-fail:
-   (void) kvm_close(statep->kd);
-   free(statep);
-   return (NULL);
 }
 
 /*
@@ -100,22 +64,39 @@
 int
 if_stats(const char *ifname, ifstatstate_t *statep, ifstats_t *ifstatsp)
 {
-   void*ifnet_addr = statep->ifnet_head;
-   struct ifnetifnet;
+   struct ifaddrs *ifa;
 
-   for (; ifnet_addr != NULL; ifnet_addr = TAILQ_NEXT(, if_list)) {
+   if (statep->ifap != NULL) {
+   freeifaddrs(statep->ifap);
+   }
 
-   if (kvm_read(statep->kd, (unsigned long)ifnet_addr, ,
-   sizeof (struct ifnet)) != sizeof (struct ifnet))
-   return (0);
+   if (getifaddrs(>ifap) != 0) {
+   warn("failed to get interface addresses");
+   statep->ifap = NULL;
+   return (0);
+   }
 
-   if (strcmp(ifnet.if_xname, ifname) == 0) {
-   ifstatsp->rxbytes = ifnet.if_ibytes;
-   ifstatsp->txbytes = ifnet.if_obytes;
-   return (1);
+   for (ifa = statep->ifap; ifa != NULL; ifa = ifa->ifa_next) {
+   if (strcmp(ifname, ifa->ifa_name)) {
+   continue;
}
+
+   if (ifa->ifa_addr->sa_family == AF_LINK) {
+   struct sockaddr_dl *dl = (struct sockaddr_dl 
*)ifa->ifa_addr;
+   struct if_data *ifd = NULL;
+
+   ifd = ifa->ifa_data;
+
+   if (ifd != NULL) {
+   ifstatsp->rxbytes = ifd->ifi_ibytes;
+   ifstatsp->txbytes = ifd->ifi_obytes;
+   return 1;
+   }
+   }
}
 
+   freeifaddrs(statep->ifap);
+   statep->ifap = NULL;
return (0);
 }
 
@@ -125,6 +106,7 @@
 void
 if_statfini(ifstatstate_t *statep)
 {
-   (void) kvm_close(statep->kd);
+   if (statep->ifap != NULL)
+   freeifaddrs(statep->ifap);
free(statep);
 }