[PATCH 1/2] explicit_bzero-tests: pacify GCC

[PATCH 1/2] explicit_bzero-tests: pacify GCC

[Paul Eggert]

Redo to pacify -Wmaybe-uninitialized with
GCC 11.1.1 20210531 (Red Hat 11.1.1-3) x86-64.
* tests/test-explicit_bzero.c (stackbuf): New static pointer.
(do_secret_stuff): Use it.
(test_stack): Set it to a local buffer.
---
 ChangeLog                   | 7 +++++++
 tests/test-explicit_bzero.c | 8 +++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 6ba44e7df..1caabff62 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
 2021-07-17  Paul Eggert  <eggert@cs.ucla.edu>
 
+	explicit_bzero-tests: pacify GCC
+	Redo to pacify -Wmaybe-uninitialized with
+	GCC 11.1.1 20210531 (Red Hat 11.1.1-3) x86-64.
+	* tests/test-explicit_bzero.c (stackbuf): New static pointer.
+	(do_secret_stuff): Use it.
+	(test_stack): Set it to a local buffer.
+
 	posixtm: pacify latest GCC
 	Also, modernize while I’m at it.
 	* lib/posixtm.c: Include c-ctype.h, idx.h, intprops.h, verify.h
diff --git a/tests/test-explicit_bzero.c b/tests/test-explicit_bzero.c
index cdb839245..c42aba93f 100644
--- a/tests/test-explicit_bzero.c
+++ b/tests/test-explicit_bzero.c
@@ -126,12 +126,12 @@ test_heap (void)
 /* There are two passes:
      1. Put a secret in memory and invoke explicit_bzero on it.
      2. Verify that the memory has been erased.
-   Implement them in the same function, so that they access the same memory
-   range on the stack.  */
+   Access the memory via a volatile pointer, so the compiler
+   does not assume the pointer's value and optimize away accesses.  */
+static char *volatile stackbuf;
 static int _GL_ATTRIBUTE_NOINLINE
 do_secret_stuff (volatile int pass)
 {
-  char stackbuf[SECRET_SIZE];
   if (pass == 1)
     {
       memcpy (stackbuf, SECRET, SECRET_SIZE);
@@ -147,6 +147,8 @@ do_secret_stuff (volatile int pass)
 static void
 test_stack (void)
 {
+  char stack_buffer[SECRET_SIZE];
+  stackbuf = stack_buffer;
   int count = 0;
   int repeat;
 
-- 
2.31.1

[PATCH 2/2] memrchr-tests: pacify GCC

[Paul Eggert]

Pacify GCC 11.1.1 20210531 (Red Hat 11.1.1-3) x86-64.
* tests/test-memrchr.c: Disable -Wmaybe-uninitialized.
---
 ChangeLog            | 4 ++++
 tests/test-memrchr.c | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 1caabff62..c50808437 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
 2021-07-17  Paul Eggert  <eggert@cs.ucla.edu>
 
+	memrchr-tests: pacify GCC
+	Pacify GCC 11.1.1 20210531 (Red Hat 11.1.1-3) x86-64.
+	* tests/test-memrchr.c: Disable -Wmaybe-uninitialized.
+
 	explicit_bzero-tests: pacify GCC
 	Redo to pacify -Wmaybe-uninitialized with
 	GCC 11.1.1 20210531 (Red Hat 11.1.1-3) x86-64.
diff --git a/tests/test-memrchr.c b/tests/test-memrchr.c
index 24719ffb9..9df55c158 100644
--- a/tests/test-memrchr.c
+++ b/tests/test-memrchr.c
@@ -27,6 +27,11 @@ SIGNATURE_CHECK (memrchr, void *, (void const *, int, size_t));
 #include "zerosize-ptr.h"
 #include "macros.h"
 
+/* Work around GCC bug 101494.  */
+#if 4 < __GNUC__ + (3 <= __GNUC_MINOR__)
+# pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
+#endif
+
 /* Calculating void * + int is not portable, so this wrapper converts
    to char * to make the tests easier to write.  */
 #define MEMRCHR (char *) memrchr
-- 
2.31.1

Re: [PATCH 1/2] explicit_bzero-tests: pacify GCC

[Bruno Haible]

Hi Paul,

> diff --git a/tests/test-explicit_bzero.c b/tests/test-explicit_bzero.c
> index cdb839245..c42aba93f 100644
> --- a/tests/test-explicit_bzero.c
> +++ b/tests/test-explicit_bzero.c
> @@ -126,12 +126,12 @@ test_heap (void)
>  /* There are two passes:
>       1. Put a secret in memory and invoke explicit_bzero on it.
>       2. Verify that the memory has been erased.
> -   Implement them in the same function, so that they access the same memory
> -   range on the stack.  */
> +   Access the memory via a volatile pointer, so the compiler
> +   does not assume the pointer's value and optimize away accesses.  */
> +static char *volatile stackbuf;
>  static int _GL_ATTRIBUTE_NOINLINE
>  do_secret_stuff (volatile int pass)
>  {
> -  char stackbuf[SECRET_SIZE];
>    if (pass == 1)
>      {
>        memcpy (stackbuf, SECRET, SECRET_SIZE);

I disagree with this change, as it significantly reduces the strength of the
test.

The purpose of the test is to verify that the compiler does not eliminate
a call to explicit_bzero, even if data flow analysis reveals that the stack
area is "dead" at the end of the function.

With this patch, it was turned into a weaker assertion: namely, that the
compiler does not eliminate a call to explicit_bzero if it cannot make
inferences about the pointer argument.

I would suggest to revert this patch, and instead use a #pragma, like you
did in the test-memrchr.c patch.

Bruno

Re: [PATCH 1/2] explicit_bzero-tests: pacify GCC

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1277 bytes --]

On 7/18/21 4:12 AM, Bruno Haible wrote:
> The purpose of the test is to verify that the compiler does not eliminate
> a call to explicit_bzero, even if data flow analysis reveals that the stack
> area is "dead" at the end of the function.
>
> With this patch, it was turned into a weaker assertion: namely, that the
> compiler does not eliminate a call to explicit_bzero if it cannot make
> inferences about the pointer argument.

Strictly speaking, neither assertion is weaker than the other. However, 
I take your point that the patch changes the meaning of the test in an 
undesirable way. I installed the attached to implement your suggestion.

This new diagnostic points out a problem with the test, though. If GCC 
can determine that memcmp reads uninitialized storage, GCC can optimize 
away the memcmp and act as if memcmp returns 0 (or any other constant). 
So the test as it stands is problematic given recent advances in 
practical compilers.

As an aside, I don't understand the discussion of asynchronous signal 
invocations in that test's commentary. There is no asynchronous 
signaling in that code.

(These points are of course low-priority, as explicit_bzero is 
documented to be best-effort as opposed to being a guarantee that the 
information is erased.)



[-- Attachment #2: 0001-explicit_bzero-tests-pacify-GCC-better.patch --]
[-- Type: text/x-patch, Size: 2797 bytes --]

From f9803478355d038aa060d71bdd9eddf2bd43325f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 18 Jul 2021 14:08:56 -0500
Subject: [PATCH] explicit_bzero-tests: pacify GCC better

Problem reported by Bruno Haible in:
https://lists.gnu.org/r/bug-gnulib/2021-07/msg00039.html
* tests/test-explicit_bzero.c: Ignore -Wmaybe-uninitialized.
(stackbuf): Remove this static pointer, reverting recent change.
(do_secret_stuff, test_stack): Revert these related changes too.
---
 ChangeLog                   |  9 +++++++++
 tests/test-explicit_bzero.c | 16 +++++++++++-----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c50808437..d175c39af 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2021-07-18  Paul Eggert  <eggert@cs.ucla.edu>
+
+	explicit_bzero-tests: pacify GCC better
+	Problem reported by Bruno Haible in:
+	https://lists.gnu.org/r/bug-gnulib/2021-07/msg00039.html
+	* tests/test-explicit_bzero.c: Ignore -Wmaybe-uninitialized.
+	(stackbuf): Remove this static pointer, reverting recent change.
+	(do_secret_stuff, test_stack): Revert these related changes too.
+
 2021-07-17  Paul Eggert  <eggert@cs.ucla.edu>
 
 	memrchr-tests: pacify GCC
diff --git a/tests/test-explicit_bzero.c b/tests/test-explicit_bzero.c
index c42aba93f..14f0ead2b 100644
--- a/tests/test-explicit_bzero.c
+++ b/tests/test-explicit_bzero.c
@@ -32,6 +32,12 @@ SIGNATURE_CHECK (explicit_bzero, void, (void *, size_t));
 #include "vma-iter.h"
 #include "macros.h"
 
+/* Suppress GCC warning that do_secret_stuff (2) reads uninitialized
+   local storage.  */
+#if 4 < __GNUC__ + (3 <= __GNUC_MINOR__)
+# pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
+#endif
+
 #define SECRET "xyzzy1729"
 #define SECRET_SIZE 9
 
@@ -126,12 +132,14 @@ test_heap (void)
 /* There are two passes:
      1. Put a secret in memory and invoke explicit_bzero on it.
      2. Verify that the memory has been erased.
-   Access the memory via a volatile pointer, so the compiler
-   does not assume the pointer's value and optimize away accesses.  */
-static char *volatile stackbuf;
+   Implement them in the same function, so that they access the same memory
+   range on the stack.  That way, the test verifies that the compiler
+   does not eliminate a call to explicit_bzero, even if data flow analysis
+   reveals that the stack area is dead at the end of the function.  */
 static int _GL_ATTRIBUTE_NOINLINE
 do_secret_stuff (volatile int pass)
 {
+  char stackbuf[SECRET_SIZE];
   if (pass == 1)
     {
       memcpy (stackbuf, SECRET, SECRET_SIZE);
@@ -147,8 +155,6 @@ do_secret_stuff (volatile int pass)
 static void
 test_stack (void)
 {
-  char stack_buffer[SECRET_SIZE];
-  stackbuf = stack_buffer;
   int count = 0;
   int repeat;
 
-- 
2.25.1

Re: [PATCH 1/2] explicit_bzero-tests: pacify GCC

[Bruno Haible]

Paul Eggert wrote:
> I take your point that the patch changes the meaning of the test in an 
> undesirable way. I installed the attached to implement your suggestion.

Thank you.

> This new diagnostic points out a problem with the test, though. If GCC 
> can determine that memcmp reads uninitialized storage, GCC can optimize 
> away the memcmp and act as if memcmp returns 0 (or any other constant). 
> So the test as it stands is problematic given recent advances in 
> practical compilers.

Such compiler optimizations would need to be backed by the standards.
Are there initiatives to "outlaw" references to uninitialized storage
in recent C or C++ standards?

I hope code such as

  int x; /* uninitialized */
  if (!((x & 1) == 0 || (x & 1) == 1))
    abort ();

will never crash. x & 1 can only be 0 or 1. Tertium not datur.

Also, I think the danger is small: GCC does not _know_ that the array
is uninitialized. It's only a "maybe uninitialized". If GCC ever
infers that it is "certainly uninitialized", we could defeat that
through a use of 'volatile', such as

int (* volatile memcmp_func) (const void *, const void *, size_t) = memcmp;

> As an aside, I don't understand the discussion of asynchronous signal 
> invocations in that test's commentary. There is no asynchronous 
> signaling in that code.

In some operating systems, interrupt handling uses the stack of the
currently executing thread. While this is not the case in Linux [1],
it was definitely the case in Atari TOS, and is still probably the
case in some embedded OSes for MMU-less CPUs.

Bruno

[1] https://stackoverflow.com/questions/28759227/which-stack-is-used-by-interrupt-handler-linux

Re: [PATCH 1/2] explicit_bzero-tests: pacify GCC

[Paul Eggert]

On 7/18/21 5:23 PM, Bruno Haible wrote:
>
> Such compiler optimizations would need to be backed by the standards.
> Are there initiatives to "outlaw" references to uninitialized storage
> in recent C or C++ standards?

No initiatives are needed, at least for C. Using uninitialized storage 
is undefined behavior in the current C standard and this has been true 
ever since C was standardized. I imagine C++ is similar.


>
> I hope code such as
>
>    int x; /* uninitialized */
>    if (!((x & 1) == 0 || (x & 1) == 1))
>      abort ();
>
> will never crash. x & 1 can only be 0 or 1. Tertium not datur.

The C standard doesn't guarantee that code will never crash. For 
example, the standard allows an implementation that uses two's 
complement but where INT_MIN == -INT_MAX and where the bit pattern 
0x80000000 is a trap value (i.e., your program aborts if it reads an int 
whose machine value is 0x80000000).


> GCC does not _know_ that the array
> is uninitialized. It's only a "maybe uninitialized".

That's what GCC's diagnostic says, yes. But in cases like these GCC 
actually "knows" that variables are uninitialized and it sometimes 
optimizes based on this knowledge. For example, for:

   _Bool f (void) {  char *p; return !p; }

gcc -O2 (GCC 11.1.1 20210531 (Red Hat 11.1.1-3)) "knows" that P is 
uninitialized and generates code equivalent to that of:

   _Bool f (void) { return 1; }

That is, GCC optimizes away the access to p's value, which GCC can do 
because the behavior is undefined.


>   If GCC ever
> infers that it is "certainly uninitialized", we could defeat that
> through a use of 'volatile', such as

Yes, some use of volatile should do the trick for GCC (which is what my 
patch did). However, one would still have problems with a debugging 
implementation, e.g., if GCC ever supports an -fsanitize=uninitialized 
option that catches use of uninitialized storage.

This is all low priority of course.

Re: [PATCH 1/2] explicit_bzero-tests: pacify GCC

[Bruno Haible]

Paul Eggert wrote:
> No initiatives are needed, at least for C. Using uninitialized storage 
> is undefined behavior in the current C standard and this has been true 
> ever since C was standardized. I imagine C++ is similar.
> ...
> But in cases like these GCC 
> actually "knows" that variables are uninitialized and it sometimes 
> optimizes based on this knowledge. For example, for:
> 
>    _Bool f (void) {  char *p; return !p; }
> 
> gcc -O2 (GCC 11.1.1 20210531 (Red Hat 11.1.1-3)) "knows" that P is 
> uninitialized and generates code equivalent to that of:
> 
>    _Bool f (void) { return 1; }
> 
> That is, GCC optimizes away the access to p's value, which GCC can do 
> because the behavior is undefined.

Ouch, I seriously underrated this warning. Thanks for correcting me.
Indeed, GCC does this optimization already since version 4.3.

> >   If GCC ever
> > infers that it is "certainly uninitialized", we could defeat that
> > through a use of 'volatile', such as
> 
> Yes, some use of volatile should do the trick for GCC (which is what my 
> patch did). However, one would still have problems with a debugging 
> implementation, e.g., if GCC ever supports an -fsanitize=uninitialized 
> option that catches use of uninitialized storage.

Yes, this test probably fails under 'valgrind'.

I don't know a fail-safe workaround. When the purpose is to verify whether
a memory region that has gone out-of-scope has been cleared, it necessarily
means to access this memory region as if it were uninitialized.

Bruno