Re: [PATCH v2] tools/mm/slabinfo: fix access to null terminator in string boundary

[SeongJae Park <sj@kernel.org>]

On Sat, 30 Aug 2025 22:50:22 +0530 Kaushlendra Kumar <kaushlendra.kumar@intel.com> wrote:

> The current code incorrectly accesses buffer[strlen(buffer)], which
> points to the null terminator ('\0') at the end of the string. This is
> technically out-of-bounds access since valid string content ends at
> index strlen(buffer)-1.
> 
> Fix by:
> 1. Declaring strlen() result variable at function scope
> 2. Adding bounds check (len > 0) to handle empty strings
> 3. Using buffer[len-1] to correctly access the last character before
>    the null terminator
> 
> v2: Move variable declaration to function scope to avoid mixing
>     declarations with statements (feedback from reviewer)

Thank you for making this change.  But, it is conventional to put the above
changelog on the comment section (after '---' line).

[1] https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format

> 
> Signed-off-by: Kaushlendra Kumar <kaushlendra.kumar@intel.com>
> ---
>  tools/mm/slabinfo.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/mm/slabinfo.c b/tools/mm/slabinfo.c
> index 1433eff99feb..d2e5b4e232b1 100644
> --- a/tools/mm/slabinfo.c
> +++ b/tools/mm/slabinfo.c
> @@ -155,6 +155,8 @@ static void usage(void)
>  
>  static unsigned long read_obj(const char *name)
>  {
> +	size_t len;
> +

I think the above blank line is not really needed.

>  	FILE *f = fopen(name, "r");
>  
>  	if (!f) {
> @@ -165,8 +167,10 @@ static unsigned long read_obj(const char *name)
>  		if (!fgets(buffer, sizeof(buffer), f))
>  			buffer[0] = 0;
>  		fclose(f);
> -		if (buffer[strlen(buffer)] == '\n')
> -			buffer[strlen(buffer)] = 0;
> +		len = strlen(buffer);
> +

The above balnk line seems not needed to me.

> +		if (len > 0 && buffer[len - 1] == '\n')
> +			buffer[len - 1] = 0;
>  	}
>  	return strlen(buffer);
>  }
> -- 
> 2.34.1