Repository navigation
fix: bugs in message and netadr natives - #390
Open
Nord1cWarr1or wants to merge 1 commit into
Open
Nord1cWarr1or wants to merge 1 commit into
Nord1cWarr1or wants to merge 1 commit into
Conversation
…am bounds abort - GetMessageOrigData(MsgMsgId) returned the current message id (getId()); use getOriginalId() like the other Original-branches do (rehlds#387) - set_netadr/get_netadr wrote/read 8 bytes into the 4-byte netadr_s.ip field through a size_t cast - OOB on 64-bit builds; use unsigned int, 32-bit behavior unchanged (rehlds#388) - CHECK_PARAMBOUNDS now aborts the native (return FALSE) after logging instead of continuing with the out-of-bounds argument; all six use-sites reviewed, every caller already has FALSE error paths (rehlds#389)
4 tasks done
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.
🖤
Purpose
Three bugs in the message-hook and netaddr natives, one per issue:
GetMessageOrigDatawithMsgMsgIdreturned the current message id (getId()) instead of the original one, breaking the native's contract. Every other*Orig*branch in the file already used the original getters, andgetOriginalId()has been part of theIMessageContextinterface (IMessageManager.h) all along. Resolves GetMessageOrigData(MsgMsgId) returns the current message ID instead of the original one #387.set_netadr/get_netadrcastnetadr_s.ip(anunsigned char ip[4]field) tosize_t *and wrote/read through it. On LP64size_tis 8 bytes, so 8 bytes went into a 4-byte field, clobbering the adjacentportand the head ofipx[10]— out-of-bounds access on 64-bit builds. Resolves Out-of-bounds write/read of netadr_s.ip[4] via size_t cast in set_netadr/get_netadr (64-bit builds) #388.CHECK_PARAMBOUNDSlogged an error but did not return, so the native kept executing with the out-of-bounds argument.Approach
GetMessageOrigDataIndex case:getId()→getOriginalId().size_t *cast becomesunsigned int *— 4 bytes on both ILP32 and LP64, so the OOB is gone and 32-bit behavior/codegen is unchanged (sizeof(size_t) == sizeof(unsigned int) == 4there).CHECK_PARAMBOUNDSgainsreturn FALSE;. All six use sites audited (SetMessageData,GetMessageData,GetMessageOrigData,GetMessageArgType,IsMessageDataModified,ResetModifiedMessageData): each is a cell-returning native with an existingFALSEerror path, so the abort follows the file's own convention.Open Questions and Pre-Merge TODOs
reapi_amxx_i386.so); the two changed translation units compile with no warnings.-m64) syntax-only compile of the changed files — clean.netadr_slayout (common/netadr.h):type(4 bytes) is followed byip[4], soipis 4-byte aligned and a 4-byte access covers exactly the field.Learning
netadr_slayout from ReHLDScommon/netadr.h; original-id getters fromIMessageManager.h.Resolves: #387, #388, #389