Build problem with ToT GCC

Build problem with ToT GCC

[Steve Ellcey]

I just ran into a build problem building the latest top-of-tree glibc
with the latest top-of-tree GCC.  The GCC change was a fix to PR 64277
(in the GCC bugzilla database).  See:

	https://gcc.gnu.org/ml/gcc-patches/2015-04/msg00919.html

Here is the problem from a glibc point of view (when SHARED is not set).
I don't know if we want to change the code or not or what the right change
would be if we did.

Steve Ellcey
sellcey@imgtec.com


In sysdeps/generic/ldsodefs.h we have:

	/* Non-shared code has no support for multiple namespaces.  */
	#ifdef SHARED
	# define DL_NNS 16
	#else
	# define DL_NNS 1
	#endif
  	EXTERN struct link_namespaces
  	{
	.
	.
  	} _dl_ns[DL_NNS];

In dlfcn/dlfcn.h we have:

	# define LM_ID_BASE	0	/* Initial namespace.  */



And in elf/dl-open.c (_dl_open) we have:

  else if (__builtin_expect (nsid != LM_ID_BASE && nsid != __LM_ID_CALLER, 0)
           && (GL(dl_ns)[nsid]._ns_nloaded == 0
               || GL(dl_ns)[nsid]._ns_loaded->l_auditing))
    _dl_signal_error (EINVAL, file, NULL,
                      N_("invalid target namespace in dlmopen()"));


So GCC seems to see that for 'GL(dl_ns)[nsid]', nsid cannot be 0.  But 0
is the only legal index for dl_ns so it gives this warning/error:

dl-open.c: In function '_dl_open':
dl-open.c:623:18: error: array subscript is outside array bounds [-Werror=array-bounds]
     && (GL(dl_ns)[nsid]._ns_nloaded == 0
                  ^
dl-open.c:624:21: error: array subscript is outside array bounds [-Werror=array-bounds]
         || GL(dl_ns)[nsid]._ns_loaded->l_auditing))

Re: Build problem with ToT GCC

[Roland McGrath]

Can you try this change (on branch roland/dl-nns) with that compiler?
I suspect a compile-time constant preventing evaluation of the
expressions doing indexing will avoid the warning.  If it doesn't,
then the right thing to do is to put that inside #if DL_NNS > 1.

While I was there I noticed that it's not properly checking for wildly
bogus NSID values that would make that indexing bogus at runtime (in
the SHARED case), so I put that in too.


Thanks,
Roland


2015-04-17  Roland McGrath  <roland@hack.frob.com>

	* elf/dl-open.c (_dl_open): Use __glibc_unlikely in invalid namespace
	check.  Reject NSID < 0 and NSID >= dl_nns, and check for DL_NNS==1,
	before using NSID as an index.

diff --git a/elf/dl-open.c b/elf/dl-open.c
index 0dbe07f..2d0e082 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -619,8 +619,14 @@ no more namespaces available for dlmopen()"));
   /* Never allow loading a DSO in a namespace which is empty.  Such
      direct placements is only causing problems.  Also don't allow
      loading into a namespace used for auditing.  */
