diff mbox series

[Openvpn-devel] dco-freebsd: dynamically re-allocate buffer if it's too small

Message ID 20240124152739.28248-1-kprovost@netgate.com
State Accepted
Headers
Series [Openvpn-devel] dco-freebsd: dynamically re-allocate buffer if it's too small |

Commit Message

Kristof Provost Jan. 24, 2024, 3:27 p.m. UTC 
  From: Kristof Provost <kp@FreeBSD.org>

It's possible for the buffer we provude for OVPN_GET_PEER_STATS to be
too small. Handle the error, re-allocate a larger buffer and try again
rather than failing.

Signed-off-by: Kristof Provost <kprovost@netgate.com>
---
 src/openvpn/dco_freebsd.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Kristof Provost Feb. 6, 2024, 4:37 p.m. UTC | #1 
Ping?

Does this need anything else before it can land?

Best regards,
Kristof

On 24 Jan 2024, at 8:27, Kristof Provost via Openvpn-devel wrote:
> From: Kristof Provost <kp@FreeBSD.org>
>
> It's possible for the buffer we provude for OVPN_GET_PEER_STATS to be
> too small. Handle the error, re-allocate a larger buffer and try again
> rather than failing.
>
> Signed-off-by: Kristof Provost <kprovost@netgate.com>
> ---
>  src/openvpn/dco_freebsd.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
> index 3ba1c817..577c65f8 100644
> --- a/src/openvpn/dco_freebsd.c
> +++ b/src/openvpn/dco_freebsd.c
> @@ -698,7 +698,8 @@ dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m)
>  {
>
>      struct ifdrv drv;
> -    uint8_t buf[4096];
> +    uint8_t *buf = NULL;
> +    size_t buf_size = 4096;
>      nvlist_t *nvl;
>      const nvlist_t *const *nvpeers;
>      size_t npeers;
> @@ -712,17 +713,28 @@ dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m)
>      CLEAR(drv);
>      snprintf(drv.ifd_name, IFNAMSIZ, "%s", dco->ifname);
>      drv.ifd_cmd = OVPN_GET_PEER_STATS;
> -    drv.ifd_len = sizeof(buf);
> +
> +retry:
> +    buf = realloc(buf, buf_size);
> +    drv.ifd_len = buf_size;
>      drv.ifd_data = buf;
>
>      ret = ioctl(dco->fd, SIOCGDRVSPEC, &drv);
> +    if (ret && errno == ENOSPC)
> +    {
> +        buf_size *= 2;
> +        goto retry;
> +    }
> +
>      if (ret)
>      {
> +        free(buf);
>          msg(M_WARN | M_ERRNO, "Failed to get peer stats");
>          return -EINVAL;
>      }
>
>      nvl = nvlist_unpack(buf, drv.ifd_len, 0);
> +    free(buf);
>      if (!nvl)
>      {
>          msg(M_WARN, "Failed to unpack nvlist");
> -- 
> 2.43.0
>
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Gert Doering Feb. 6, 2024, 5:43 p.m. UTC | #2 
Hi,

On Tue, Feb 06, 2024 at 09:37:47AM -0700, Kristof Provost wrote:
> Ping?
> 
> Does this need anything else before it can land?

Just time on my side.  Busy weeks, apologies.  Will handle tomorrow.

gert
Gert Doering Feb. 7, 2024, 10:06 a.m. UTC | #3 
Acked-by: Gert Doering <gert@greenie.muc.de>

Stare-at-code looks good :-) - there's one possible catch should realloc()
return NULL - in that case we'd pass drv.ifd_data = NULL to the kernel - but
I'm reasonably sure the kernel will then not crash but return EINVAL.

I do not have a sufficient number of clients on a FreeBSD 14 system today
to trigger the original problem, but I can attest that the new dynamic
code will do the right thing - I tested this by reducing the start allocation
to 1 byte and see it grow to 1024 (test with 2 clients, 512 sufficient
for 1 client).

(Also, master+patch passes the full server test rig on FreeBSD 14)

Your patch has been applied to the master and release/2.6 branch (bugfix)

commit 62676935d738f74908845ca96819a36a8c0c230e (master)
commit d8faf568d237667c66141e2c3f6df3f999aa02bd (release/2.6)
Author: Kristof Provost
Date:   Wed Jan 24 16:27:39 2024 +0100

     dco-freebsd: dynamically re-allocate buffer if it's too small

     Signed-off-by: Kristof Provost <kprovost@netgate.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20240124152739.28248-1-kprovost@netgate.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28128.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
index 3ba1c817..577c65f8 100644
--- a/src/openvpn/dco_freebsd.c
+++ b/src/openvpn/dco_freebsd.c
@@ -698,7 +698,8 @@  dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m)
 {
 
     struct ifdrv drv;
-    uint8_t buf[4096];
+    uint8_t *buf = NULL;
+    size_t buf_size = 4096;
     nvlist_t *nvl;
     const nvlist_t *const *nvpeers;
     size_t npeers;
@@ -712,17 +713,28 @@  dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m)
     CLEAR(drv);
     snprintf(drv.ifd_name, IFNAMSIZ, "%s", dco->ifname);
     drv.ifd_cmd = OVPN_GET_PEER_STATS;
-    drv.ifd_len = sizeof(buf);
+
+retry:
+    buf = realloc(buf, buf_size);
+    drv.ifd_len = buf_size;
     drv.ifd_data = buf;
 
     ret = ioctl(dco->fd, SIOCGDRVSPEC, &drv);
+    if (ret && errno == ENOSPC)
+    {
+        buf_size *= 2;
+        goto retry;
+    }
+
     if (ret)
     {
+        free(buf);
         msg(M_WARN | M_ERRNO, "Failed to get peer stats");
         return -EINVAL;
     }
 
     nvl = nvlist_unpack(buf, drv.ifd_len, 0);
+    free(buf);
     if (!nvl)
     {
         msg(M_WARN, "Failed to unpack nvlist");