diff mbox

Fix 'array subscript is above array bounds' warning in res_send.c

Message ID 20141216100950.GM30928@spoyarek.pnq.redhat.com
State New
Headers show

Commit Message

Siddhesh Poyarekar Dec. 16, 2014, 10:09 a.m. UTC 
Hi,

I see this warning in my build on F21 x86_64, which seems to be due to
a weak check for array bounds.  Fixed by making the bounds check
stronger.

OK to commit?

Siddhesh

	* resolv/res_send.c (__libc_res_nsend): Fix check for nsmap
	bounds.

Comments

Andreas Schwab Dec. 16, 2014, 10:17 a.m. UTC | #1 
Siddhesh Poyarekar <siddhesh@redhat.com> writes:

> diff --git a/resolv/res_send.c b/resolv/res_send.c
> index 4a95eb8..5a9882c 100644
> --- a/resolv/res_send.c
> +++ b/resolv/res_send.c
> @@ -429,7 +429,7 @@ __libc_res_nsend(res_state statp, const u_char *buf, int buflen,
>  				while (ns < MAXNS
>  				       && EXT(statp).nsmap[ns] != MAXNS)
>  					ns++;
> -				if (ns == MAXNS)
> +				if (ns >= MAXNS)

How is that possible?

Andreas.
Siddhesh Poyarekar Dec. 16, 2014, 10:45 a.m. UTC | #2 
On Tue, Dec 16, 2014 at 11:17:04AM +0100, Andreas Schwab wrote:
> Siddhesh Poyarekar <siddhesh@redhat.com> writes:
> 
> > diff --git a/resolv/res_send.c b/resolv/res_send.c
> > index 4a95eb8..5a9882c 100644
> > --- a/resolv/res_send.c
> > +++ b/resolv/res_send.c
> > @@ -429,7 +429,7 @@ __libc_res_nsend(res_state statp, const u_char *buf, int buflen,
> >  				while (ns < MAXNS
> >  				       && EXT(statp).nsmap[ns] != MAXNS)
> >  					ns++;
> > -				if (ns == MAXNS)
> > +				if (ns >= MAXNS)
> 
> How is that possible?

The warning is seen on -O3.  The actual access beyond array bounds
shouldn't ever happen since nscount is always set to less than MAXNS.
the compiler however does not know this and hence warns for the
possibility of both nscount's being greater than MAXNS.

So this does not actually fix a bug, only the warning.

Siddhesh
Andreas Schwab Dec. 16, 2014, 10:57 a.m. UTC | #3 
Siddhesh Poyarekar <siddhesh@redhat.com> writes:

> So this does not actually fix a bug, only the warning.

So this should be classified as such.

Andreas.
Siddhesh Poyarekar Dec. 16, 2014, 11:26 a.m. UTC | #4 
On Tue, Dec 16, 2014 at 11:57:24AM +0100, Andreas Schwab wrote:
> Siddhesh Poyarekar <siddhesh@redhat.com> writes:
> 
> > So this does not actually fix a bug, only the warning.
> 
> So this should be classified as such.

OK, I have made a clear note in the commit log mentioning that it is
not an actual bug.

Siddhesh
Adhemerval Zanella Dec. 16, 2014, 12:27 p.m. UTC | #5 
On 16-12-2014 09:26, Siddhesh Poyarekar wrote:
> On Tue, Dec 16, 2014 at 11:57:24AM +0100, Andreas Schwab wrote:
>> Siddhesh Poyarekar <siddhesh@redhat.com> writes:
>>
>>> So this does not actually fix a bug, only the warning.
>> So this should be classified as such.
> OK, I have made a clear note in the commit log mentioning that it is
> not an actual bug.
>
> Siddhesh
I think the idea for such cases is to use DIAG_IGNORE_NEEDS_COMMENT
along with a comment why this is a false positive.
Siddhesh Poyarekar Dec. 16, 2014, 12:52 p.m. UTC | #6 
On Tue, Dec 16, 2014 at 10:27:58AM -0200, Adhemerval Zanella wrote:
> I think the idea for such cases is to use DIAG_IGNORE_NEEDS_COMMENT
> along with a comment why this is a false positive.

But why bother with such ugliness if it can be handled with a single
character change?

Siddhesh
Adhemerval Zanella Dec. 16, 2014, 12:59 p.m. UTC | #7 
On 16-12-2014 10:52, Siddhesh Poyarekar wrote:
> On Tue, Dec 16, 2014 at 10:27:58AM -0200, Adhemerval Zanella wrote:
>> I think the idea for such cases is to use DIAG_IGNORE_NEEDS_COMMENT
>> along with a comment why this is a false positive.
> But why bother with such ugliness if it can be handled with a single
> character change?
>
> Siddhesh

My understanding is to not shadow possible compiler issues with unrequired
code.
Siddhesh Poyarekar Dec. 16, 2014, 1:05 p.m. UTC | #8 
On Tue, Dec 16, 2014 at 10:59:10AM -0200, Adhemerval Zanella wrote:
> My understanding is to not shadow possible compiler issues with unrequired
> code.

I don't think this is a compiler issue since I don't think the
compiler will ever be able to evaluate that the range for the nscounts
will be limited to MAXNS.  In fact, given the wide usage of nscount
within the code, a bug could technically send the nscounts beyond
MAXNS.

Siddhesh
Adhemerval Zanella Dec. 16, 2014, 1:12 p.m. UTC | #9 
On 16-12-2014 11:05, Siddhesh Poyarekar wrote:
> On Tue, Dec 16, 2014 at 10:59:10AM -0200, Adhemerval Zanella wrote:
>> My understanding is to not shadow possible compiler issues with unrequired
>> code.
> I don't think this is a compiler issue since I don't think the
> compiler will ever be able to evaluate that the range for the nscounts
> will be limited to MAXNS.  In fact, given the wide usage of nscount
> within the code, a bug could technically send the nscounts beyond
> MAXNS.
>
> Siddhesh

 426                 if (statp->nscount > EXT(statp).nscount)
 427                         for (n = EXT(statp).nscount, ns = 0;
 428                              n < statp->nscount; n++) {
 429                                 while (ns < MAXNS
 430                                        && EXT(statp).nsmap[ns] != MAXNS)
 431                                         ns++;
 432                                 if (ns >= MAXNS)
 433                                         break;
 434                                 EXT(statp).nsmap[ns] = n;
 435                                 map[n] = ns++;
 436                         }

In this loop 'ns' is initialized to '0' and updated on a simple while with
2 constraints.  Someone with more compiler background could correct me, but
I don't think this is really hard to compile evaluate that will fall
in 0 <= ns < MAXNS in all cases.
Andreas Schwab Dec. 16, 2014, 1:19 p.m. UTC | #10 
IMHO you should report this as a compiler bug.

Andreas.
Ondřej Bílka Dec. 16, 2014, 10:08 p.m. UTC | #11 
On Tue, Dec 16, 2014 at 11:12:24AM -0200, Adhemerval Zanella wrote:
> On 16-12-2014 11:05, Siddhesh Poyarekar wrote:
> > On Tue, Dec 16, 2014 at 10:59:10AM -0200, Adhemerval Zanella wrote:
> >> My understanding is to not shadow possible compiler issues with unrequired
> >> code.
> > I don't think this is a compiler issue since I don't think the
> > compiler will ever be able to evaluate that the range for the nscounts
> > will be limited to MAXNS.  In fact, given the wide usage of nscount
> > within the code, a bug could technically send the nscounts beyond
> > MAXNS.
> >
> > Siddhesh
> 
>  426                 if (statp->nscount > EXT(statp).nscount)
>  427                         for (n = EXT(statp).nscount, ns = 0;
>  428                              n < statp->nscount; n++) {
>  429                                 while (ns < MAXNS
>  430                                        && EXT(statp).nsmap[ns] != MAXNS)
>  431                                         ns++;
>  432                                 if (ns >= MAXNS)
>  433                                         break;
>  434                                 EXT(statp).nsmap[ns] = n;
>  435                                 map[n] = ns++;
>  436                         }
> 
> In this loop 'ns' is initialized to '0' and updated on a simple while with
> 2 constraints.  Someone with more compiler background could correct me, but
> I don't think this is really hard to compile evaluate that will fall
> in 0 <= ns < MAXNS in all cases.

Something is fishy here as compile should detect that in range
propagation pass.

If you are not sure you could always check if it optimizes simpler code.
And my gcc-4.9.1-2 indeed simplifies this to zero, Siddhesh could you
check it too?

int foo (int x)
{
  int i;
  for (i=0; i < 1000000; i++)
    { 
      if (i > 1000000)
        return 1;
    }        
  return 0;
}
diff mbox

Patch

diff --git a/resolv/res_send.c b/resolv/res_send.c
index 4a95eb8..5a9882c 100644
--- a/resolv/res_send.c
+++ b/resolv/res_send.c
@@ -429,7 +429,7 @@  __libc_res_nsend(res_state statp, const u_char *buf, int buflen,
 				while (ns < MAXNS
 				       && EXT(statp).nsmap[ns] != MAXNS)
 					ns++;
-				if (ns == MAXNS)
+				if (ns >= MAXNS)
 					break;
 				EXT(statp).nsmap[ns] = n;
 				map[n] = ns++;