Re: [PATCH v2] builtin/gc: protect against sysctl() failure in total_ram

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 2, 2025, 18:42 UTC
Message-ID: <xmqqikkacu30.fsf@gitster.g>
In-Reply-To: <20250702154649.44210-1-carenas@gmail.com>

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

Show 5 quoted lines

> In the unlikely scenario that sysctl() fails, and uninitialized
> value could be returned.
>
> Initialize the variable used and make sure its expected size
> was correct before using it.

Oh, that's interesting. I wonder if the system returns physical memory in 32 bits, we would want fall back doing something silly like

	if (!sysctl(mib, 2, &i64_tmp, &length, NULL, 0)) {
		if (length == sizeof(i64_tmp))
			return i64_tmp;
		else if (length == 4 &&
                	 !sysctl(mib, 2, &i32_tmp, &length, NULL, 0) &&
			length == 4)
			return i32_tmp;
	}

> While at it, add a cast for consistency.

OK, I do not mind being more explicit than necessary, but wouldn't "return X" take care of casting X to the expected return type of that function?

Anyway, thanks for improvements.  Will queue.

Show 32 quoted lines

> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> V1 would regress FreeBSD, so instead make sure that the obsoleted name isn't
> used in OpenBSD/NetBSD instead
>
>  builtin/gc.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 845876ff02..3958707feb 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -539,7 +539,7 @@ static uint64_t total_ram(void)
>  		return total;
>  	}
>  #elif defined(HAVE_BSD_SYSCTL) && (defined(HW_MEMSIZE) || defined(HW_PHYSMEM) || defined(HW_PHYSMEM64))
> -	int64_t physical_memory;
> +	int64_t physical_memory = 0;
>  	int mib[2];
>  	size_t length;
>  
> @@ -552,8 +552,9 @@ static uint64_t total_ram(void)
>  	mib[1] = HW_PHYSMEM;
>  # endif
>  	length = sizeof(int64_t);
> -	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0))
> -		return physical_memory;
> +	if (!sysctl(mib, 2, &physical_memory, &length, NULL, 0) &&
> +	    length == sizeof(int64_t))
> +		return (uint64_t)physical_memory;
>  #elif defined(GIT_WINDOWS_NATIVE)
>  	MEMORYSTATUSEX memInfo;