|SUCCESS| [6/6] cmdline: add null checks for invalid input

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163792

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-07

Reviewing this series of six patches addressing safety and hardening issues in cfgfile and cmdline libraries.

git sha: [commits 1-6 from series]
Author: Bruce Richardson <bruce.richardson at intel.com>
Subject: [PATCH 1-6/6] cfgfile/cmdline: add null checks and overflow protection

Summary: This patch series adds NULL pointer checks and overflow protection to 
cfgfile and cmdline libraries. The changes include parameter validation, 
allocation overflow guards, and buffer size checks.

[ ... ]

> diff --git a/lib/cfgfile/rte_cfgfile.c b/lib/cfgfile/rte_cfgfile.c
> index c495bdf6ae..25fc792274 100644
> --- a/lib/cfgfile/rte_cfgfile.c
> +++ b/lib/cfgfile/rte_cfgfile.c
> @@ -198,6 +198,10 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
>  		return NULL;
>  
>  	cfg = rte_cfgfile_create(flags);
> +	if (cfg == NULL) {
> +		fclose(f);
> +		return NULL;
> +	}

Does the explicit if statement add value here?  The next line would return NULL 
anyway if cfg is NULL.  Though the explicit fclose is valuable.

[ ... ]

> @@ -523,8 +530,14 @@ rte_cfgfile_sections(struct rte_cfgfile *cfg, char *sections[],
>  {
>  	int i;
>  
> -	for (i = 0; i < cfg->num_sections && i < max_sections; i++)
> +	if (cfg == NULL || sections == NULL || max_sections < 0)
> +		return -1;
> +
> +	for (i = 0; i < cfg->num_sections && i < max_sections; i++) {
> +		if (sections[i] == NULL)
> +			return -1;
>  		strlcpy(sections[i], cfg->sections[i].name, CFG_NAME_LEN);
> +	}

Does this check assume sections[] entries are pre-allocated?  The caller could 
reasonably pass uninitialized array elements.  Is there documentation clarifying 
whether sections[i] must be pre-allocated?

[ ... ]

> diff --git a/lib/cfgfile/rte_cfgfile.c b/lib/cfgfile/rte_cfgfile.c
> index 25fc792274..040351ab4d 100644
> --- a/lib/cfgfile/rte_cfgfile.c
> +++ b/lib/cfgfile/rte_cfgfile.c
> @@ -9,6 +9,7 @@
>  #include <ctype.h>
>  #include <errno.h>
>  #include <limits.h>
> +#include <stdint.h>

Is stdint.h needed?  The patch doesn't appear to introduce new stdint.h types.

[ ... ]

> +/** define a max allocation limit for entry and section types
> + * for 64-bit systems, this is based on INT_MAX since APIs all work on int values.
> + * For 32-bit systems, this is based on SIZE_MAX / sizeof(type) to prevent overflow on allocation.
> + */
> +#define CFG_ALLOC_MAX(type) \
> +	(sizeof(int) == sizeof(size_t) ? (SIZE_MAX / sizeof(type)) : ((size_t)INT_MAX))

Can this macro produce incorrect results when sizeof(int) < sizeof(size_t) but 
not on 64-bit (e.g., some embedded platforms)?  The comment says "32-bit 
systems" but the check is sizeof equality.

[ ... ]

> @@ -118,6 +132,9 @@ _add_entry(struct rte_cfgfile_section *section, const char *entryname,
>  
>  	/* resize entry structure if we don't have room for more entries */
>  	if (section->num_entries == section->allocated_entries) {
> +		if (section->allocated_entries > CFG_ALLOC_ENTRY_MAX - CFG_ALLOC_ENTRY_BATCH)
> +			return -ENOMEM;
> +
>  		struct rte_cfgfile_entry *n_entries = realloc(

Does this overflow check correctly handle the case where allocated_entries is 
exactly 0?  Can the addition in realloc later overflow when allocated_entries 
is close to but not exceeding the check threshold?

[ ... ]

> @@ -507,20 +517,20 @@ int
>  rte_cfgfile_num_sections(struct rte_cfgfile *cfg, const char *sectionname,
>  	size_t length)
>  {
> -	int num_sections = 0;
> -	int i;
> +	size_t num_sections = 0;
>  
>  	if (cfg == NULL)
>  		return -1;
>  
>  	if (sectionname == NULL)
> -		return cfg->num_sections;
> +		return (int)cfg->num_sections;

Can this cast overflow when cfg->num_sections exceeds INT_MAX?  Should there be 
a check before casting?

[ ... ]

> diff --git a/lib/cmdline/cmdline_parse.c b/lib/cmdline/cmdline_parse.c
> index 201fddb8c3..d55c8db19d 100644
> --- a/lib/cmdline/cmdline_parse.c
> +++ b/lib/cmdline/cmdline_parse.c
> @@ -133,7 +133,7 @@ match_inst(cmdline_parse_inst_t *inst, const char *buf,
>  		} else {
>  			unsigned rb_sz;
>  
> -			if (token_hdr.offset > resbuf_size) {
> +			if (token_hdr.offset >= resbuf_size) {

Does changing > to >= introduce any regressions for valid code that previously 
used offset == resbuf_size - 1 legitimately?

[ ... ]

> @@ -519,7 +519,9 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
>  				}
>  				(*state)++;
>  				l=strlcpy(dst, tmpbuf, size);
> -				if (l>=0 && token_hdr.ops->get_help) {
> +				if ((unsigned int)l >= size)
> +					return 1;
> +				if (token_hdr.ops->get_help) {

Is the cast to unsigned int safe?  Can strlcpy ever return negative values that 
should be checked?  According to POSIX, strlcpy returns size_t so it cannot be 
negative, but is the return type correctly declared?

[ ... ]

> diff --git a/lib/cmdline/cmdline_parse_bool.c b/lib/cmdline/cmdline_parse_bool.c
> index e03cc3d545..a3f7adab58 100644
> --- a/lib/cmdline/cmdline_parse_bool.c
> +++ b/lib/cmdline/cmdline_parse_bool.c
> @@ -35,17 +35,30 @@ static cmdline_parse_token_string_t cmd_parse_token_bool = {
>  /* parse string to bool */
>  int
>  cmdline_parse_bool(__rte_unused cmdline_parse_token_hdr_t *tk, const char *srcbuf, void *res,
> -	__rte_unused unsigned int ressize)
> +	unsigned int ressize)
>  {
>  	cmdline_fixed_string_t on_off = {0};
> +	uint8_t val;
> +
> +	if (!srcbuf || !*srcbuf)
> +		return -1;

Does the !*srcbuf check reject empty strings intentionally?  Is this documented 
as expected behavior?

> +
> +	if (res != NULL && ressize < sizeof(uint8_t))
> +		return -1;
> +
>  	if (cmdline_token_string_ops.parse
>  			(&cmd_parse_token_bool.hdr, srcbuf, on_off, sizeof(on_off)) < 0)
>  		return -1;
>  
>  	if (strcmp((char *)on_off, "on") == 0)
> -		*(uint8_t *)res = 1;
> +		val = 1;
>  	else if (strcmp((char *)on_off, "off") == 0)
> -		*(uint8_t *)res = 0;
> +		val = 0;
> +	else
> +		return -1;

Can the string parsing produce values other than "on" or "off"?  The earlier 
parse call returned success, so does this mean parse accepts but this function 
later rejects?

[ ... ]

> diff --git a/lib/cmdline/cmdline_parse_num.c b/lib/cmdline/cmdline_parse_num.c
> index f21796bedb..4cfc900391 100644
> --- a/lib/cmdline/cmdline_parse_num.c
> +++ b/lib/cmdline/cmdline_parse_num.c
> @@ -323,7 +323,7 @@ cmdline_get_help_num(cmdline_parse_token_hdr_t *tk, char *dstbuf, unsigned int s
>  	struct cmdline_token_num_data nd;
>  	int ret;
>  
> -	if (!tk)
> +	if (!tk || !dstbuf || size == 0)
>  		return -1;

Does the function properly handle all error paths after this check?  Can any 
code path after this point dereference dstbuf without checking?