Attention is currently required from: pespin.

andrei.gosman@gmail.com has posted comments on this change by andrei.gosman@gmail.com. ( https://gerrit.osmocom.org/c/libosmocore/+/43574?usp=email )

Change subject: core/socket: init_osa: pass family-correct len ......................................................................

Patch Set 1:

(1 comment)

Correct, AF_UNIX would fail here. Worth being precise about what this patch changes for that case: nothing.

Before the patch bind() got sizeof(struct osmo_sockaddr), 128. osmo_sockaddr_size() returns the same 128 for AF_UNIX, because the family falls through to the default branch. AF_UNIX is therefore exactly as broken after this patch as before it, and the patch is a strict fix for AF_INET and AF_INET6 only. No regression, but no improvement either.

The type does not stop it: the union has a sockaddr_storage member, 128 bytes, and sockaddr_un fits in it (106 bytes on Darwin, and well under 128 on glibc too). libosmocore relies on that in osmo_sock_get_name_buf(), which getsockname()s into an osmo_sockaddr and then reads the AF_UNIX case back through a struct sockaddr_un *.

Nor does anything in the function reject it. The AF_UNSPEC case in socket_test.c fails only because it is a BIND|CONNECT pair with mismatched families and the same-family check catches it. A BIND-only call with an unsupported family reaches bind() with namelen 128, unchecked.

In practice AF_UNIX callers use osmo_sock_unix_init(), which computes SUN_LEN() itself, and every in-tree osmo_sock_init_osa() caller passes IP. So this is latent rather than an active bug. It is an exported symbol though, so out-of-tree callers are not covered by that argument.

Two ways to close it, if you want it closed:

(a) osmo_sockaddr_size() default branch: LOGP an error and return 0 instead of sizeof(struct osmo_sockaddr). One place, and it covers bind, connect and the three osmo_io msg_namelen callers, where a bogus family currently makes sendmsg read 128 stray bytes instead of treating msg_name as unset. It does change documented behaviour: the docstring promises the size of struct osmo_sockaddr for an unsupported family.

(b) Early return -EINVAL in osmo_sock_init_osa() for anything other than AF_INET and AF_INET6. Narrower, no contract change.

I lean (a) for the wider effect, but (b) is the conservative one. Either way I would send it as a separate change on top of this one rather than fold it in, since it is a different bug from the namelen fix. Tell me which you prefer, or mark this resolved if you would rather leave it as is.

File src/core/socket.c:

https://gerrit.osmocom.org/c/libosmocore/+/43574/comment/a40f9606_a7fdb777?u... : PS1, Line 603: if (bind(sfd, &local->u.sa, osmo_sockaddr_size(local)) == -1) {

AFAIU this will still fail for you when using a UNIX socket here?

Context: this patch is extracted from a port of the Osmocom stack to native macOS ARM64.

On AF_UNIX: struct osmo_sockaddr's union does not include sockaddr_un today (only sockaddr_in, sockaddr_in6, sockaddr_storage). UNIX sockets have their own path via osmo_sock_unix_init(), which uses SUN_LEN() and does not touch osmo_sockaddr. So AF_UNIX cannot reach this code path even before the patch. Behaviour for hypothetical AF_UNIX input is unchanged: previously 128 bytes (sizeof struct osmo_sockaddr), now 128 bytes (sizeof struct sockaddr_storage via the default branch in osmo_sockaddr_size). Both wrong on Darwin, both irrelevant given the union restriction. If we ever add sockaddr_un to the union, a separate AF_UNIX case in osmo_sockaddr_size() returning SUN_LEN(sun) would be needed.