Segfault in getifaddrs_internal in glibc-2.20
Commit Message
Allan McRae <allan@archlinux.org> writes:
> diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c
> b/sysdeps/unix/sysv/linux/ifaddrs.c
> index 2c04e17..1fa4960 100644
> --- a/sysdeps/unix/sysv/linux/ifaddrs.c
> +++ b/sysdeps/unix/sysv/linux/ifaddrs.c
> @@ -774,7 +774,7 @@ getifaddrs_internal (struct ifaddrs **ifap)
> unsigned int preflen;
>
> if ((max_prefixlen > 0) &&
> - (ifam->ifa_prefixlen > max_prefixlen))
> + (max_prefixlen > ifam->ifa_prefixlen))
> preflen = max_prefixlen;
> else
> preflen = ifam->ifa_prefixlen;
>
>
> This seems to fix the issue for them. After looking at the code for a
> long time, I am still not sure if this is the correct fix. Can anyone
> confirm/deny?
That doesn't make any sense. As the name says, max_prefixlen is the
limit for the prefix length, but this makes it always equal to
max_prefixlen (which is always > 0 here). The bug is of course that
commit d89b3d8 doesn't handle a prefix length of zero.
Andreas.
[BZ #17371]
* sysdeps/unix/sysv/linux/ifaddrs.c (getifaddrs_internal): Fix
last change to handle zero prefix length.
---
sysdeps/unix/sysv/linux/ifaddrs.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
Comments
On 13/09/14 18:18, Andreas Schwab wrote:
> Allan McRae <allan@archlinux.org> writes:
>
>> diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c
>> b/sysdeps/unix/sysv/linux/ifaddrs.c
>> index 2c04e17..1fa4960 100644
>> --- a/sysdeps/unix/sysv/linux/ifaddrs.c
>> +++ b/sysdeps/unix/sysv/linux/ifaddrs.c
>> @@ -774,7 +774,7 @@ getifaddrs_internal (struct ifaddrs **ifap)
>> unsigned int preflen;
>>
>> if ((max_prefixlen > 0) &&
>> - (ifam->ifa_prefixlen > max_prefixlen))
>> + (max_prefixlen > ifam->ifa_prefixlen))
>> preflen = max_prefixlen;
>> else
>> preflen = ifam->ifa_prefixlen;
>>
>>
>> This seems to fix the issue for them. After looking at the code for a
>> long time, I am still not sure if this is the correct fix. Can anyone
>> confirm/deny?
>
> That doesn't make any sense. As the name says, max_prefixlen is the
> limit for the prefix length, but this makes it always equal to
> max_prefixlen (which is always > 0 here). The bug is of course that
> commit d89b3d8 doesn't handle a prefix length of zero.
>
Ah - that explains it! I could not figure out how the above patch
"fixed" the issue.
This fix looks good to me.
Allan
>
> [BZ #17371]
> * sysdeps/unix/sysv/linux/ifaddrs.c (getifaddrs_internal): Fix
> last change to handle zero prefix length.
> ---
> sysdeps/unix/sysv/linux/ifaddrs.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c
> index 2c04e17..a47b2ed 100644
> --- a/sysdeps/unix/sysv/linux/ifaddrs.c
> +++ b/sysdeps/unix/sysv/linux/ifaddrs.c
> @@ -770,20 +770,17 @@ getifaddrs_internal (struct ifaddrs **ifap)
>
> if (cp != NULL)
> {
> - char c;
> unsigned int preflen;
>
> - if ((max_prefixlen > 0) &&
> - (ifam->ifa_prefixlen > max_prefixlen))
> + if (ifam->ifa_prefixlen > max_prefixlen)
> preflen = max_prefixlen;
> else
> preflen = ifam->ifa_prefixlen;
>
> - for (i = 0; i < ((preflen - 1) / 8); i++)
> + for (i = 0; i < preflen / 8; i++)
> *cp++ = 0xff;
> - c = 0xff;
> - c <<= ((128 - preflen) % 8);
> - *cp = c;
> + if (preflen % 8)
> + *cp = 0xff << (8 - preflen % 8);
> }
> }
> }
>
@@ -770,20 +770,17 @@ getifaddrs_internal (struct ifaddrs **ifap)
if (cp != NULL)
{
- char c;
unsigned int preflen;
- if ((max_prefixlen > 0) &&
- (ifam->ifa_prefixlen > max_prefixlen))
+ if (ifam->ifa_prefixlen > max_prefixlen)
preflen = max_prefixlen;
else
preflen = ifam->ifa_prefixlen;
- for (i = 0; i < ((preflen - 1) / 8); i++)
+ for (i = 0; i < preflen / 8; i++)
*cp++ = 0xff;
- c = 0xff;
- c <<= ((128 - preflen) % 8);
- *cp = c;
+ if (preflen % 8)
+ *cp = 0xff << (8 - preflen % 8);
}
}
}