[PATCH] getopt: pacify gcc -Wanalyzer-null-dereference

[PATCH] getopt: pacify gcc -Wanalyzer-null-dereference

[Paul Eggert]

* lib/getopt.c (process_long_option): Simplify logic slightly.
This pacifies gcc -flto -Wanalyzer-null-dereference when compiling
GNU tar on x86-64 with gcc 13.2.1 20231205 (Red Hat 13.2.1-6).
---
 ChangeLog    |  7 +++++++
 lib/getopt.c | 29 ++++++++++++++++-------------
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index a017833f64..0b8c3fc640 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2023-12-11  Paul Eggert  <eggert@cs.ucla.edu>
+
+	getopt: pacify gcc -Wanalyzer-null-dereference
+	* lib/getopt.c (process_long_option): Simplify logic slightly.
+	This pacifies gcc -flto -Wanalyzer-null-dereference when compiling
+	GNU tar on x86-64 with gcc 13.2.1 20231205 (Red Hat 13.2.1-6).
+
 2023-12-10  Pádraig Brady  <P@draigBrady.com>
 
 	bootstrap: fix option propagation with --bootstrap-sync
diff --git a/lib/getopt.c b/lib/getopt.c
index 1e2441c4af..a2a291d697 100644
--- a/lib/getopt.c
+++ b/lib/getopt.c
@@ -223,8 +223,9 @@ process_long_option (int argc, char **argv, const char *optstring,
     {
       /* Didn't find an exact match, so look for abbreviations.  */
       unsigned char *ambig_set = NULL;
-      int ambig_malloced = 0;
-      int ambig_fallback = 0;
+      /* Use simpler fallback diagnostic if ambig_set == &ambig_fallback.  */
+      unsigned char ambig_fallback;
+      void *ambig_malloced = NULL;
       int indfound = -1;
 
       for (p = longopts, option_index = 0; p->name; p++, option_index++)
@@ -242,23 +243,26 @@ process_long_option (int argc, char **argv, const char *optstring,
 		     || pfound->val != p->val)
 	      {
 		/* Second or later nonexact match found.  */
-		if (!ambig_fallback)
+		if (ambig_set != &ambig_fallback)
 		  {
 		    if (!print_errors)
 		      /* Don't waste effort tracking the ambig set if
 			 we're not going to print it anyway.  */
-		      ambig_fallback = 1;
+		      ambig_set = &ambig_fallback;
 		    else if (!ambig_set)
 		      {
 			if (__libc_use_alloca (n_options))
 			  ambig_set = alloca (n_options);
-			else if ((ambig_set = malloc (n_options)) == NULL)
-			  /* Fall back to simpler error message.  */
-			  ambig_fallback = 1;
 			else
-			  ambig_malloced = 1;
+			  {
+			    ambig_malloced = malloc (n_options);
+			    /* Fall back to simpler diagnostic if
+			       memory allocation fails.  */
+			    ambig_set = (ambig_malloced ? ambig_malloced
+					 : &ambig_fallback);
+			  }
 
-			if (ambig_set)
+			if (ambig_set != &ambig_fallback)
 			  {
 			    memset (ambig_set, 0, n_options);
 			    ambig_set[indfound] = 1;
@@ -270,11 +274,11 @@ process_long_option (int argc, char **argv, const char *optstring,
 	      }
 	  }
 
-      if (ambig_set || ambig_fallback)
+      if (ambig_set)
 	{
 	  if (print_errors)
 	    {
-	      if (ambig_fallback)
+	      if (ambig_set == &ambig_fallback)
 		fprintf (stderr, _("%s: option '%s%s' is ambiguous\n"),
 			 argv[0], prefix, d->__nextchar);
 	      else
@@ -296,8 +300,7 @@ process_long_option (int argc, char **argv, const char *optstring,
 		  funlockfile (stderr);
 		}
 	    }
-	  if (ambig_malloced)
-	    free (ambig_set);
+	  free (ambig_malloced);
 	  d->__nextchar += strlen (d->__nextchar);
 	  d->optind++;
 	  d->optopt = 0;
-- 
2.43.0

Re: [PATCH] getopt: pacify gcc -Wanalyzer-null-dereference

[Bruno Haible]

Paul Eggert wrote:
> * lib/getopt.c (process_long_option): Simplify logic slightly.
> This pacifies gcc -flto -Wanalyzer-null-dereference when compiling
> GNU tar on x86-64 with gcc 13.2.1 20231205 (Red Hat 13.2.1-6).

This appears to trade a false alarm for another false alarm. Namely,
now Coverity reports:

270                           }
271                         if (ambig_set)
>>> CID 1574557:  Memory - corruptions  (ARRAY_VS_SINGLETON)
>>> Using "ambig_set" as an array.  This might corrupt or misinterpret adjacent memory locations.
272                           ambig_set[option_index] = 1;

I guess we can ignore it?

Bruno

Re: [PATCH] getopt: pacify gcc -Wanalyzer-null-dereference

[Paul Eggert]

On 2023-12-18 05:04, Bruno Haible wrote:
> Paul Eggert wrote:
>> * lib/getopt.c (process_long_option): Simplify logic slightly.
>> This pacifies gcc -flto -Wanalyzer-null-dereference when compiling
>> GNU tar on x86-64 with gcc 13.2.1 20231205 (Red Hat 13.2.1-6).
> This appears to trade a false alarm for another false alarm. Namely,
> now Coverity reports:
> 
> 270                           }
> 271                         if (ambig_set)
>>>> CID 1574557:  Memory - corruptions  (ARRAY_VS_SINGLETON)
>>>> Using "ambig_set" as an array.  This might corrupt or misinterpret adjacent memory locations.
> 272                           ambig_set[option_index] = 1;
> 
> I guess we can ignore it?

Yes, let's do that. We're already ignoring what must be hundreds of 
Coverity false positives, and there's little harm in ignoring one more.

Re: [PATCH] getopt: pacify gcc -Wanalyzer-null-dereference

[Bruno Haible]

Paul Eggert wrote:
> > Paul Eggert wrote:
> >> * lib/getopt.c (process_long_option): Simplify logic slightly.
> >> This pacifies gcc -flto -Wanalyzer-null-dereference when compiling
> >> GNU tar on x86-64 with gcc 13.2.1 20231205 (Red Hat 13.2.1-6).
> > This appears to trade a false alarm for another false alarm. Namely,
> > now Coverity reports:
> > 
> > 270                           }
> > 271                         if (ambig_set)
> >>>> CID 1574557:  Memory - corruptions  (ARRAY_VS_SINGLETON)
> >>>> Using "ambig_set" as an array.  This might corrupt or misinterpret adjacent memory locations.
> > 272                           ambig_set[option_index] = 1;
> > 
> > I guess we can ignore it?
> 
> Yes, let's do that. We're already ignoring what must be hundreds of 
> Coverity false positives, and there's little harm in ignoring one more.

Unfortunately, this wasn't a false alarm, but a good alarm.

Namely, I see the 'test-getopt-gnu' test fail (via abort()) on
  - Linux/x86_64 with clang+asan+ubsan,
  - OpenBSD 7.2/sparc64,
and crash (via SIGSEGV) on
  - FreeBSD 14.0/powerpc64.

Debugging it with clang+asan+ubsan, it gets an out-of-bounds access
here:

		    if (ambig_set && ambig_set != &ambig_fallback)
		      ambig_set[option_index] = 1;		// <=== getopt.c line 272
		  }

with these local variables:

(gdb) info locals
ambig_set = 0x7ffff5c001a0 ""
ambig_fallback = 0 '\000'
ambig_malloced = 0x0
indfound = 4
nameend = 0x55555576c243 <str+3> ""
namelen = 1
p = 0x555555794b60 <long_options_required+160>
pfound = 0x555555794b40 <long_options_required+128>
n_options = 8
option_index = 5
(gdb) print ambig_set == &ambig_fallback
$5 = 1

This patch fixes it.


2024-01-17  Bruno Haible  <bruno@clisp.org>

	getopt-gnu: Fix out-of-bounds access (regression 2023-12-11).
	* lib/getopt.c (process_long_option): Don't set ambig_set[option_index]
	if ambig_set is &ambig_fallback.

diff --git a/lib/getopt.c b/lib/getopt.c
index 928306304e..f66f119ec5 100644
--- a/lib/getopt.c
+++ b/lib/getopt.c
@@ -268,7 +268,7 @@ process_long_option (int argc, char **argv, const char *optstring,
 			    ambig_set[indfound] = 1;
 			  }
 		      }
-		    if (ambig_set)
+		    if (ambig_set && ambig_set != &ambig_fallback)
 		      ambig_set[option_index] = 1;
 		  }
 	      }

Re: [PATCH] getopt: pacify gcc -Wanalyzer-null-dereference

[Paul Eggert]

Thanks for fixing that. I had misinterpreted the Coverity message; sorry 
about that.