* [dpdk-dev] [PATCH 1/3] bus/vmbus: fix leak on device scan
2021-09-17 8:23 [dpdk-dev] [PATCH 0/3] Experiment ASAN in GHA David Marchand
@ 2021-09-17 8:23 ` David Marchand
2021-09-29 20:57 ` Long Li
2021-09-17 8:23 ` [dpdk-dev] [PATCH 2/3] test/latencystats: fix incorrect loop boundary David Marchand
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: David Marchand @ 2021-09-17 8:23 UTC (permalink / raw)
To: dev; +Cc: aconole, zhihongx.peng, stable, Stephen Hemminger, Long Li
Caught running ASAN.
The device name is leaked on scan.
rte_device name field being a const, use the private vmbus struct to
store the device name and point at it.
Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
drivers/bus/vmbus/linux/vmbus_bus.c | 5 ++++-
drivers/bus/vmbus/rte_bus_vmbus.h | 1 +
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/vmbus/linux/vmbus_bus.c b/drivers/bus/vmbus/linux/vmbus_bus.c
index 3c924eee14..d8eb07d398 100644
--- a/drivers/bus/vmbus/linux/vmbus_bus.c
+++ b/drivers/bus/vmbus/linux/vmbus_bus.c
@@ -242,7 +242,7 @@ vmbus_scan_one(const char *name)
return -1;
dev->device.bus = &rte_vmbus_bus.bus;
- dev->device.name = strdup(name);
+ dev->device.name = dev->name = strdup(name);
if (!dev->device.name)
goto error;
@@ -261,6 +261,7 @@ vmbus_scan_one(const char *name)
/* skip non-network devices */
if (rte_uuid_compare(dev->class_id, vmbus_nic_uuid) != 0) {
+ free(dev->name);
free(dev);
return 0;
}
@@ -312,6 +313,7 @@ vmbus_scan_one(const char *name)
} else { /* already registered */
VMBUS_LOG(NOTICE,
"%s already registered", name);
+ free(dev->name);
free(dev);
}
return 0;
@@ -322,6 +324,7 @@ vmbus_scan_one(const char *name)
error:
VMBUS_LOG(DEBUG, "failed");
+ free(dev->name);
free(dev);
return -1;
}
diff --git a/drivers/bus/vmbus/rte_bus_vmbus.h b/drivers/bus/vmbus/rte_bus_vmbus.h
index 4cf73ce815..438eb481fc 100644
--- a/drivers/bus/vmbus/rte_bus_vmbus.h
+++ b/drivers/bus/vmbus/rte_bus_vmbus.h
@@ -65,6 +65,7 @@ struct rte_vmbus_device {
TAILQ_ENTRY(rte_vmbus_device) next; /**< Next probed VMBUS device */
const struct rte_vmbus_driver *driver; /**< Associated driver */
struct rte_device device; /**< Inherit core device */
+ char *name; /**< Device name */
rte_uuid_t device_id; /**< VMBUS device id */
rte_uuid_t class_id; /**< VMBUS device type */
uint32_t relid; /**< id for primary */
--
2.23.0
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH 1/3] bus/vmbus: fix leak on device scan
2021-09-17 8:23 ` [dpdk-dev] [PATCH 1/3] bus/vmbus: fix leak on device scan David Marchand
@ 2021-09-29 20:57 ` Long Li
2021-09-30 7:50 ` David Marchand
0 siblings, 1 reply; 17+ messages in thread
From: Long Li @ 2021-09-29 20:57 UTC (permalink / raw)
To: David Marchand, dev; +Cc: aconole, zhihongx.peng, stable, Stephen Hemminger
> Subject: [PATCH 1/3] bus/vmbus: fix leak on device scan
>
> Caught running ASAN.
>
> The device name is leaked on scan.
> rte_device name field being a const, use the private vmbus struct to store the
> device name and point at it.
>
> Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> drivers/bus/vmbus/linux/vmbus_bus.c | 5 ++++-
> drivers/bus/vmbus/rte_bus_vmbus.h | 1 +
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/vmbus/linux/vmbus_bus.c
> b/drivers/bus/vmbus/linux/vmbus_bus.c
> index 3c924eee14..d8eb07d398 100644
> --- a/drivers/bus/vmbus/linux/vmbus_bus.c
> +++ b/drivers/bus/vmbus/linux/vmbus_bus.c
> @@ -242,7 +242,7 @@ vmbus_scan_one(const char *name)
> return -1;
>
> dev->device.bus = &rte_vmbus_bus.bus;
> - dev->device.name = strdup(name);
> + dev->device.name = dev->name = strdup(name);
I suggest not to add a "name" in struct rte_vmbus_device. How about defining a local variable in this function, like:
dev->device.name = dev_name = strdup(name);
If failed, just free(dev_name).
> if (!dev->device.name)
> goto error;
>
> @@ -261,6 +261,7 @@ vmbus_scan_one(const char *name)
>
> /* skip non-network devices */
> if (rte_uuid_compare(dev->class_id, vmbus_nic_uuid) != 0) {
> + free(dev->name);
> free(dev);
> return 0;
> }
> @@ -312,6 +313,7 @@ vmbus_scan_one(const char *name)
> } else { /* already registered */
> VMBUS_LOG(NOTICE,
> "%s already registered", name);
> + free(dev->name);
> free(dev);
> }
> return 0;
> @@ -322,6 +324,7 @@ vmbus_scan_one(const char *name)
> error:
> VMBUS_LOG(DEBUG, "failed");
>
> + free(dev->name);
> free(dev);
> return -1;
> }
> diff --git a/drivers/bus/vmbus/rte_bus_vmbus.h
> b/drivers/bus/vmbus/rte_bus_vmbus.h
> index 4cf73ce815..438eb481fc 100644
> --- a/drivers/bus/vmbus/rte_bus_vmbus.h
> +++ b/drivers/bus/vmbus/rte_bus_vmbus.h
> @@ -65,6 +65,7 @@ struct rte_vmbus_device {
> TAILQ_ENTRY(rte_vmbus_device) next; /**< Next probed VMBUS
> device */
> const struct rte_vmbus_driver *driver; /**< Associated driver */
> struct rte_device device; /**< Inherit core device */
> + char *name; /**< Device name */
> rte_uuid_t device_id; /**< VMBUS device id */
> rte_uuid_t class_id; /**< VMBUS device type */
> uint32_t relid; /**< id for primary */
> --
> 2.23.0
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH 1/3] bus/vmbus: fix leak on device scan
2021-09-29 20:57 ` Long Li
@ 2021-09-30 7:50 ` David Marchand
2021-09-30 19:14 ` Long Li
0 siblings, 1 reply; 17+ messages in thread
From: David Marchand @ 2021-09-30 7:50 UTC (permalink / raw)
To: Long Li; +Cc: dev, aconole, zhihongx.peng, stable, Stephen Hemminger
On Wed, Sep 29, 2021 at 10:57 PM Long Li <longli@microsoft.com> wrote:
>
> > Subject: [PATCH 1/3] bus/vmbus: fix leak on device scan
> >
> > Caught running ASAN.
> >
> > The device name is leaked on scan.
> > rte_device name field being a const, use the private vmbus struct to store the
> > device name and point at it.
> >
> > Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > drivers/bus/vmbus/linux/vmbus_bus.c | 5 ++++-
> > drivers/bus/vmbus/rte_bus_vmbus.h | 1 +
> > 2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/bus/vmbus/linux/vmbus_bus.c
> > b/drivers/bus/vmbus/linux/vmbus_bus.c
> > index 3c924eee14..d8eb07d398 100644
> > --- a/drivers/bus/vmbus/linux/vmbus_bus.c
> > +++ b/drivers/bus/vmbus/linux/vmbus_bus.c
> > @@ -242,7 +242,7 @@ vmbus_scan_one(const char *name)
> > return -1;
> >
> > dev->device.bus = &rte_vmbus_bus.bus;
> > - dev->device.name = strdup(name);
> > + dev->device.name = dev->name = strdup(name);
>
>
> I suggest not to add a "name" in struct rte_vmbus_device. How about defining a local variable in this function, like:
> dev->device.name = dev_name = strdup(name);
What is your concern for storing the name?
rte_device name only points at some location where the name is stored.
In general this storage is in the bus object or (in some buses) the
devarg that resulted in the rte_device object creation.
If we won't store the name in the bus object, then we lose the ability
to release the name later.
This is probably fine as long as we never release rte_vmbus_device
objects which is the case atm.
But I don't understand why vmbus should be an exception when comparing
to other buses.
--
David Marchand
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH 1/3] bus/vmbus: fix leak on device scan
2021-09-30 7:50 ` David Marchand
@ 2021-09-30 19:14 ` Long Li
2021-10-01 6:47 ` David Marchand
0 siblings, 1 reply; 17+ messages in thread
From: Long Li @ 2021-09-30 19:14 UTC (permalink / raw)
To: David Marchand; +Cc: dev, aconole, zhihongx.peng, stable, Stephen Hemminger
> Subject: Re: [PATCH 1/3] bus/vmbus: fix leak on device scan
>
> On Wed, Sep 29, 2021 at 10:57 PM Long Li <longli@microsoft.com> wrote:
> >
> > > Subject: [PATCH 1/3] bus/vmbus: fix leak on device scan
> > >
> > > Caught running ASAN.
> > >
> > > The device name is leaked on scan.
> > > rte_device name field being a const, use the private vmbus struct to
> > > store the device name and point at it.
> > >
> > > Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---
> > > drivers/bus/vmbus/linux/vmbus_bus.c | 5 ++++-
> > > drivers/bus/vmbus/rte_bus_vmbus.h | 1 +
> > > 2 files changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/bus/vmbus/linux/vmbus_bus.c
> > > b/drivers/bus/vmbus/linux/vmbus_bus.c
> > > index 3c924eee14..d8eb07d398 100644
> > > --- a/drivers/bus/vmbus/linux/vmbus_bus.c
> > > +++ b/drivers/bus/vmbus/linux/vmbus_bus.c
> > > @@ -242,7 +242,7 @@ vmbus_scan_one(const char *name)
> > > return -1;
> > >
> > > dev->device.bus = &rte_vmbus_bus.bus;
> > > - dev->device.name = strdup(name);
> > > + dev->device.name = dev->name = strdup(name);
> >
> >
> > I suggest not to add a "name" in struct rte_vmbus_device. How about
> defining a local variable in this function, like:
> > dev->device.name = dev_name = strdup(name);
>
> What is your concern for storing the name?
This name is only used in this function. I see little value of putting it in struct rte_vmbus_device. Am I missing another patch that uses it from struct rte_vmbus_device from another place?
>
>
> rte_device name only points at some location where the name is stored.
> In general this storage is in the bus object or (in some buses) the devarg that
> resulted in the rte_device object creation.
>
> If we won't store the name in the bus object, then we lose the ability to
> release the name later.
> This is probably fine as long as we never release rte_vmbus_device objects
> which is the case atm.
> But I don't understand why vmbus should be an exception when comparing
> to other buses.
I don’t understand why you want to put a name there if it's never used outside vmbus_scan_one().
Long
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH 1/3] bus/vmbus: fix leak on device scan
2021-09-30 19:14 ` Long Li
@ 2021-10-01 6:47 ` David Marchand
2021-10-01 16:28 ` Long Li
0 siblings, 1 reply; 17+ messages in thread
From: David Marchand @ 2021-10-01 6:47 UTC (permalink / raw)
To: Long Li; +Cc: dev, aconole, zhihongx.peng, stable, Stephen Hemminger
On Thu, Sep 30, 2021 at 9:14 PM Long Li <longli@microsoft.com> wrote:
> > rte_device name only points at some location where the name is stored.
> > In general this storage is in the bus object or (in some buses) the devarg that
> > resulted in the rte_device object creation.
> >
> > If we won't store the name in the bus object, then we lose the ability to
> > release the name later.
> > This is probably fine as long as we never release rte_vmbus_device objects
> > which is the case atm.
> > But I don't understand why vmbus should be an exception when comparing
> > to other buses.
>
> I don’t understand why you want to put a name there if it's never used outside vmbus_scan_one().
Since you don't care about releasing rte_vmbus_device objects, I'll
rework as you suggest.
--
David Marchand
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dpdk-dev] [PATCH 1/3] bus/vmbus: fix leak on device scan
2021-10-01 6:47 ` David Marchand
@ 2021-10-01 16:28 ` Long Li
0 siblings, 0 replies; 17+ messages in thread
From: Long Li @ 2021-10-01 16:28 UTC (permalink / raw)
To: David Marchand; +Cc: dev, aconole, zhihongx.peng, stable, Stephen Hemminger
> Subject: Re: [PATCH 1/3] bus/vmbus: fix leak on device scan
>
> On Thu, Sep 30, 2021 at 9:14 PM Long Li <longli@microsoft.com> wrote:
> > > rte_device name only points at some location where the name is stored.
> > > In general this storage is in the bus object or (in some buses) the
> > > devarg that resulted in the rte_device object creation.
> > >
> > > If we won't store the name in the bus object, then we lose the
> > > ability to release the name later.
> > > This is probably fine as long as we never release rte_vmbus_device
> > > objects which is the case atm.
> > > But I don't understand why vmbus should be an exception when
> > > comparing to other buses.
> >
> > I don’t understand why you want to put a name there if it's never used outside
> vmbus_scan_one().
>
> Since you don't care about releasing rte_vmbus_device objects, I'll rework as
> you suggest.
I don't have any objection defining new data fields in the device struct, as long as it's used during the life cycle of the device.
The rest of the patch looks good, thank you.
Long
^ permalink raw reply [flat|nested] 17+ messages in thread
* [dpdk-dev] [PATCH 2/3] test/latencystats: fix incorrect loop boundary
2021-09-17 8:23 [dpdk-dev] [PATCH 0/3] Experiment ASAN in GHA David Marchand
2021-09-17 8:23 ` [dpdk-dev] [PATCH 1/3] bus/vmbus: fix leak on device scan David Marchand
@ 2021-09-17 8:23 ` David Marchand
2021-09-20 8:51 ` Pattan, Reshma
2021-09-17 8:23 ` [dpdk-dev] [PATCH 3/3] ci: run unit tests with ASAN David Marchand
2021-10-02 16:24 ` [dpdk-dev] [PATCH v2 0/3] Experiment ASAN in GHA David Marchand
3 siblings, 1 reply; 17+ messages in thread
From: David Marchand @ 2021-09-17 8:23 UTC (permalink / raw)
To: dev; +Cc: aconole, zhihongx.peng, stable, Reshma Pattan, Naga Suresh Somarowthu
Caught running ASAN.
lat_stats_strings[] is an array containing NUM_STATS strings.
Fixes: 1e3676a06e4c ("test/latency: add unit tests for latencystats library")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
app/test/test_latencystats.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/app/test/test_latencystats.c b/app/test/test_latencystats.c
index 427339904d..724acbc315 100644
--- a/app/test/test_latencystats.c
+++ b/app/test/test_latencystats.c
@@ -80,7 +80,7 @@ static int test_latencystats_get_names(void)
/* Success Test: Valid names and size */
size = NUM_STATS;
ret = rte_latencystats_get_names(names, size);
- for (i = 0; i <= NUM_STATS; i++) {
+ for (i = 0; i < NUM_STATS; i++) {
if (strcmp(lat_stats_strings[i].name, names[i].name) == 0)
printf(" %s\n", names[i].name);
else
--
2.23.0
^ permalink raw reply [flat|nested] 17+ messages in thread* [dpdk-dev] [PATCH 3/3] ci: run unit tests with ASAN
2021-09-17 8:23 [dpdk-dev] [PATCH 0/3] Experiment ASAN in GHA David Marchand
2021-09-17 8:23 ` [dpdk-dev] [PATCH 1/3] bus/vmbus: fix leak on device scan David Marchand
2021-09-17 8:23 ` [dpdk-dev] [PATCH 2/3] test/latencystats: fix incorrect loop boundary David Marchand
@ 2021-09-17 8:23 ` David Marchand
2021-10-02 16:24 ` [dpdk-dev] [PATCH v2 0/3] Experiment ASAN in GHA David Marchand
3 siblings, 0 replies; 17+ messages in thread
From: David Marchand @ 2021-09-17 8:23 UTC (permalink / raw)
To: dev; +Cc: aconole, zhihongx.peng, Michael Santana
Enable ASAN for clang jobs.
This can greatly help identify leaks and buffer overflows.
This patch is more a fyi, as some unit tests stil have issues.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
.ci/linux-build.sh | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 91e43a975b..a961d9b92d 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -79,7 +79,14 @@ fi
OPTS="$OPTS -Dmachine=default"
OPTS="$OPTS --default-library=$DEF_LIB"
-OPTS="$OPTS --buildtype=debugoptimized"
+
+if [ "$CC" != "${CC%%clang}" ] && [ "$RUN_TESTS" = 'true' ]; then
+ # Let's run tests with ASAN
+ OPTS="$OPTS -Db_sanitize=address -Db_lundef=false --buildtype=debug"
+else
+ OPTS="$OPTS --buildtype=debugoptimized"
+fi
+
OPTS="$OPTS -Dcheck_includes=true"
meson build --werror $OPTS
ninja -C build
--
2.23.0
^ permalink raw reply [flat|nested] 17+ messages in thread* [dpdk-dev] [PATCH v2 0/3] Experiment ASAN in GHA
2021-09-17 8:23 [dpdk-dev] [PATCH 0/3] Experiment ASAN in GHA David Marchand
` (2 preceding siblings ...)
2021-09-17 8:23 ` [dpdk-dev] [PATCH 3/3] ci: run unit tests with ASAN David Marchand
@ 2021-10-02 16:24 ` David Marchand
2021-10-02 16:24 ` [dpdk-dev] [PATCH v2 1/3] bus/vmbus: fix leak on device scan David Marchand
` (3 more replies)
3 siblings, 4 replies; 17+ messages in thread
From: David Marchand @ 2021-10-02 16:24 UTC (permalink / raw)
To: dev; +Cc: aconole, zhihongx.peng
Following fixes for ASAN, I added ASAN in GHA to see where we stand.
Here are two fixes that can be merged.
The last patch enables ASAN, but as we can see, there are still some
unit tests who fails because of ASAN:
- multiprocess is broken, we could flag the ut requiring/testing mp,
- hash tests have a lot of issues, I suspect bugs in the hash library,
bugs have been opened,
--
David Marchand
David Marchand (3):
bus/vmbus: fix leak on device scan
test/latencystats: fix incorrect loop boundary
ci: run unit tests with ASAN
.ci/linux-build.sh | 9 ++++++++-
app/test/test_latencystats.c | 2 +-
drivers/bus/vmbus/linux/vmbus_bus.c | 6 +++++-
3 files changed, 14 insertions(+), 3 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 17+ messages in thread* [dpdk-dev] [PATCH v2 1/3] bus/vmbus: fix leak on device scan
2021-10-02 16:24 ` [dpdk-dev] [PATCH v2 0/3] Experiment ASAN in GHA David Marchand
@ 2021-10-02 16:24 ` David Marchand
2021-10-04 17:43 ` Long Li
2021-10-02 16:24 ` [dpdk-dev] [PATCH v2 2/3] test/latencystats: fix incorrect loop boundary David Marchand
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: David Marchand @ 2021-10-02 16:24 UTC (permalink / raw)
To: dev; +Cc: aconole, zhihongx.peng, stable, Stephen Hemminger, Long Li
Caught running ASAN.
The device name was leaked on scan.
rte_device name field being a const, use a local pointer and release
in error path.
Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- left rte_vmbus_device untouched,
---
drivers/bus/vmbus/linux/vmbus_bus.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/vmbus/linux/vmbus_bus.c b/drivers/bus/vmbus/linux/vmbus_bus.c
index 3c924eee14..68f6cc5742 100644
--- a/drivers/bus/vmbus/linux/vmbus_bus.c
+++ b/drivers/bus/vmbus/linux/vmbus_bus.c
@@ -236,13 +236,14 @@ vmbus_scan_one(const char *name)
char filename[PATH_MAX];
char dirname[PATH_MAX];
unsigned long tmp;
+ char *dev_name;
dev = calloc(1, sizeof(*dev));
if (dev == NULL)
return -1;
dev->device.bus = &rte_vmbus_bus.bus;
- dev->device.name = strdup(name);
+ dev->device.name = dev_name = strdup(name);
if (!dev->device.name)
goto error;
@@ -261,6 +262,7 @@ vmbus_scan_one(const char *name)
/* skip non-network devices */
if (rte_uuid_compare(dev->class_id, vmbus_nic_uuid) != 0) {
+ free(dev_name);
free(dev);
return 0;
}
@@ -312,6 +314,7 @@ vmbus_scan_one(const char *name)
} else { /* already registered */
VMBUS_LOG(NOTICE,
"%s already registered", name);
+ free(dev_name);
free(dev);
}
return 0;
@@ -322,6 +325,7 @@ vmbus_scan_one(const char *name)
error:
VMBUS_LOG(DEBUG, "failed");
+ free(dev_name);
free(dev);
return -1;
}
--
2.23.0
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH v2 1/3] bus/vmbus: fix leak on device scan
2021-10-02 16:24 ` [dpdk-dev] [PATCH v2 1/3] bus/vmbus: fix leak on device scan David Marchand
@ 2021-10-04 17:43 ` Long Li
0 siblings, 0 replies; 17+ messages in thread
From: Long Li @ 2021-10-04 17:43 UTC (permalink / raw)
To: David Marchand, dev; +Cc: aconole, zhihongx.peng, stable, Stephen Hemminger
> Subject: [PATCH v2 1/3] bus/vmbus: fix leak on device scan
>
> Caught running ASAN.
> The device name was leaked on scan.
> rte_device name field being a const, use a local pointer and release in error path.
>
> Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
Thank you.
Acked-by: Long Li <longli@microsoft.com>
> ---
> Changes since v1:
> - left rte_vmbus_device untouched,
>
> ---
> drivers/bus/vmbus/linux/vmbus_bus.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/vmbus/linux/vmbus_bus.c
> b/drivers/bus/vmbus/linux/vmbus_bus.c
> index 3c924eee14..68f6cc5742 100644
> --- a/drivers/bus/vmbus/linux/vmbus_bus.c
> +++ b/drivers/bus/vmbus/linux/vmbus_bus.c
> @@ -236,13 +236,14 @@ vmbus_scan_one(const char *name)
> char filename[PATH_MAX];
> char dirname[PATH_MAX];
> unsigned long tmp;
> + char *dev_name;
>
> dev = calloc(1, sizeof(*dev));
> if (dev == NULL)
> return -1;
>
> dev->device.bus = &rte_vmbus_bus.bus;
> - dev->device.name = strdup(name);
> + dev->device.name = dev_name = strdup(name);
> if (!dev->device.name)
> goto error;
>
> @@ -261,6 +262,7 @@ vmbus_scan_one(const char *name)
>
> /* skip non-network devices */
> if (rte_uuid_compare(dev->class_id, vmbus_nic_uuid) != 0) {
> + free(dev_name);
> free(dev);
> return 0;
> }
> @@ -312,6 +314,7 @@ vmbus_scan_one(const char *name)
> } else { /* already registered */
> VMBUS_LOG(NOTICE,
> "%s already registered", name);
> + free(dev_name);
> free(dev);
> }
> return 0;
> @@ -322,6 +325,7 @@ vmbus_scan_one(const char *name)
> error:
> VMBUS_LOG(DEBUG, "failed");
>
> + free(dev_name);
> free(dev);
> return -1;
> }
> --
> 2.23.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [dpdk-dev] [PATCH v2 2/3] test/latencystats: fix incorrect loop boundary
2021-10-02 16:24 ` [dpdk-dev] [PATCH v2 0/3] Experiment ASAN in GHA David Marchand
2021-10-02 16:24 ` [dpdk-dev] [PATCH v2 1/3] bus/vmbus: fix leak on device scan David Marchand
@ 2021-10-02 16:24 ` David Marchand
2021-10-02 16:24 ` [dpdk-dev] [PATCH v2 3/3] ci: run unit tests with ASAN David Marchand
2021-10-05 15:20 ` [dpdk-dev] [PATCH v2 0/3] Experiment ASAN in GHA David Marchand
3 siblings, 0 replies; 17+ messages in thread
From: David Marchand @ 2021-10-02 16:24 UTC (permalink / raw)
To: dev; +Cc: aconole, zhihongx.peng, stable, Reshma Pattan, Naga Suresh Somarowthu
Caught running ASAN.
lat_stats_strings[] is an array containing NUM_STATS strings.
Fixes: 1e3676a06e4c ("test/latency: add unit tests for latencystats library")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Reshma Pattan <reshma.pattan@intel.com>
---
app/test/test_latencystats.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/app/test/test_latencystats.c b/app/test/test_latencystats.c
index 427339904d..724acbc315 100644
--- a/app/test/test_latencystats.c
+++ b/app/test/test_latencystats.c
@@ -80,7 +80,7 @@ static int test_latencystats_get_names(void)
/* Success Test: Valid names and size */
size = NUM_STATS;
ret = rte_latencystats_get_names(names, size);
- for (i = 0; i <= NUM_STATS; i++) {
+ for (i = 0; i < NUM_STATS; i++) {
if (strcmp(lat_stats_strings[i].name, names[i].name) == 0)
printf(" %s\n", names[i].name);
else
--
2.23.0
^ permalink raw reply [flat|nested] 17+ messages in thread* [dpdk-dev] [PATCH v2 3/3] ci: run unit tests with ASAN
2021-10-02 16:24 ` [dpdk-dev] [PATCH v2 0/3] Experiment ASAN in GHA David Marchand
2021-10-02 16:24 ` [dpdk-dev] [PATCH v2 1/3] bus/vmbus: fix leak on device scan David Marchand
2021-10-02 16:24 ` [dpdk-dev] [PATCH v2 2/3] test/latencystats: fix incorrect loop boundary David Marchand
@ 2021-10-02 16:24 ` David Marchand
2021-10-05 12:00 ` Aaron Conole
2021-10-05 15:20 ` [dpdk-dev] [PATCH v2 0/3] Experiment ASAN in GHA David Marchand
3 siblings, 1 reply; 17+ messages in thread
From: David Marchand @ 2021-10-02 16:24 UTC (permalink / raw)
To: dev; +Cc: aconole, zhihongx.peng, Michael Santana
Enable ASAN for clang jobs.
This can greatly help identify leaks and buffer overflows.
This patch is more a fyi, as some unit tests stil have issues.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
.ci/linux-build.sh | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 91e43a975b..a961d9b92d 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -79,7 +79,14 @@ fi
OPTS="$OPTS -Dmachine=default"
OPTS="$OPTS --default-library=$DEF_LIB"
-OPTS="$OPTS --buildtype=debugoptimized"
+
+if [ "$CC" != "${CC%%clang}" ] && [ "$RUN_TESTS" = 'true' ]; then
+ # Let's run tests with ASAN
+ OPTS="$OPTS -Db_sanitize=address -Db_lundef=false --buildtype=debug"
+else
+ OPTS="$OPTS --buildtype=debugoptimized"
+fi
+
OPTS="$OPTS -Dcheck_includes=true"
meson build --werror $OPTS
ninja -C build
--
2.23.0
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH v2 3/3] ci: run unit tests with ASAN
2021-10-02 16:24 ` [dpdk-dev] [PATCH v2 3/3] ci: run unit tests with ASAN David Marchand
@ 2021-10-05 12:00 ` Aaron Conole
0 siblings, 0 replies; 17+ messages in thread
From: Aaron Conole @ 2021-10-05 12:00 UTC (permalink / raw)
To: David Marchand; +Cc: dev, zhihongx.peng, Michael Santana
David Marchand <david.marchand@redhat.com> writes:
> Enable ASAN for clang jobs.
> This can greatly help identify leaks and buffer overflows.
> This patch is more a fyi, as some unit tests stil have issues.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
I know this isn't yet ready to be merged due to failing unit tests, and
I still support it :)
Acked-by: Aaron Conole <aconole@redhat.com>
> .ci/linux-build.sh | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> index 91e43a975b..a961d9b92d 100755
> --- a/.ci/linux-build.sh
> +++ b/.ci/linux-build.sh
> @@ -79,7 +79,14 @@ fi
>
> OPTS="$OPTS -Dmachine=default"
> OPTS="$OPTS --default-library=$DEF_LIB"
> -OPTS="$OPTS --buildtype=debugoptimized"
> +
> +if [ "$CC" != "${CC%%clang}" ] && [ "$RUN_TESTS" = 'true' ]; then
> + # Let's run tests with ASAN
> + OPTS="$OPTS -Db_sanitize=address -Db_lundef=false --buildtype=debug"
> +else
> + OPTS="$OPTS --buildtype=debugoptimized"
> +fi
> +
> OPTS="$OPTS -Dcheck_includes=true"
> meson build --werror $OPTS
> ninja -C build
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dpdk-dev] [PATCH v2 0/3] Experiment ASAN in GHA
2021-10-02 16:24 ` [dpdk-dev] [PATCH v2 0/3] Experiment ASAN in GHA David Marchand
` (2 preceding siblings ...)
2021-10-02 16:24 ` [dpdk-dev] [PATCH v2 3/3] ci: run unit tests with ASAN David Marchand
@ 2021-10-05 15:20 ` David Marchand
3 siblings, 0 replies; 17+ messages in thread
From: David Marchand @ 2021-10-05 15:20 UTC (permalink / raw)
To: dev; +Cc: Aaron Conole, Zhihong Peng, Long Li, Pattan, Reshma
On Sat, Oct 2, 2021 at 6:24 PM David Marchand <david.marchand@redhat.com> wrote:
>
> Following fixes for ASAN, I added ASAN in GHA to see where we stand.
> Here are two fixes that can be merged.
>
> The last patch enables ASAN, but as we can see, there are still some
> unit tests who fails because of ASAN:
> - multiprocess is broken, we could flag the ut requiring/testing mp,
> - hash tests have a lot of issues, I suspect bugs in the hash library,
>
> --
> David Marchand
>
> David Marchand (3):
> bus/vmbus: fix leak on device scan
> test/latencystats: fix incorrect loop boundary
> ci: run unit tests with ASAN
Series^WFirst two patches applied.
Thanks.
--
David Marchand
^ permalink raw reply [flat|nested] 17+ messages in thread