Skip to content

Commit

Permalink
ip2net : zero less significant bits in ip4/ip6 addresses, workaround …
Browse files Browse the repository at this point in the history
…GCC bug
  • Loading branch information
bol-van committed Sep 12, 2021
1 parent 69dab1a commit 9402cd2
Show file tree
Hide file tree
Showing 10 changed files with 21 additions and 3 deletions.
Binary file modified binaries/aarch64/ip2net
Binary file not shown.
Binary file modified binaries/arm/ip2net
Binary file not shown.
Binary file modified binaries/mac64/ip2net
Binary file not shown.
Binary file modified binaries/mips32r1-lsb/ip2net
Binary file not shown.
Binary file modified binaries/mips32r1-msb/ip2net
Binary file not shown.
Binary file modified binaries/mips64r2-msb/ip2net
Binary file not shown.
Binary file modified binaries/ppc/ip2net
Binary file not shown.
Binary file modified binaries/x86/ip2net
Binary file not shown.
Binary file modified binaries/x86_64/ip2net
Binary file not shown.
24 changes: 21 additions & 3 deletions ip2net/ip2net.c
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,16 @@ static void mask_from_bitcount6(uint32_t zct, struct in6_addr *a)
// result = a & b
static void ip6_and(const struct in6_addr *a, const struct in6_addr *b, struct in6_addr *result)
{
// POSSIBLE GCC COMPILER BUG . when using uint64_t and -O2/-O3 optimizations this function gets inlined with the wrong code and produces wrong results
#if defined(__GNUC__) && !defined(__llvm__) && !defined(__NO_INLINE__)
((uint32_t*)result->s6_addr)[0] = ((uint32_t*)a->s6_addr)[0] & ((uint32_t*)b->s6_addr)[0];
((uint32_t*)result->s6_addr)[1] = ((uint32_t*)a->s6_addr)[1] & ((uint32_t*)b->s6_addr)[1];
((uint32_t*)result->s6_addr)[2] = ((uint32_t*)a->s6_addr)[2] & ((uint32_t*)b->s6_addr)[2];
((uint32_t*)result->s6_addr)[3] = ((uint32_t*)a->s6_addr)[3] & ((uint32_t*)b->s6_addr)[3];
#else

This comment has been minimized.

Copy link
@jwakely

jwakely Jun 8, 2022

Contributor

This is not a GCC bug, this is an aliasing violation. You are accessing a uint8_t[16] object through a pointer to uint32_t* (or uint64_t*) which has undefined behaviour.

The portable solution would be:

uint64_t a_addr[2], b_addr[2];
memcpy(a_addr, a->s6_addr, 16);
memcpy(b_addr, b->s6_addr, 16);
a_addr[0] &= b_addr[0];
a_addr[1] &= b_addr[1];
memcpy(result->s6_addr, a_addr, 16);

This comment has been minimized.

Copy link
@bol-van

bol-van Jun 8, 2022

Author Owner

the whole idea of this code is speed. no copying. use of max operand width.
((uint64_t*)&a->s6_addr[0])[1]
is this correct ?

This comment has been minimized.

Copy link
@jwakely

jwakely Jun 8, 2022

Contributor

No, that's an aliasing violation, i.e. undefined behaviour.

My memcpy solution will not copy, the compiler is smart enough to optimize out those copies. Instead of dismissing the correct solution, you should try it. Even at -O1 it generates much smaller code than your current code for GCC using uint32_t and equivalent code to your original code using uint64_t. With no optimization it's a little larger than the uint64_t code, but still smaller than the uint32_t code.

Live demo: https://godbolt.org/z/Gbc9qbvhd

This comment has been minimized.

Copy link
@bol-van

bol-van Jun 8, 2022

Author Owner

ok. i read an article about aliasing. you are right. i've always thought c is portable asm.
i cant fix the code now. will fo it later. if it behaves wrong in your case pls fix it yourself

This comment has been minimized.

Copy link
@jwakely

jwakely Jun 8, 2022

Contributor

I submitted #113

((uint64_t*)result->s6_addr)[0] = ((uint64_t*)a->s6_addr)[0] & ((uint64_t*)b->s6_addr)[0];
((uint64_t*)result->s6_addr)[1] = ((uint64_t*)a->s6_addr)[1] & ((uint64_t*)b->s6_addr)[1];
#endif
}

static void rtrim(char *s)
Expand Down Expand Up @@ -259,7 +267,7 @@ int main(int argc, char **argv)

/*
for(uint32_t i=0;i<ipct;i++)
if (inet_ntop(AF_INET6,iplist+i,str,256))
if (inet_ntop(AF_INET6,iplist+i,str,sizeof(str)))
printf("%s\n",str);
printf("\n");
*/
Expand Down Expand Up @@ -294,7 +302,14 @@ int main(int argc, char **argv)
break;
}
}
if (!zct_best) ip_start = iplist[pos], pos_end = pos + 1; // network not found, use single ip
if (zct_best)
{
// network was found
mask_from_bitcount6(zct_best, &mask);
ip6_and(iplist + pos, &mask, &ip_start);
}
else
ip_start = iplist[pos], pos_end = pos + 1; // network not found, use single ip
inet_ntop(AF_INET6, &ip_start, str, sizeof(str));
printf(zct_best ? "%s/%u\n" : "%s\n", str, 128 - zct_best);

Expand Down Expand Up @@ -379,7 +394,10 @@ int main(int argc, char **argv)
break;
}
}
if (!zct_best) ip_start = iplist[pos], pos_end = pos + 1; // network not found, use single ip
if (zct_best)
ip_start = iplist[pos] & mask_from_bitcount(zct_best);
else
ip_start = iplist[pos], pos_end = pos + 1; // network not found, use single ip

u1 = ip_start >> 24;
u2 = (ip_start >> 16) & 0xFF;
Expand Down

0 comments on commit 9402cd2

Please sign in to comment.