Skip to content

fix(network): Fix missing value initialization of LANGameInfo::m_isDirectConnect #1836

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

Merged
xezon merged 2 commits into from
Dec 13, 2025
Merged

fix(network): Fix missing value initialization of LANGameInfo::m_isDirectConnect #1836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c3ca233
Set isDirectConnect field when announcing game in lobby response
bobtista Nov 11, 2025
47862f7
Initialize isDirectConnect member variable in LANGameInfo constructor
bobtista Dec 3, 2025
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
1 change: 1 addition & 0 deletions Core/GameEngine/Source/GameNetwork/LANAPIhandlers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ void LANAPI::handleRequestLocations( LANMessage *msg, UnsignedInt senderIP )
strlcpy(reply.GameInfo.options, gameOpts.str(), ARRAY_SIZE(reply.GameInfo.options));
wcslcpy(reply.GameInfo.gameName, m_currentGame->getName().str(), ARRAY_SIZE(reply.GameInfo.gameName));
reply.GameInfo.inProgress = m_currentGame->isGameInProgress();
reply.GameInfo.isDirectConnect = m_currentGame->getIsDirectConnect();

@xezon xezon Nov 12, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What issue did this cause then? Was Direct Connect game visible in Network Lobby when it should not have been?

@bobtista bobtista Nov 12, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I don't have my PC to check this, I was just following the notes in the Issue. handleRequestLocations handler wasn't setting reply.GameInfo.isDirectConnect when building the game announcement message, so could we end up sending uninitialized data in that specific message struct? Does LANMessage reply also get auto zeroed?

@xezon xezon Nov 13, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You need to test the consequence of this bug so we can properly describe what this change fixes.

@Caball009 Caball009 Dec 5, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What issue did this cause then?

See other comments on this PR.


sendMessage(&reply);
}
Expand Down
2 changes: 2 additions & 0 deletions Core/GameEngine/Source/GameNetwork/LANGameInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ LANGameInfo::LANGameInfo()
{
m_lastHeard = 0;
m_next = NULL;
m_isDirectConnect = false;

@xezon xezon Nov 12, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Are you certain this caused uninitialized memory use on runtime? LANGameInfo is allocated through Game Memory overloads, which means its memory is memset 0 before the constructor is called. So if you look at this object in debugger, then this field should 0 regardless.

@bobtista bobtista Nov 12, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

No, I think you're right here. Is it still good practice to do it explicitly here? If not, I can just drop that commit.

@xezon xezon Nov 13, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This happens all over the code base. Class members are not explicitly zero initialized, which is a mistake in principle, but the memory allocator is forgiving (and very pessimistic) by zeroing every allocation.

@Caball009 Caball009 Dec 5, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Are you certain this caused uninitialized memory use on runtime?

No, I don't think this changed anything, but it's always a good idea to initialize variables. The meaningful change is setting reply.GameInfo.isDirectConnect.

//
for (Int i = 0; i< MAX_SLOTS; ++i)
setSlotPointer(i, &m_LANSlot[i]);

Expand Down
Loading