Fix invalid memory access when connecting to Online Lobby #348
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.
Fixes #91: Memory corruption on connecting to Online Lobby
This should remain a draft PR until this has been tested.
Analysis:
bind_ptr->name.idsis currently being assigned to some stack memory, instead of the copy that's allocated viaSnmpUtilMemAlloc. That's probably the main cause of the originally reported issueSnmpExtensionQueryis documented as potentially allocating/reallocating/freeing memory on the VarBindList. This is validated by the fact that the reported crash happens in that function, when it attempts to free the aforementioned stack memory. So for safety, we should probably assume it invalidates any pointers.SnmpUtilVarBindListFreehandles cleaning up members of theSnmpVarBindList. Based on example usage and peeking at the ReactOS implementation, it seems like the actualSnmpVarBindListis not free'd by the function, because it's not necessarily expected to be allocated viaSnmpUtilMemAlloc. So there's an opportunity to clean this up to not require callingSnmpUtilMemFreedirectly at all.mib_ii_name_ptrdirectly toname.idswithout cleaning up some of these other issues, we risk a double-free and small memory leakChanges:
bind_ptr->name.idsassignment to actually use theSnmpUtilMemAllocallocated memory, rather than the static array sitting in stack, to satisfy requirements indicated by MSDN documentationSnmpUtilVarBindListFreeand be done.Testing:
This can probably be merged after some basic testing.