Re: bgpd fix bad free() call when deflating aspath

[Claudio Jeker]

On Tue, Jun 22, 2021 at 08:19:22PM +0200, Claudio Jeker wrote:
> I changed the way up_generate_attr() calls aspath_deflate() but did not
> realize that aspath_deflate() frees the pdata at the end. That free should
> no longer happen but for that also the mrt case where aspath_deflate()
> needs to be adjusted.
> 
> With this both the mrt and as0 integration test pass again.

Ping. This crashes bgpd when talking to old 2-byte only BGP speakers. So
I would like to commit this soon.

-- 
:wq Claudio

Index: rde_attr.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde_attr.c,v
retrieving revision 1.124
diff -u -p -r1.124 rde_attr.c
--- rde_attr.c  16 Jan 2021 13:14:54 -  1.124
+++ rde_attr.c  22 Jun 2021 18:13:15 -
@@ -568,7 +568,6 @@ aspath_put(struct aspath *aspath)
 
 /*
  * convert a 4 byte aspath to a 2 byte one.
- * data is freed by aspath_deflate
  */
 u_char *
 aspath_deflate(u_char *data, u_int16_t *len, int *flagnew)
@@ -614,7 +613,6 @@ aspath_deflate(u_char *data, u_int16_t *
}
}
 
-   free(data);
*len = nlen;
return (ndata);
 }
Index: mrt.c
===
RCS file: /cvs/src/usr.sbin/bgpd/mrt.c,v
retrieving revision 1.103
diff -u -p -r1.103 mrt.c
--- mrt.c   9 Jan 2020 11:55:25 -   1.103
+++ mrt.c   22 Jun 2021 18:12:45 -
@@ -160,15 +160,19 @@ mrt_attr_dump(struct ibuf *buf, struct r
return (-1);
 
/* aspath */
-   pdata = aspath_prepend(a->aspath, rde_local_as(), 0, &plen);
+   plen = aspath_length(a->aspath);
+   pdata = aspath_dump(a->aspath);
+
if (!v2)
pdata = aspath_deflate(pdata, &plen, &neednewpath);
if (attr_writebuf(buf, ATTR_WELL_KNOWN, ATTR_ASPATH, pdata,
plen) == -1) {
-   free(pdata);
+   if (!v2)
+   free(pdata);
return (-1);
}
-   free(pdata);
+   if (!v2)
+   free(pdata);
 
if (nexthop && nexthop->aid == AID_INET) {
/* nexthop, already network byte order */

bgpd fix bad free() call when deflating aspath

[Claudio Jeker]

I changed the way up_generate_attr() calls aspath_deflate() but did not
realize that aspath_deflate() frees the pdata at the end. That free should
no longer happen but for that also the mrt case where aspath_deflate()
needs to be adjusted.

With this both the mrt and as0 integration test pass again.
-- 
:wq Claudio

Index: rde_attr.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde_attr.c,v
retrieving revision 1.124
diff -u -p -r1.124 rde_attr.c
--- rde_attr.c  16 Jan 2021 13:14:54 -  1.124
+++ rde_attr.c  22 Jun 2021 18:13:15 -
@@ -568,7 +568,6 @@ aspath_put(struct aspath *aspath)
 
 /*
  * convert a 4 byte aspath to a 2 byte one.
- * data is freed by aspath_deflate
  */
 u_char *
 aspath_deflate(u_char *data, u_int16_t *len, int *flagnew)
@@ -614,7 +613,6 @@ aspath_deflate(u_char *data, u_int16_t *
}
}
 
-   free(data);
*len = nlen;
return (ndata);
 }
Index: mrt.c
===
RCS file: /cvs/src/usr.sbin/bgpd/mrt.c,v
retrieving revision 1.103
diff -u -p -r1.103 mrt.c
--- mrt.c   9 Jan 2020 11:55:25 -   1.103
+++ mrt.c   22 Jun 2021 18:12:45 -
@@ -160,15 +160,19 @@ mrt_attr_dump(struct ibuf *buf, struct r
return (-1);
 
/* aspath */
-   pdata = aspath_prepend(a->aspath, rde_local_as(), 0, &plen);
+   plen = aspath_length(a->aspath);
+   pdata = aspath_dump(a->aspath);
+
if (!v2)
pdata = aspath_deflate(pdata, &plen, &neednewpath);
if (attr_writebuf(buf, ATTR_WELL_KNOWN, ATTR_ASPATH, pdata,
plen) == -1) {
-   free(pdata);
+   if (!v2)
+   free(pdata);
return (-1);
}
-   free(pdata);
+   if (!v2)
+   free(pdata);
 
if (nexthop && nexthop->aid == AID_INET) {
/* nexthop, already network byte order */