-  else if (__builtin_expect (nsid != LM_ID_BASE && nsid != __LM_ID_CALLER, 0)
-	   && (GL(dl_ns)[nsid]._ns_nloaded == 0
+  else if (__glibc_unlikely (nsid != LM_ID_BASE && nsid != __LM_ID_CALLER)
+	   && (__glibc_unlikely (nsid < 0 || nsid >= GL(dl_nns))
+	       /* This prevents the [NSID] index expressions from being
+		  evaluated, so the compiler won't think that we are
+		  accessing an invalid index here in the !SHARED case where
+		  DL_NNS is 1 and so any NSID != 0 is invalid.  */
+	       || DL_NNS == 1
+	       || GL(dl_ns)[nsid]._ns_nloaded == 0
 	       || GL(dl_ns)[nsid]._ns_loaded->l_auditing))
     _dl_signal_error (EINVAL, file, NULL,
 		      N_("invalid target namespace in dlmopen()"));

Re: Build problem with ToT GCC

[Steve Ellcey]

On Fri, 2015-04-17 at 12:20 -0700, Roland McGrath wrote:
> Can you try this change (on branch roland/dl-nns) with that compiler?
> I suspect a compile-time constant preventing evaluation of the
> expressions doing indexing will avoid the warning.  If it doesn't,
> then the right thing to do is to put that inside #if DL_NNS > 1.
> 
> While I was there I noticed that it's not properly checking for wildly
> bogus NSID values that would make that indexing bogus at runtime (in
> the SHARED case), so I put that in too.
> 
> 
> Thanks,
> Roland
> 
> 
> 2015-04-17  Roland McGrath  <roland@hack.frob.com>
> 
> 	* elf/dl-open.c (_dl_open): Use __glibc_unlikely in invalid namespace
> 	check.  Reject NSID < 0 and NSID >= dl_nns, and check for DL_NNS==1,
> 	before using NSID as an index.

This patch did fix the problem in dl-open.c but dl-close.c has the same
issue and would need the same fix.

Steve Ellcey
sellcey@imgtec.com

Re: Build problem with ToT GCC

[Roland McGrath]

> This patch did fix the problem in dl-open.c but dl-close.c has the same
> issue and would need the same fix.

Please show that error.

Re: Build problem with ToT GCC

[Steve Ellcey]

On Fri, 2015-04-17 at 12:58 -0700, Roland McGrath wrote:
> > This patch did fix the problem in dl-open.c but dl-close.c has the same
> > issue and would need the same fix.
> 
> Please show that error.

dl-close.c: In function '_dl_close_worker':
dl-close.c:136:42: error: array subscript is outside array bounds
[-Werror=array-bounds]
   struct link_namespaces *ns = &GL(dl_ns)[nsid];
                                          ^
cc1: all warnings being treated as errors

Re: Build problem with ToT GCC

[Steve Ellcey]

On Fri, 2015-04-17 at 13:02 -0700, Steve Ellcey wrote:
> On Fri, 2015-04-17 at 12:58 -0700, Roland McGrath wrote:
> > > This patch did fix the problem in dl-open.c but dl-close.c has the same
> > > issue and would need the same fix.
> > 
> > Please show that error.
> 
> dl-close.c: In function '_dl_close_worker':
> dl-close.c:136:42: error: array subscript is outside array bounds
> [-Werror=array-bounds]
>    struct link_namespaces *ns = &GL(dl_ns)[nsid];
>                                           ^
> cc1: all warnings being treated as errors

Weird, I assumed that the dl-close.c issue was the same as the dl-open.c
problem.  But it looks different.  After cutting it down with delta I
get the following small test case and error.  I do not see how GCC can
know that nsid is not 0.

Steve Ellcey
sellcey@imgtec.com


extern void bad (const char *__assertion) __attribute__ ((__nothrow__ ))
__attribute__ ((__noreturn__));
struct link_map { long int l_ns; };
extern struct link_namespaces
  {
    unsigned int _ns_nloaded;
  } _dl_ns[1];
void _dl_close_worker (struct link_map *map)
{
  long int nsid = map->l_ns;
  struct link_namespaces *ns = &_dl_ns[nsid];
  (nsid != 0) ? (void) (0) : bad ("nsid != 0");
  --ns->_ns_nloaded;


% inst*/bin/*-gcc -O2 -Wall -Werror x.c
x.c: In function '_dl_close_worker':
x.c:10:39: error: array subscript is outside array bounds [-Werror=array-bounds]
   struct link_namespaces *ns = &_dl_ns[nsid];
                                       ^
x.c:10:39: error: array subscript is outside array bounds [-Werror=array-bounds]
cc1: all warnings being treated as errors

[COMMITTED PATCH] Fuller check for invalid NSID in _dl_open.

[Roland McGrath]

This is the patch I just posted under:
	Subject: Re: Build problem with ToT GCC
But in case some people didn't notice it was a proposed patch,
here it is again, just committed after Steve's verification that
it fixes the trunk-gcc warning.


Thanks,
Roland


2015-04-17  Roland McGrath  <roland@hack.frob.com>

	* elf/dl-open.c (_dl_open): Use __glibc_unlikely in invalid namespace
	check.  Reject NSID < 0 and NSID >= dl_nns, and check for DL_NNS==1,
	before using NSID as an index.

diff --git a/elf/dl-open.c b/elf/dl-open.c
index 0dbe07f..2d0e082 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -619,8 +619,14 @@ no more namespaces available for dlmopen()"));
   /* Never allow loading a DSO in a namespace which is empty.  Such
      direct placements is only causing problems.  Also don't allow
      loading into a namespace used for auditing.  */
-  else if (__builtin_expect (nsid != LM_ID_BASE && nsid != __LM_ID_CALLER, 0)
-	   && (GL(dl_ns)[nsid]._ns_nloaded == 0
+  else if (__glibc_unlikely (nsid != LM_ID_BASE && nsid != __LM_ID_CALLER)
+	   && (__glibc_unlikely (nsid < 0 || nsid >= GL(dl_nns))
+	       /* This prevents the [NSID] index expressions from being
+		  evaluated, so the compiler won't think that we are
+		  accessing an invalid index here in the !SHARED case where
+		  DL_NNS is 1 and so any NSID != 0 is invalid.  */
+	       || DL_NNS == 1
+	       || GL(dl_ns)[nsid]._ns_nloaded == 0
 	       || GL(dl_ns)[nsid]._ns_loaded->l_auditing))
     _dl_signal_error (EINVAL, file, NULL,
 		      N_("invalid target namespace in dlmopen()"));

Re: Build problem with ToT GCC

[Roland McGrath]

> Weird, I assumed that the dl-close.c issue was the same as the dl-open.c
> problem.  But it looks different.  After cutting it down with delta I
> get the following small test case and error.  I do not see how GCC can
> know that nsid is not 0.

It's because of the call to a noreturn function ("bad"):

>   struct link_namespaces *ns = &_dl_ns[nsid];
>   (nsid != 0) ? (void) (0) : bad ("nsid != 0");
>   --ns->_ns_nloaded;

If 'nsid != 0' is not true, then you never reach the last line,
which is the only one actually dereferencing _dl_ns[nsid].

It's rather confusing that it only reports the error at the site of
the address calculation and does not show the site of the dereference,
let alone the site of the preceding code path that made the compiler
believe the value of 'nsid' was constrained (which is a handful of
lines earlier: 'assert (nsid != LM_ID_BASE)').  In your minimized
example is easy enough to see the relationship.  But in the original
code, 'ns = &_dl_ns[nsid]' appears over 500 lines before
'--ns->_ns_nloaded', which itself is several lines after the assert.
The message "subscript is outside" is also somewhat misleading for
this case, because the assert that testifies the value cannot be zero
is only in one branch of an if test--in the other branch, there is no
such assert and so the cited dereference might be just fine (which is
the case here, as it's dynamically impossible for the if test to fail,
for reasons nobody would expect the compiler to figure out).  The
ideal message would be something like "subscript is outside array
bounds in some code paths" followed by a "note: code paths passing
through here" for the line with the assert.  You should file a GCC bug
about improving the diagnostics for this case.

Please try this patch (branch roland/dl-nns):

	* elf/dl-close.c (_dl_close_worker) [DL_NNS == 1]: Just assert that
	IMAP->l_prev cannot be null, and #if out the code for the contrary
	case, avoiding 'assert (nsid != LM_ID_BASE)' making the compiler
	believe that NS (&_dl_ns[NSID]) could point outside the array.

diff --git a/elf/dl-close.c b/elf/dl-close.c
index cf8f9e0..412f71d 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -641,9 +641,16 @@ _dl_close_worker (struct link_map *map)
 	  DL_UNMAP (imap);
 
 	  /* Finally, unlink the data structure and free it.  */
-	  if (imap->l_prev != NULL)
-	    imap->l_prev->l_next = imap->l_next;
-	  else
+#if DL_NNS == 1
+	  /* The assert in the (imap->l_prev == NULL) case gives
+	     the compiler license to warn that NS points outside
+	     the dl_ns array bounds in that case (as nsid != LM_ID_BASE
+	     is tantamount to nsid >= DL_NNS).  That should be impossible
+	     in this configuration, so just assert about it instead.  */
+	  assert (nsid == LM_ID_BASE);
+	  assert (imap->l_prev != NULL);
+#else
+	  if (imap->l_prev == NULL)
 	    {
 	      assert (nsid != LM_ID_BASE);
 	      ns->_ns_loaded = imap->l_next;
@@ -652,6 +659,9 @@ _dl_close_worker (struct link_map *map)
 		 we leave for debuggers to examine.  */
 	      r->r_map = (void *) ns->_ns_loaded;
 	    }
+	  else
+#endif
+	    imap->l_prev->l_next = imap->l_next;
 
 	  --ns->_ns_nloaded;
 	  if (imap->l_next != NULL)


Thanks,
Roland

Re: Build problem with ToT GCC

[Steve Ellcey]

On Fri, 2015-04-17 at 14:31 -0700, Roland McGrath wrote:

> Please try this patch (branch roland/dl-nns):
> 
> 	* elf/dl-close.c (_dl_close_worker) [DL_NNS == 1]: Just assert that
> 	IMAP->l_prev cannot be null, and #if out the code for the contrary
> 	case, avoiding 'assert (nsid != LM_ID_BASE)' making the compiler
> 	believe that NS (&_dl_ns[NSID]) could point outside the array.

This patch allowed me to compile elf/dl-close.c and my glibc build
completed without any other problems.

Steve Ellcey
sellcey@imgtec.com

Re: Build problem with ToT GCC

[Roland McGrath]

> This patch allowed me to compile elf/dl-close.c and my glibc build
> completed without any other problems.

Thanks.  I verified that it caused no 'make check' regressions for
me on x86_64-linux-gnu (GCC 4.8.2).  I've committed it now.


Thanks,
Roland