Message ID | 20141216100950.GM30928@spoyarek.pnq.redhat.com |
---|---|
State | Committed |
Headers |
Received: (qmail 14389 invoked by alias); 16 Dec 2014 10:09:57 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 14379 invoked by uid 89); 16 Dec 2014 10:09:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Date: Tue, 16 Dec 2014 15:39:50 +0530 From: Siddhesh Poyarekar <siddhesh@redhat.com> To: libc-alpha@sourceware.org Subject: [PATCH] Fix 'array subscript is above array bounds' warning in res_send.c Message-ID: <20141216100950.GM30928@spoyarek.pnq.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="00sTfE/IIAT5d2r5" Content-Disposition: inline User-Agent: Mutt/1.5.22.1-rc1 (2013-10-16) |
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
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.
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
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.
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
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.
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
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.
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
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.
IMHO you should report this as a compiler bug. Andreas.
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 --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++;