Skip to content

Reject a mismatched scroll instead of exiting in sub_6FC49AE0 - #239

Open
devtejasx wants to merge 1 commit into
ThePhrozenKeep:masterfrom
devtejasx:fix/tome-scroll-type-check-223
Open

devtejasx wants to merge 1 commit into
ThePhrozenKeep:masterfrom
devtejasx:fix/tome-scroll-type-check-223

Conversation

@devtejasx

Copy link
Copy Markdown
Contributor

Fixes #223.

Packet 0x29 (put a scroll into a book) takes both item GUIDs from the client. The handler checks that one is a scroll and the other is a stored book, then sub_6FC49AE0 compares their suffixes and, on a mismatch, calls FOG_DisplayAssert followed by exit(-1). So a client that asks to put a Town Portal scroll into a Tome of Identify brings the whole server down.

This treats the mismatch the same way as the other invalid-input cases in that function: set *a5 and return 0, which makes the packet handler return 3. The pickup path isn't affected, because INVENTORY_FindFillableBook already only returns a book whose suffix matches the scroll.

The original assert + exit is kept behind NO_BUG_FIX, as agreed in #14.

I can't build D2MOO locally (32-bit MSVC toolchain), so CI is the real check here.

Packet 0x29 (scroll to book) carries both GUIDs straight from the
client. The handler checks that the scroll is a scroll and the book is
a stored book, then sub_6FC49AE0 compares their suffixes and, on a
mismatch, calls FOG_DisplayAssert and exit(-1). A client asking to put
a Town Portal scroll into a Tome of Identify therefore shuts down the
whole game server.

Treat the mismatch like the other invalid-input cases in the function:
set *a5 and return 0, which makes the packet handler return 3. The
pickup path is unaffected because INVENTORY_FindFillableBook already
only returns a book whose suffix matches the scroll.

The original assert and exit stay behind NO_BUG_FIX, as agreed in ThePhrozenKeep#14.

Fixes ThePhrozenKeep#223
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.

Potentially abusable crash

1 participant