The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32109.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

cc @darosior, @marcofleon

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/39169547262}

9c3298eb04...52b65186e1: fix CI (lint and win64 build)

Concept ACK

if it indicates that the result is less than sizeof(sockaddr_in6) and sets sa_family equal to AF_INET6 in the output.

FWIW, i've tried to make our code robust to this case by adding the length checks in ab1d3ec.

But it's a very unlikely OS bug and this change will likely result in more useful test coverage.

Leaning toward Concept NACK.

OP is suggesting to provide more guarantees in our mocked getsockname than the one used in production. On its face i think the motivation is misguided and we should rather aim for having stricter tests to make sure our code is robust as well as to surface some edge case we might have not considered. If the reason for opening this PR is that the change in #31676 led to some fuzz crashes, then i would say it works as intended. Instead of working around the crash in the fuzz utility, the crashing code should be made more robust.

In addition i think that trying to give the guarantees that OP suggests will lead to unnecessarily convoluted code in the fuzz utility.

It would be a bug in getsockname(2) if it returns a result that is smaller than the returned socket address family.

Maybe. But it also does not give any such guarantee, so code assuming this is brittle.

In other words, the name->sa_family in the output should be consistent with the returned *name_len.

Much better this way, but not enough - it could still return e.g. IPv6 address that is smaller than the expected size for an IPv6 address, possibly confusing callers to read data past the end of the buffer.

(Second quote is from #31676 (comment))

Code that, when reading the socket addr buffer, disregards said buffer size and blindly relies on the socket family type is brittle and should be fixed.

Code that, when reading the socket addr buffer, disregards said buffer size and blindly relies on the socket family type is brittle and should be fixed.

Yes. Though to be fair, it's something hard to get right due to lack of guarantees with regard to the socket name structure on different platforms. For example, even reading .sa_family could ostensibly reach outside the buffer. It's common that these are the first two bytes, but not guaranteed (for example macOS has a .sa_len field i don't remember if before or after). So to be 100% sure you'd have to check first against min(sizeof(sockaddr_in), sizeof(sockaddr_in6), sizeof(sockaddr_un), ...) (depending on the families you're going to accept. Of course sockaddr_in is likely to be smallest but who really knows...).

How much effort to spend on this depends on how far you want to go down this rabbit hole of "the OS could give us malicious data".

It is ok to assume the OS functions conform to their specifications and our production code need not handle non-conforming behavior from OS functions. For example, fopen(3) says "on error NULL is returned and the global variable errno is set to indicate the error". So, our production code need not handle cases where NULL is returned but errno is not set. Or, another example, if memcpy() is used, we don't check that it actually copied the bytes with memcmp() afterwards, we just assume the bytes were copied.

Do you agree?

From here it follows that the mocked/fuzzed OS functions must be conforming as well. E.g. if we mock fopen() and return NULL, then our mocked function should bother to set errno, otherwise it is non-conforming.

Right?

Now, I guess, the question here is whether getsockname() returning inconsistent name and namelen is conforming or not. For example this:

int socket = ...;
sockaddr_in name_in;
socklen_t len = sizeof(name_in);
sockaddr* name = (sockaddr*)&name_in;

if (getsockname(socket, name, &len) == 0) {
    if (name.sa_family == AF_INET && len == 3) {
        // Is this getsockname() conforming? Or is that a bug in
        // getsockname() that our production code should not care about,
        // like memcpy() not copying the bytes, or fopen() not setting errno?
    }
}