nss_dns: Check for proper A/AAAA address alignment
Commit Message
* 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.
Comments
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;
>
@@ -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;