From patchwork Fri May 24 19:27:03 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 32853 Received: (qmail 43541 invoked by alias); 24 May 2019 19:27:10 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 43533 invoked by uid 89); 24 May 2019 19:27:10 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.9 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mx1.redhat.com From: Florian Weimer To: Carlos O'Donell Cc: libc-alpha@sourceware.org Subject: Re: [PATCH] nss_dns: Check for proper A/AAAA address alignment References: <87imuas1os.fsf@oldenburg2.str.redhat.com> <8ae2349f-4794-bce2-afe6-7b772957b4ef@redhat.com> Date: Fri, 24 May 2019 21:27:03 +0200 In-Reply-To: <8ae2349f-4794-bce2-afe6-7b772957b4ef@redhat.com> (Carlos O'Donell's message of "Fri, 24 May 2019 12:54:40 -0400") Message-ID: <87ef4nodq0.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 * Carlos O'Donell: > On 5/16/19 1:18 PM, Florian Weimer wrote: >> 2019-05-16 Florian Weimer >> >> * resolv/nss_dns/dns-host.c (getanswer_r): Be more explicit about >> struct in_addr/struct in6_addr alignment. >> > > Can we use standard macros to compute alignmnet and differences, it > makes the code more easy to read at a glance. I want to convert this to struct alloc_buffer eventually, then this will go away anyway. I'm trying to post smaller patches towards this goal. These changes are really hard to review unfortunately. As a stop-gap measure, I've changed the code to use PTR_ALIGN_UP. >> diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c >> index 9c15f25f28..9c40051aff 100644 >> --- a/resolv/nss_dns/dns-host.c >> +++ b/resolv/nss_dns/dns-host.c >> @@ -947,8 +947,15 @@ getanswer_r (struct resolv_context *ctx, >> linebuflen -= nn; >> } >> >> - linebuflen -= sizeof (align) - ((u_long) bp % sizeof (align)); >> - bp += sizeof (align) - ((u_long) bp % sizeof (align)); >> + /* Provide sufficient alignment for both address >> + families. */ >> + enum { align = 4 }; >> + _Static_assert ((align % __alignof__ (struct in_addr)) == 0, >> + "struct in_addr alignment"); >> + _Static_assert ((align % __alignof__ (struct in6_addr)) == 0, >> + "struct in6_addr alignment"); > > OK. > >> + linebuflen -= align - ((u_long) bp % align); >> + bp += align - ((u_long) bp % align); > > Is the use case common enough to add something to libc-pointer-arith.h > to handle linebuflen adjustment? Yes, see struct alloc_buffer. > align_diff = ALIGN_UP_DIFF (bp, align); > linebuflen -= align_diff; > bp += align_diff; > > If not then can we still use ALIGN_UP? It makes it immediately > obvious the intent was to align the pointer upwards and adjust > the length of the remaining buffer. This lacks overflow checking. It is not a good coding pattern in my opinion. Thanks, Florian nss_dns: Check for proper A/AAAA address alignment 2019-05-24 Florian Weimer * resolv/nss_dns/dns-host.c (getanswer_r): Be more explicit about struct in_addr/struct in6_addr alignment. Reviewed-by: Carlos O'Donell diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c index 9c15f25f28..5af47fd10d 100644 --- a/resolv/nss_dns/dns-host.c +++ b/resolv/nss_dns/dns-host.c @@ -78,6 +78,7 @@ #include #include #include +#include #include "nsswitch.h" #include @@ -947,8 +948,18 @@ getanswer_r (struct resolv_context *ctx, linebuflen -= nn; } - linebuflen -= sizeof (align) - ((u_long) bp % sizeof (align)); - bp += sizeof (align) - ((u_long) bp % sizeof (align)); + /* Provide sufficient alignment for both address + families. */ + enum { align = 4 }; + _Static_assert ((align % __alignof__ (struct in_addr)) == 0, + "struct in_addr alignment"); + _Static_assert ((align % __alignof__ (struct in6_addr)) == 0, + "struct in6_addr alignment"); + { + char *new_bp = PTR_ALIGN_UP (bp, align); + linebuflen -= new_bp - bp; + bp = new_bp; + } if (__glibc_unlikely (n > linebuflen)) goto too_small;