Fix startup crash from an out-of-range customdirectoryindex - #3931
Open
mcfnord wants to merge 1 commit into
Open
Fix startup crash from an out-of-range customdirectoryindex#3931mcfnord wants to merge 1 commit into
mcfnord wants to merge 1 commit into
Conversation
GetNumericIniSet's upper bound is inclusive and MAX_NUM_SERVER_ADDR_ITEMS is the element count of vstrDirectoryAddress, so the value one past the end was accepted from the settings file and then used as a subscript, crashing the client at startup in CConnectDlg::RequestServerList. Fixes jamulussoftware#3917.
softins
approved these changes
Aug 30, 2026
softins
left a comment
Member
There was a problem hiding this comment.
Looks ok to me. I usually prefer the upper bound of a range check to be exclusive rather than inclusive, but this change matches the convention established by all the other uses of GetNumericIniSet()
pljones
approved these changes
Aug 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI: Fixes #3917, which @pljones asked for a PR on. The client accepts a
customdirectoryindexofMAX_NUM_SERVER_ADDR_ITEMSout of the settings file and then uses it as a subscript into a vector holding exactly that many elements, so the one value past the end crashes the client at startup. The range check is one wider than the object it guards.Short description of changes
One line in
CClientSettings::ReadSettingsFromXML: the upper bound becomesMAX_NUM_SERVER_ADDR_ITEMS - 1.GetNumericIniSet's bound is inclusive,MAX_NUM_SERVER_ADDR_ITEMSis 12 andvstrDirectoryAddressholds 12 elements, so 12 was accepted and 12 is out of range. The two unguarded subscripts are inCConnectDlg::RequestServerListand thejamulusclient/getCurrentDirectoryRPC method, so it is not GUI-only.The client's own writer only ever emits
0..11, so reaching this needs a hand-edited or corrupted settings file. The range check exists to make that safe.Both arms, built from the same tree with and without this one line, Qt 5.15.13, gcc 13.3,
QT_QPA_PLATFORM=offscreen, settings file carryingdirectorytype7 (AT_CUSTOM) andwinviscon1:customdirectoryindexin the filemainThe fallback is visible in the settings file the client writes back on exit:
0where 12 was rejected,11where 11 was accepted.CHANGELOG: Client: Fixed a startup crash caused by an out-of-range custom directory index in the settings file.
Context: Fixes an issue?
Fixes: #3917
Does this change need documentation? What needs to be documented and how?
No.
Status of this Pull Request
Ready for review.
What is missing until this pull request can be merged?
Review.
Checklist
🤖 This message was written by AI and reviewed by @mcfnord.