[v2,07/10] tailq: fix cast macro for null pointer

Message ID 20250623135242.461965-8-david.marchand@redhat.com (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers
Series Run with UBSan in GHA |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

David Marchand June 23, 2025, 1:52 p.m. UTC
Doing arithmetics with the NULL pointer is undefined.

Caught by UBSan:

../app/test/test_tailq.c:111:9: runtime error:
	member access within null pointer of type 'struct rte_tailq_head'

Fixes: f6b4f6c9c123 ("tailq: use a single cast macro")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/eal/include/rte_tailq.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Marat Khalili June 30, 2025, 4:06 p.m. UTC | #1
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday 23 June 2025 14:53
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Bruce Richardson <bruce.richardson@intel.com>; Tyler
> Retzlaff <roretzla@linux.microsoft.com>; Neil Horman
> <nhorman@tuxdriver.com>
> Subject: [PATCH v2 07/10] tailq: fix cast macro for null pointer
> 
> Doing arithmetics with the NULL pointer is undefined.
> 
> Caught by UBSan:
> 
> ../app/test/test_tailq.c:111:9: runtime error:
> 	member access within null pointer of type 'struct rte_tailq_head'
> 
> Fixes: f6b4f6c9c123 ("tailq: use a single cast macro")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/eal/include/rte_tailq.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/eal/include/rte_tailq.h b/lib/eal/include/rte_tailq.h
> index 89f7ef2134..c23df77d96 100644
> --- a/lib/eal/include/rte_tailq.h
> +++ b/lib/eal/include/rte_tailq.h
> @@ -54,7 +54,7 @@ struct rte_tailq_elem {
>   * Return the first tailq entry cast to the right struct.
>   */
>  #define RTE_TAILQ_CAST(tailq_entry, struct_name) \
> -	(struct struct_name *)&(tailq_entry)->tailq_head
> +	(tailq_entry == NULL ? NULL : (struct struct_name *)&(tailq_entry)-
> >tailq_head)
> 
>  /**
>   * Utility macro to make looking up a tailqueue for a particular struct easier.

First tailq_entry is missing parentheses. Also, it is worrying that we now use macro argument twice. E.g. RTE_TAILQ_LOOKUP may become twice slower as a result.

Could we perhaps simplify the macro to `(struct struct_name *)(tailq_entry)`. I tried to find or understand the reasons behind the original construction, but could not.
  
David Marchand July 16, 2025, 8:25 a.m. UTC | #2
On Mon, Jun 30, 2025 at 6:06 PM Marat Khalili <marat.khalili@huawei.com> wrote:
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Monday 23 June 2025 14:53
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Bruce Richardson <bruce.richardson@intel.com>; Tyler
> > Retzlaff <roretzla@linux.microsoft.com>; Neil Horman
> > <nhorman@tuxdriver.com>
> > Subject: [PATCH v2 07/10] tailq: fix cast macro for null pointer
> >
> > Doing arithmetics with the NULL pointer is undefined.
> >
> > Caught by UBSan:
> >
> > ../app/test/test_tailq.c:111:9: runtime error:
> >       member access within null pointer of type 'struct rte_tailq_head'
> >
> > Fixes: f6b4f6c9c123 ("tailq: use a single cast macro")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  lib/eal/include/rte_tailq.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/eal/include/rte_tailq.h b/lib/eal/include/rte_tailq.h
> > index 89f7ef2134..c23df77d96 100644
> > --- a/lib/eal/include/rte_tailq.h
> > +++ b/lib/eal/include/rte_tailq.h
> > @@ -54,7 +54,7 @@ struct rte_tailq_elem {
> >   * Return the first tailq entry cast to the right struct.
> >   */
> >  #define RTE_TAILQ_CAST(tailq_entry, struct_name) \
> > -     (struct struct_name *)&(tailq_entry)->tailq_head
> > +     (tailq_entry == NULL ? NULL : (struct struct_name *)&(tailq_entry)-
> > >tailq_head)
> >
> >  /**
> >   * Utility macro to make looking up a tailqueue for a particular struct easier.
>
> First tailq_entry is missing parentheses. Also, it is worrying that we now use macro argument twice. E.g. RTE_TAILQ_LOOKUP may become twice slower as a result.
>
> Could we perhaps simplify the macro to `(struct struct_name *)(tailq_entry)`. I tried to find or understand the reasons behind the original construction, but could not.

Thanks for the comment, it made me relookup at this code.

I realise this macro should be called with a correct object in the first place.
Instead the lookup macro itself is the part that is undefined behavior
(wrt comment in case of an error).
  

Patch

diff --git a/lib/eal/include/rte_tailq.h b/lib/eal/include/rte_tailq.h
index 89f7ef2134..c23df77d96 100644
--- a/lib/eal/include/rte_tailq.h
+++ b/lib/eal/include/rte_tailq.h
@@ -54,7 +54,7 @@  struct rte_tailq_elem {
  * Return the first tailq entry cast to the right struct.
  */
 #define RTE_TAILQ_CAST(tailq_entry, struct_name) \
-	(struct struct_name *)&(tailq_entry)->tailq_head
+	(tailq_entry == NULL ? NULL : (struct struct_name *)&(tailq_entry)->tailq_head)
 
 /**
  * Utility macro to make looking up a tailqueue for a particular struct easier.