ipc_core, ipc_session, ipc_shm_arena_lend: Native_socket_stream::remote_peer_process_credentials() can be misleading when from socketpair(). / SHM-jemalloc resulting bug/possible instabilities.

[ygoldfeld]

This has been a long-standing bug, but we only just noticed it recently due to some internal tests failing in a component depending on Flow-IPC. This was starting a couple SHM-jemalloc sessions in a rapid succession, and as a result a certain new assert() would trip; upon investigating the reason I was led to the following.

Normally I explain stuff top-down, from what goes wrong then get down to why, but in this case it seems easier to do the reverse. Anyway once investigated it's straightforward:

getsockopt(F) in Linux, with SO_PEERCRED, gets stuff including PID, UID, GID - which we use in various key ways in various places across Flow-IPC - of the process pertaining to the other side of the connection w/r/t FD F. The confusion stemmed from the exact meaning of this which we didn't contemplate fully in a certain context leading to the bug.

The exact definition, per man page, is that the values are internally saved by the kernal at the time of when the connection was created, namely: connect(), listen(), socketpair(). (The listen() one is not totally clear per se; does it mean accept()? or perhaps at the time that the earlier listen() call caused a connection to be registered in the background ahead of accept()? In any case, what matters is on the accept()/server side the "right thing" is what it saves.) In our case:

• This works great for a N_s_s::sync_connect() guy and for a N_s_s_acceptor::async_accept() guy, as those use connect() and listen().
  • Typically the only time this is used by a user (in any case in the mainstream tests; technically it is perfectly allowed for the user to do otherwise) is that ipc::session uses it to establish (internally) the session master channel.
• The other connection technique -- and it is preferred and usually used for all other cases -- is to use socketpair() to generate two pre-connected FDs; then transmit one (over the session master channel in fact) to the opposing process; now the two processes can speak. This is how N_s_s channels are established by ipc::session.
  • The getsockopt() technique works great for this, too, if you're the guy receiving the FD (so the opposing process; in ipc::session convention that is the session-client).
  • The bug is: the technique fails for the guy sending the FD: The FD that "stays" will return the PID of that same process itself, since at socketpair() time that was accurate.
    • The FD is then transmitted to another process, but that value stays. Note that this isn't even "wrong"; as transmitting an FD over a socket-stream does not destroy (necessarily) the FD being "transmitted"; so now the "opposing" process is really two processes - why should the answer change to the 2nd one? So the man page is accurate, and that behavior is subjectively reasonable.

Is this a bug in Flow-IPC? As described so far, other than arguably incomplete documentation (this caveat is not mentioned), technically not. Like the SO_PEERCRED behavior, the Native_socket_stream behavior could be described as reasonable.

(That said, an enhancement would be to compensate for this somehow in practice. More on that below.)

Unfortunately there are spots in Flow-IPC that internally rely on this feature acting differently. The one causing the unspecified assert-trip mentioned above is in SHM-jemalloc, namely shm_session.cpp:

  m_remote_process_id(shm_channel.owned_channel().remote_peer_process_credentials().process_id()),

m_remote_process_id is used to store internal tracking of SHM-stored objects; on the session-client side m_remote_process_id would always be that process's own PID, not the opposing guy's, and moreover simultaneous sessions to 2+ processes would start colliding in these data structures (hence the unspecified assert). In any case it's clearly chaotically wrong, though it has not caused too much trouble in practice yet.

How to fix

There are various approaches, but I suggest this at the moment:

• Firstly, have the N_s_s impl perform the getsockopt(F, SO_PEERCRED) at the start of PEER state, perhaps in the PEER-state ctor. (It cannot really fail at that point, so if error handling is annoying can just assert/abort on failure.) Save the resulting Process_credentials object (it's little BTW, ~3 ints). Document that the only possible error for remote_peer_process_credentials() is connection-earlier-hosed; it cannot fail unless called on a closed endpoint.
  • This is not important; just setup to simplify some stuff.
• Add remote_peer_process_credentials() mutator that takes a Process_credentials and simply overwrites the saved one with the given one.
• Document the accessor, explaining the nuance of what the normal return value actually means, and that it can be overridden manually, if you prefer; and also document the result of the following change:
• In ipc::session, when opening a Channel with a N_s_s pipe, use the remote_peer_process_credentials() new mutator to set what the accessor returns to X.remote_peer_process_credentials(), where X is the session master channel; this being only necessary on the session-server side for the N_s_s endpoint on the session-server side itself.
  • Repeat the same for the SHM-jemalloc internal-use IPC channel.
    • That'll fix the SHM-jemalloc assert problem: shm_channel.owne_channel() is the internal-use SHM-jemalloc IPC channel.

Priority / scheduling

The SHM-jemalloc effect is pretty naughty; needs fixing ASAP. The rest blocks it, so it needs fixing ASAP too.

I am in the middle of #143, so hopefully all of that and this fix too will go in pretty soon.