Skip to content

Conversation

@practicalswift
Copy link
Contributor

Avoid use of low file descriptor ids (which may be in use) in FuzzedSock.

Context: https://github.com/bitcoin/bitcoin/pull/21630/files#r610694541

@DrahtBot DrahtBot added the Tests label Apr 14, 2021
@laanwj
Copy link
Member

laanwj commented Apr 14, 2021

Concept ACK but this seems a bit… arbitrary. If you want to be 100% certain to not step on an existing one, could you open a (real) file descriptor, connected to something like a pipe() or /dev/null etc, and use that? Or do you use the fact that it needs to be a non-allocated file descriptor somewhere?

@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@vasild
Copy link
Contributor

vasild commented Apr 14, 2021

... could you open a (real) file descriptor, connected to ...

Right! It does not need to be connected to anything. Just reuse this:

static SOCKET CreateSocket()
{
const SOCKET s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
BOOST_REQUIRE(s != static_cast<SOCKET>(SOCKET_ERROR));
return s;
}

plus a close() in the FuzzedSocket destructor.

@practicalswift
Copy link
Contributor Author

practicalswift commented Apr 14, 2021

Concept ACK but this seems a bit… arbitrary. If you want to be 100% certain to not step on an existing one, could you open a (real) file descriptor, connected to something like a pipe() or /dev/null etc, and use that? Or do you use the fact that it needs to be a non-allocated file descriptor somewhere?

The nice thing about going with INVALID_SOCKET - 1 is that it doesn't involve slowdowns due to syscalls, and that reading or writing from fd INVALID_SOCKET - 1 is guaranteed to fail (unless the fuzzing harness has four billion open file descriptors :)).

Note that INVALID_SOCKET - 1 is 4 294 967 294, and that POSIX requires file descriptor id re-use on close.

Note that this PR is only a short-term bug fix to avoid file descriptors stdin (0), stdout (1), stderr (2) and other likely to be used file descriptors while awaiting vasild's suggested cleanup/refactoring.

@practicalswift practicalswift force-pushed the avoid-open-fds-when-fuzzing branch from 6464d10 to 6262182 Compare April 14, 2021 22:23
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6262182

@maflcko maflcko merged commit 9712f75 into bitcoin:master Apr 15, 2021
@vasild
Copy link
Contributor

vasild commented Apr 15, 2021

The chosen constant INVALID_SOCKET - 1 is bigger than FD_SETSIZE, so those dummy sockets are not selectable:

bitcoin/src/compat.h

Lines 100 to 106 in 2cd834e

bool static inline IsSelectableSocket(const SOCKET& s) {
#if defined(USE_POLL) || defined(WIN32)
return true;
#else
return (s < FD_SETSIZE);
#endif
}

I think that's ok. I am working on a change to convert IsSelectableSocket(bare SOCKET or sock.Get()) to sock.IsSelectable() so that the mocked sockets could override that.

@vasild
Copy link
Contributor

vasild commented Apr 15, 2021

#21700 reduces the usage of Sock::Get() to tests and CreateSockTCP() which is not to be mocked. It also changes IsSelectableSocket() to be a method of Sock() so that FuzzedSock can override it nicely.

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants