UBSan flagged a legitimate warning [1] with this approach. There are
existing use cases in the codebase that pass ptr as NULL.
Before this change: NULL (0) is cast to uintptr_t. The addition
happens in the integer domain, which is valid defined behavior.
After this change: NULL is cast to char*. Performing arithmetic on a
NULL pointer ((char*)0 + x) is Undefined Behavior in C, even if the
offset is 0.
An example of this occurring is shown in [2].
We have a few options to resolve this:
1. Add runtime NULL checks into RTE_PTR_ADD -> Undesirable due to
runtime overhead in a hot-path macro.
2. Clarify that the caller is responsible for NULL checks -> Require
audit & update call sites in DPDK (and potentially breaking external
applications) to ensure NULL is never passed.
3. Revisit the API -> Existing API is convenient but limits type
safety, discards qualifiers (in caller code), and obscures pointer
provenance from the compiler (preventing optimizations). Here are
options to split the API:
Option 3.1: Distinct APIs per use case (Split Const/Non-Const) This
splits the macro into three explicit variants.
/* Add a byte-value offset to an integer address */
#define RTE_INT_PTR_ADD(intptr, x) ((typeof(intptr))((uintptr_t)(intptr) + (x)))
/* Add a byte-value offset to a non-const pointer */
#define RTE_PTR_ADD(ptr, x) ((void *)((char *)(ptr) + (x)))
/* Add a byte-value offset to a const pointer */
#define RTE_CONST_PTR_ADD(ptr, x) ((const void *)((const char *)(ptr) + (x)))
Pros:
Simpler Implementation: No suppressions or casting under the hood.
Cons:
Pointer API fragmentation: Callers must manually select the right macro.
Dependency Cascade: Dependent macros like RTE_PTR_ALIGN_CEIL rely on
RTE_PTR_ADD. Splitting RTE_PTR_ADD would also split RTE_PTR_ALIGN_CEIL
(into const and non-const variants). If we change CEIL then
RTE_PTR_ALIGN_FLOOR should be considered for consistency too.
Option 3.2: Pointer API and Integer-Address API (Preferred) This
separates integer use cases but keeps a single unified macro for
pointers that preserves const correctness automatically.
/* Add a byte-value offset to an integer address */
#define RTE_INT_PTR_ADD(intptr, x) ((typeof(intptr))((uintptr_t)(intptr) + (x)))
/* Add a byte-value offset to a pointer. Returns the same type as the
input (preserving const/volatile). */
#define RTE_PTR_ADD(ptr, x) \
(__extension__ ({ \
__rte_diagnostic_push \
__rte_diagnostic_ignored_wcast_qual \
typeof(ptr) __rte_ptr_add_result = \
(typeof(ptr))((char *)(ptr) + (x)); \
__rte_diagnostic_pop \
__rte_ptr_add_result; \
}))
Pros:
Preserves Optimization: Uses char* arithmetic, allowing the compiler
to track pointer provenance.
Better Ergonomics: Callers don't need to distinguish between const and
non-const.
Existing Precedent: Following the existing pattern of RTE_PTR_ALIGN
which returns typeof(ptr).
Cons:
Requires warning suppressions: Suppressions are localized within the
macro and don't affect user code, and the typeof() return ensures type
safety at the call site.
I will submit a draft for v11 that implements option 3.2. If people
like this approach we can discuss if/how breaking into multiple
patches makes sense.
[1] ../lib/eal/common/eal_common_memory.c:335:9: runtime error:
applying zero offset to null pointer
[2] github.com/DPDK/dpdk/blob/main/lib/eal/common/eal_common_memory.c#L335
start = msl->base_va; // void* (can be NULL)
end = RTE_PTR_ADD(start, msl->len); // UB if start is NULL
if (addr >= start && addr < end)
break;
