Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redo wire protocol race condition fix. #2164

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Redo wire protocol race condition fix. #2164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions llarp/iwp/session.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ namespace llarp
void
Session::EncryptAndSend(ILinkSession::Packet_t data)
{
if (m_State == State::Closed)
return;
m_EncryptNext.emplace_back(std::move(data));
TriggerPump();
if (!IsEstablished())
Expand Down Expand Up @@ -176,12 +178,9 @@ namespace llarp
return;
auto close_msg = CreatePacket(Command::eCLOS, 0, 16, 16);
m_Parent->UnmapAddr(m_RemoteAddr);
m_State = State::Closed;
if (m_SentClosed.test_and_set())
return;
EncryptAndSend(std::move(close_msg));

LogInfo(m_Parent->PrintableName(), " closing connection to ", m_RemoteAddr);
m_State = State::Closed;
}

bool
Expand Down Expand Up @@ -352,7 +351,7 @@ namespace llarp
bool
Session::TimedOut(llarp_time_t now) const
{
if (m_State == State::Ready)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line here is the most important part of the change.

State::LinkIntro means we are finalizing the protocol handshake, we are in a kind of half established state were we are an encrypted but unauthenticated session. this is where we are sending the other end a "link intro" which identifies who we are (by snode address), including the ephemeral pubkey that we were challenged with for establishing the encryption. it's stupid af and i hate it.

State::Ready means we are authenticated and ready to exchange frames for inter node communication.

the state we hit here in this regression that one side thinks the other side is timed out when they really aren't, and are trying to finish the handshake. they can still send/recv but they just have no idea who it is they are talking to in terms of their .snode addr but the other end disagrees about where we are in that process and it gets stuck. this is because snode and client timeouts are different (because of course they are). someone wanted to add that requirement onto the pile and so it was added, even though it was something i did object to for complexity reasons.
as usual i gave up fighting on that because people have strong opinions here and i just want stuff to get done, rocking the boat is a waste of time.

if (m_State == State::Ready || m_State == State::LinkIntro)
{
return now > m_LastRX
&& now - m_LastRX
Expand Down
1 change: 0 additions & 1 deletion llarp/iwp/session.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,6 @@ namespace llarp

std::atomic_flag m_PlaintextEmpty;
llarp::thread::Queue<CryptoQueue_t> m_PlaintextRecv;
std::atomic_flag m_SentClosed;

void
EncryptWorker(CryptoQueue_t msgs);
Expand Down
4 changes: 2 additions & 2 deletions llarp/router/router.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,8 +482,8 @@ namespace llarp
LogError("RC is invalid, not saving");
return false;
}
if (m_isServiceNode)
_nodedb->Put(_rc);
if (IsServiceNode())
_nodedb->Put(rc());
QueueDiskIO([&]() { HandleSaveRC(); });
return true;
}
Expand Down