Re: [PATCH v5 02/16] reftable: fix resource leak in block.c error path

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 13, 2022, 18:52 UTC
Message-ID: <xmqqczkvii3v.fsf@gitster.g>
In-Reply-To: <6993c156-4df1-198f-5ce0-781ba13d5dd2@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 36 quoted lines

> Am 12.01.22 um 12:58 schrieb Han-Wen Nienhuys:
>> On Fri, Dec 24, 2021 at 5:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>>> Once you
>>> initialize at the declaration with "less meaningful" value (like
>>> zero initialization), the tools won't be able to tell when the code
>>> uses that variable "uninitialized" (because the assignment was
>>> skipped by a bug), since it appears to always be initialied to them.
>>
>> Which tools are these? When I add
>>
>> static void test_memcpy(void)
>> {
>>  uint32_t dest;
>>  char not_init[200];
>>  int i;
>>  memcpy(&dest, not_init, sizeof(dest));
>>
>>  for (i = 0 ; i < 10; i++)
>>   not_init[i] = rand() % 255 + 1;
>>  printf("%d", (int) strlen(not_init));
>> }
>>
>> to the C code, it compiles cleanly if I do "make DEVELOPER=1".
>
> https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html says about
> -Wuninitialized:
>
>    "Note that there may be no warning about a variable that is used only
>     to compute a value that itself is never used, because such
>     computations may be deleted by data flow analysis before the
>     warnings are printed."
>
> And indeed, dest is not used above.  But even if we change the funtion
> to use dest by returning it, GCC versions 9.1 and higher don't warn
> about the use of the uninitialized buffer.  GCC 4.7.1 to 8.5 do warn,
> though: https://godbolt.org/z/zYz9KvPdK

What I meant in the original discussion was a runtime checker that notices a read from an uninitialized location (I've also written one myself way before my Git time, which was how I noticed strcpy() on old SunOS, in an attempt to optimize by loading a word at a time, sometimes read one word too much beyond the end of a page).

But if a static analysis may catch itas a potential error, that is another reason to worry about it, too.

The original code wanted to do

	char message[100] = { 0 };

	for (i = 0; i < sizeof(message) - 1; i++)
        	message[i] = something();

	use_NUL_terminated_string(message);

and it allowed to omit an assigmnent

	message[sizeof(message) - 1] = '\0';

after the loop.

As I already said, I do not care too deeply in a test-only helper like this.

Thanks.