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

Resolve crash on inventory pickup #647

Merged
merged 1 commit into from
Nov 25, 2024
Merged

Conversation

jengelh
Copy link
Contributor

@jengelh jengelh commented Nov 4, 2024

Commit d185ab9 broke the pointer-moving logic. When the allweapons cheat is executed or when e.g. the afterburner is picked up, ASAN terminates the program with:

==8330==ERROR: AddressSanitizer: heap-use-after-free on address 0x50f00007ab60
at pc 0x7f23334f6843 bp 0x7ffe724d2b10 sp 0x7ffe724d22d0
READ of size 3 at 0x50f00007ab60 thread T0
    f0 strdup
    f1 Inventory::AddCounterMeasure(int, int, int, int, char const*) Descent3/Inventory.cpp:575
    f2 Inventory::Add(int, int, object*, int, int, int, char const*) Descent3/Inventory.cpp:520
    f3 DemoCheats(int) Descent3/GameCheat.cpp:606
    f4 ProcessKeys() Descent3/GameLoop.cpp:2420
    f5 GameFrame() Descent3/GameLoop.cpp:2956
    f6 GameSequencer() Descent3/gamesequence.cpp:1212
    f7 PlayGame() Descent3/game.cpp:826
    f8 MainLoop() Descent3/descent.cpp:554
    f9 Descent3() Descent3/descent.cpp:507
    f10 oeD3LnxApp::run() Descent3/sdlmain.cpp:142
    f11 main Descent3/sdlmain.cpp:323

0x50f00007ab60 is located 0 bytes inside of 175-byte region [0x50f00007ab60,0x50f00007ac0f)
freed by thread T0 here:
    f1 mng_LoadNetGenericPage(CFILE*, bool) manage/generic.cpp:2216
    f2 mng_LoadNetPages(int) manage/manage.cpp:1281
    f3 mng_LoadTableFiles(int) manage/manage.cpp:648
    f4 InitD3Systems2(bool) Descent3/init.cpp:1891
    f5 Descent3() Descent3/descent.cpp:503
    f6 oeD3LnxApp::run() Descent3/sdlmain.cpp:142
    f7 main Descent3/sdlmain.cpp:323

previously allocated by thread T0 here:
    f0 malloc
    f1 mem_rmalloc<char> mem/mem.h:138
    f2 mng_ReadNewGenericPage(CFILE*, mngs_generic_page*) manage/generic.cpp:1145
    f3 mng_LoadNetGenericPage(CFILE*, bool) manage/generic.cpp:2196
    f4 mng_LoadNetPages(int) manage/manage.cpp:1281
    f5 mng_LoadTableFiles(int) manage/manage.cpp:648
    f6 InitD3Systems2(bool) Descent3/init.cpp:1891
    f7 Descent3() Descent3/descent.cpp:503
    f8 oeD3LnxApp::run() Descent3/sdlmain.cpp:142
    f9 main Descent3/sdlmain.cpp:323

The pointer value of mngs_generic_page::description was copied to object_info::description (by function
mng_AssignGenericPageToObjInfo) and then the page was freed in mng_LoadNetGenericPage, leaving object_info::description non-NULL and dangling.

Fixes: d185ab9

Pull Request Type

  • Runtime changes
    • Other changes

Checklist

  • I have tested my changes locally and verified that they work as intended.
  • I have reviewed the changes to ensure they do not introduce any unnecessary complexity or duplicate code.
  • I understand that by submitting this pull request, I am agreeing to license my contributions under the project's license.

Commit d185ab9 broke the
pointer-moving logic. When the allweapons cheat is executed or when
e.g. the afterburner is picked up, ASAN terminates the program with:

```
==8330==ERROR: AddressSanitizer: heap-use-after-free on address 0x50f00007ab60
at pc 0x7f23334f6843 bp 0x7ffe724d2b10 sp 0x7ffe724d22d0
READ of size 3 at 0x50f00007ab60 thread T0
    f0 strdup
    f1 Inventory::AddCounterMeasure(int, int, int, int, char const*) Descent3/Inventory.cpp:575
    f2 Inventory::Add(int, int, object*, int, int, int, char const*) Descent3/Inventory.cpp:520
    f3 DemoCheats(int) Descent3/GameCheat.cpp:606
    f4 ProcessKeys() Descent3/GameLoop.cpp:2420
    f5 GameFrame() Descent3/GameLoop.cpp:2956
    f6 GameSequencer() Descent3/gamesequence.cpp:1212
    f7 PlayGame() Descent3/game.cpp:826
    f8 MainLoop() Descent3/descent.cpp:554
    f9 Descent3() Descent3/descent.cpp:507
    f10 oeD3LnxApp::run() Descent3/sdlmain.cpp:142
    f11 main Descent3/sdlmain.cpp:323

0x50f00007ab60 is located 0 bytes inside of 175-byte region [0x50f00007ab60,0x50f00007ac0f)
freed by thread T0 here:
    f1 mng_LoadNetGenericPage(CFILE*, bool) manage/generic.cpp:2216
    f2 mng_LoadNetPages(int) manage/manage.cpp:1281
    f3 mng_LoadTableFiles(int) manage/manage.cpp:648
    f4 InitD3Systems2(bool) Descent3/init.cpp:1891
    f5 Descent3() Descent3/descent.cpp:503
    f6 oeD3LnxApp::run() Descent3/sdlmain.cpp:142
    f7 main Descent3/sdlmain.cpp:323

previously allocated by thread T0 here:
    f0 malloc
    f1 mem_rmalloc<char> mem/mem.h:138
    f2 mng_ReadNewGenericPage(CFILE*, mngs_generic_page*) manage/generic.cpp:1145
    f3 mng_LoadNetGenericPage(CFILE*, bool) manage/generic.cpp:2196
    f4 mng_LoadNetPages(int) manage/manage.cpp:1281
    f5 mng_LoadTableFiles(int) manage/manage.cpp:648
    f6 InitD3Systems2(bool) Descent3/init.cpp:1891
    f7 Descent3() Descent3/descent.cpp:503
    f8 oeD3LnxApp::run() Descent3/sdlmain.cpp:142
    f9 main Descent3/sdlmain.cpp:323
```

The pointer value of mngs_generic_page::description was copied to
object_info::description (by function
``mng_AssignGenericPageToObjInfo``) and then the page was freed in
``mng_LoadNetGenericPage``, leaving object_info::description non-NULL
and dangling.

Fixes: d185ab9
Copy link
Member

@Lgt2x Lgt2x left a comment

Choose a reason for hiding this comment

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

the fix looks fine, thanks! And apologies for the long review time, I was on vacation/busy the last few weeks :)

@Lgt2x Lgt2x merged commit f7e6dd9 into DescentDevelopers:main Nov 25, 2024
10 checks passed
@jengelh jengelh deleted the inventory branch November 26, 2024 22:44
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.

2 participants