Build problem with ToT GCC
diff mbox

Message ID 20150417192032.70DE42C3B91@topped-with-meat.com
State New
Headers show

Commit Message

Roland McGrath April 17, 2015, 7:20 p.m. UTC
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.

Comments

Steve Ellcey April 17, 2015, 7:51 p.m. UTC | #1
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
Roland McGrath April 17, 2015, 7:58 p.m. UTC | #2
> 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.
Steve Ellcey April 17, 2015, 8:02 p.m. UTC | #3
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
Steve Ellcey April 17, 2015, 8:46 p.m. UTC | #4
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

Patch
diff mbox

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()"));