Message ID | 87imuas1os.fsf@oldenburg2.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | nss_dns: Check for proper A/AAAA address alignment | expand |
On 5/16/19 1:18 PM, Florian Weimer wrote: > 2019-05-16 Florian Weimer <fweimer@redhat.com> > > * 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. > 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? e.g. 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. > > if (__glibc_unlikely (n > linebuflen)) > goto too_small; >
* Carlos O'Donell: > On 5/16/19 1:18 PM, Florian Weimer wrote: >> 2019-05-16 Florian Weimer <fweimer@redhat.com> >> >> * 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 <fweimer@redhat.com> * resolv/nss_dns/dns-host.c (getanswer_r): Be more explicit about struct in_addr/struct in6_addr alignment. 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 <stdlib.h> #include <stddef.h> #include <string.h> +#include <libc-pointer-arith.h> #include "nsswitch.h" #include <arpa/nameser.h> @@ -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;
On 5/24/19 2:27 PM, Florian Weimer wrote: > * Carlos O'Donell: > >> On 5/16/19 1:18 PM, Florian Weimer wrote: >>> 2019-05-16 Florian Weimer <fweimer@redhat.com> >>> >>> * 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. That makes perfect sense. > 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. Good point. >> 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. Also a good point. I assumed, but hadn't checked that this didn't have overflow checking. Yes, from a "pattern" perspective it's better to use some kind of buffer managment API like alloc_buffer. > Thanks, > Florian > > nss_dns: Check for proper A/AAAA address alignment > > 2019-05-24 Florian Weimer <fweimer@redhat.com> > > * resolv/nss_dns/dns-host.c (getanswer_r): Be more explicit about > struct in_addr/struct in6_addr alignment. > New version looks good to me. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > 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 <stdlib.h> > #include <stddef.h> > #include <string.h> > +#include <libc-pointer-arith.h> OK. > > #include "nsswitch.h" > #include <arpa/nameser.h> > @@ -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; > + } OK. > > if (__glibc_unlikely (n > linebuflen)) > goto too_small; >
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"); + linebuflen -= align - ((u_long) bp % align); + bp += align - ((u_long) bp % align); if (__glibc_unlikely (n > linebuflen)) goto too_small;