Andrei G uploaded patch set #3 to this change.

View Change

The following approvals got outdated and were removed: Verified+1 by Jenkins Builder

core/socket: sockaddr_cmp: compare fields

osmo_sockaddr_cmp() decides whether two addresses are equal with a
memcmp() over the whole struct sockaddr_in or sockaddr_in6. On Linux
those structures hold only family, port, address and padding, so the
comparison is exact. On Darwin and the BSDs the first byte is sin_len /
sin6_len. The kernel fills it in on recvfrom() and accept(), while an
address the application built from configuration leaves it zero. The
same peer then compares as two different addresses.

Every static NS-VC in gprs_ns2 breaks on this. The answer to the first
NS-RESET arrives from the configured remote, gprs_ns2_udp.c cannot match
it to the NS-VC ("Ignoring NS RESET ACK from newconnection for
non-existing NS-VC", gprs_ns2.c:1066) and the link never leaves RESET.
Observed with osmo-pcu against osmo-sgsn on Darwin loopback: the SGSN
side, which learned the peer from the packet, goes to BLOCKED and then
loses every NS-ALIVE-ACK, while the PCU side stays in RESET. Dynamic
NS-VCs are unaffected because their remote address is a copy of what
recvfrom() returned.

Compare the fields instead of the bytes: port and address for AF_INET,
port, flow info, address and scope id for AF_INET6, in the order
memcmp() visited them, so the ordering the function gives to sorted
users does not change. The default branch keeps its memcmp() over the
full osmo_sockaddr. Linux behaviour is unchanged.

Add tests/sockaddr_cmp covering the two cases that regressed and the
ordering the fix has to preserve: an IPv4 and an IPv6 pair equal in port
and address but differing in the length byte, both expected to compare
equal, and ordering by port, by address, by flow info and by scope id,
each checked in both directions. Against the previous implementation the
two length-byte cases return -16 and -28 instead of 0.

The length byte only exists where the sockaddr carries one, so
configure.ac gains an AC_CHECK_MEMBER for struct sockaddr_in.sin_len,
in the style of the existing s6_addr32 check. Where the member is
absent the two addresses are identical and the expectation is the same,
so the test and its .ok output are the same on every platform.

Note that sin6_flowinfo and sin6_scope_id are compared byte by byte,
which is not numeric order on a little endian host. That is unchanged
from the memcmp() this replaces. The test uses values differing in one
byte only, where both orders agree.

Change-Id: I75fc62a92f546a906dbb1440984d62f962579255
Signed-off-by: Andrei Gosman <andrei.gosman@gmail.com>
---
M configure.ac
M src/core/socket.c
M tests/Makefile.am
A tests/sockaddr_cmp/sockaddr_cmp_test.c
A tests/sockaddr_cmp/sockaddr_cmp_test.ok
M tests/testsuite.at
6 files changed, 201 insertions(+), 4 deletions(-)

git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/75/43575/3

To view, visit change 43575. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: newpatchset
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I75fc62a92f546a906dbb1440984d62f962579255
Gerrit-Change-Number: 43575
Gerrit-PatchSet: 3
Gerrit-Owner: Andrei G <andrei.gosman@gmail.com>
Gerrit-Reviewer: Jenkins Builder