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.

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

Fix memory leak when checking player list setting #261

Merged
merged 1 commit into from
Feb 10, 2025
Merged

Conversation

connorhsm
Copy link
Member

@connorhsm connorhsm commented Jan 5, 2025

While investigating for #258, I've used Valgrind to find a memory leak relating to the PLAYER_LIST message.

Valgrind output after server setup completed and then running for ~30 seconds:

==2966== 480 bytes in 80 blocks are definitely lost in loss record 266 of 542
==2966==    at 0x48485C3: operator new[](unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==2966==    by 0x151858: stringDuplicate(char const*) (stringUtils.h:100)
==2966==    by 0x1D6BC0: trimWhitespace(char*) (stringUtils.cpp:493)
==2966==    by 0x134579: main (server.cpp:13242)

More specifically, when we periodically reload the playerListSecret setting, playerListSecret would be overwritten before being deleted:

playerListSecret = trimmed;

While the main functionality was introduced in #207, this memory leak was introduced shortly after by #212.

To fix this, I have refactored to only read the setting when we receive a PLAYER_LIST message and free it soon after. I also removed a reference to the secret which was used to decide how long to wait until killing the socket, which I deemed unnecessary. This implementation is similar to how other secret settings are already fetched (close to when they are used).

OneLife/server/server.cpp

Lines 10812 to 10814 in 330d897

char *reflectorSharedSecret =
SettingsManager::
getStringSetting( "reflectorSharedSecret" );

I have compiled the server before and after my fix, and observed Valgrind output in both cases. With the refactor applied, no memory leaks are detected. I have also tested the continued functionality of the PLAYER_LIST message with the refactor applied, Dictator was able to continue communicating without any changes or issues.

Note the current production server is using ~4GB swap (detailed in #258), which I suspect is due to this issue. It's possible this is the sole cause of #258, but further monitoring will be required.

…t settings

Resovles memory leak in periodic udpate of setting
@connorhsm connorhsm requested a review from risvh January 5, 2025 14:19
@connorhsm
Copy link
Member Author

@Eboubaker May be of interest, your input is appreciated 😄

@Eboubaker
Copy link
Contributor

a classic memory leak, sorry for my oversight on this. You changed the code to always read the setting file playerListSecret when a request for playerList is made, this will make the password change effect immediately I think it is better that way.

Copy link
Contributor

@risvh risvh left a comment

Choose a reason for hiding this comment

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

Great find, thanks both.

@risvh risvh merged commit c02287e into master Feb 10, 2025
@risvh risvh deleted the update-player-list branch February 10, 2025 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants