nss_dns: Check for proper A/AAAA address alignment

Message ID 87imuas1os.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series
  • nss_dns: Check for proper A/AAAA address alignment
Related show

Commit Message

Florian Weimer May 16, 2019, 6:18 p.m.
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.

Comments

Carlos O'Donell May 24, 2019, 4:54 p.m. | #1
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;

> 



-- 
Cheers,
Carlos.
Florian Weimer May 24, 2019, 7:27 p.m. | #2
* 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;
Carlos O'Donell May 24, 2019, 7:42 p.m. | #3
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;

> 



-- 
Cheers,
Carlos.

Patch

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;