[v2,1/1] pcapng: warn if NULL is passed to rte_pcapng_close

rte_pcapng_close() might dereference a null pointer; as example,
PVS-Studio gives its usage in test_pcapng.c: indeed, that call to
rte_pcapng_close() might receive a null pointer.

In that case, rte_errno is set to EINVAL. The API is updated accordingly.

Link: https://pvs-studio.com/en/docs/warnings/v522/
Link: https://github.com/DPDK/dpdk/blob/e5176f23ae8b31437c3e5eb875c81f95bf3a9942/app/test/test_pcapng.c#L438
Fixes: 8d23ce8f5ee9 ("pcapng: add new library for writing pcapng files")
Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>
---
 .mailmap                | 2 +-
 lib/pcapng/rte_pcapng.c | 3 +++
 lib/pcapng/rte_pcapng.h | 2 ++
 3 files changed, 6 insertions(+), 1 deletion(-)
  

2025-02-23 22:41 (UTC+0100), Ariel Otilibili:
> diff --git a/lib/pcapng/rte_pcapng.c b/lib/pcapng/rte_pcapng.c
> index 16485b27cb46..d2cbcea42885 100644
> --- a/lib/pcapng/rte_pcapng.c
> +++ b/lib/pcapng/rte_pcapng.c
> @@ -716,6 +716,9 @@ rte_pcapng_fdopen(int fd,
>  void
>  rte_pcapng_close(rte_pcapng_t *self)
>  {
> +	if (!self)
> +		rte_errno = EINVAL;
> +
>  	close(self->outfd);
>  	free(self);
>  }

Still dereferencing self == NULL.
  

On Sun, 23 Feb 2025 22:41:23 +0100
Ariel Otilibili <ariel.otilibili@6wind.com> wrote:

> rte_pcapng_close() might dereference a null pointer; as example,
> PVS-Studio gives its usage in test_pcapng.c: indeed, that call to
> rte_pcapng_close() might receive a null pointer.
> 
> In that case, rte_errno is set to EINVAL. The API is updated accordingly.
> 
> Link: https://pvs-studio.com/en/docs/warnings/v522/
> Link: https://github.com/DPDK/dpdk/blob/e5176f23ae8b31437c3e5eb875c81f95bf3a9942/app/test/test_pcapng.c#L438
> Fixes: 8d23ce8f5ee9 ("pcapng: add new library for writing pcapng files")
> Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>

The convention (back from Unix) is that errno is only set on failure.
Simpler fix would just to silently ignore NULL case.



diff --git a/lib/pcapng/rte_pcapng.c b/lib/pcapng/rte_pcapng.c
index 16485b27cb..4a0aa6afe5 100644
--- a/lib/pcapng/rte_pcapng.c
+++ b/lib/pcapng/rte_pcapng.c
@@ -716,6 +716,7 @@ rte_pcapng_fdopen(int fd,
 void
 rte_pcapng_close(rte_pcapng_t *self)
 {
-       close(self->outfd);
+       if (self)
+               close(self->outfd);
  

Hello Dmitry, hello Stephen;

On Mon, Feb 24, 2025 at 4:42 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:

> The convention (back from Unix) is that errno is only set on failure.
> Simpler fix would just to silently ignore NULL case.
>

Thanks for your feedback. Here they are addressed,
https://inbox.dpdk.org/dev/20250224164031.587829-1-ariel.otilibili@6wind.com/
  

Recheck-request: iol-marvell-Functional

Putting in a recheck as I believe the fail CI reported is false.

On Sun, Feb 23, 2025 at 4:41 PM Ariel Otilibili <ariel.otilibili@6wind.com>
wrote:

> rte_pcapng_close() might dereference a null pointer; as example,
> PVS-Studio gives its usage in test_pcapng.c: indeed, that call to
> rte_pcapng_close() might receive a null pointer.
>
> In that case, rte_errno is set to EINVAL. The API is updated accordingly.
>
> Link: https://pvs-studio.com/en/docs/warnings/v522/
> Link:
> https://github.com/DPDK/dpdk/blob/e5176f23ae8b31437c3e5eb875c81f95bf3a9942/app/test/test_pcapng.c#L438
> Fixes: 8d23ce8f5ee9 ("pcapng: add new library for writing pcapng files")
> Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>
> ---
>  .mailmap                | 2 +-
>  lib/pcapng/rte_pcapng.c | 3 +++
>  lib/pcapng/rte_pcapng.h | 2 ++
>  3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/.mailmap b/.mailmap
> index a03d3cfb591b..ea68d6180ccc 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -135,7 +135,7 @@ Anupam Kapoor <anupam.kapoor@gmail.com>
>  Apeksha Gupta <apeksha.gupta@nxp.com>
>  Archana Muniganti <marchana@marvell.com> <
> muniganti.archana@caviumnetworks.com>
>  Archit Pandey <architpandeynitk@gmail.com>
> -Ariel Otilibili <otilibil@eurecom.fr> <ariel.otilibili@6wind.com>
> +Ariel Otilibili <ariel.otilibili@6wind.com> <otilibil@eurecom.fr>
>  Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>  Arkadiusz Kusztal <arkadiuszx.kusztal@intel.com>
>  Arnaud Fiorini <arnaud.fiorini@polymtl.ca>
> diff --git a/lib/pcapng/rte_pcapng.c b/lib/pcapng/rte_pcapng.c
> index 16485b27cb46..d2cbcea42885 100644
> --- a/lib/pcapng/rte_pcapng.c
> +++ b/lib/pcapng/rte_pcapng.c
> @@ -716,6 +716,9 @@ rte_pcapng_fdopen(int fd,
>  void
>  rte_pcapng_close(rte_pcapng_t *self)
>  {
> +       if (!self)
> +               rte_errno = EINVAL;
> +
>         close(self->outfd);
>         free(self);
>  }
> diff --git a/lib/pcapng/rte_pcapng.h b/lib/pcapng/rte_pcapng.h
> index 48f2b5756430..f7b976987320 100644
> --- a/lib/pcapng/rte_pcapng.h
> +++ b/lib/pcapng/rte_pcapng.h
> @@ -60,6 +60,8 @@ rte_pcapng_fdopen(int fd,
>   *
>   * @param self
>   *  handle to library
> +
> + * If self is NULL, rte_errno is set to EINVAL.
>   */
>  void
>  rte_pcapng_close(rte_pcapng_t *self);
> --
> 2.30.2
>
>
  

Hello Patrick,

On Tue, Feb 25, 2025 at 9:42 PM Patrick Robb <probb@iol.unh.edu> wrote:

> Recheck-request: iol-marvell-Functional
>
> Putting in a recheck as I believe the fail CI reported is false.
>
>>
>>
Thanks for the heads up. This patch did supersede version 2,
https://patches.dpdk.org/project/dpdk/patch/20250224164031.587829-2-ariel.otilibili@6wind.com/

Regards,
Ariel
  

Thanks Ariel,

If you submitted another version that supersedes it, I think you need to
mark this superseded:
https://patchwork.dpdk.org/project/dpdk/patch/20250223214123.447579-2-ariel.otilibili@6wind.com/

Best,
Patrick