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

requester: fix memory corruption and refactor #1393

Merged
merged 6 commits into from
Jul 1, 2024

Conversation

walkawayy
Copy link
Collaborator

Fixes issues with incorrectly sized items_flags, standardizes requestor functions, and switches to snprintf during savegame scanning.

Checklist

  • I have read the coding conventions
  • I have added a changelog entry about what my pull request accomplishes, or it is an internal change

Description

Part 2 of separating #1389 into more manageable pull requests. Fixes issues with incorrectly sized items_flags, standardizes requestor functions, and switches to snprintf during savegame scanning. Part of the fix for #1374 as this fixes incorrectly sized items_flags which could cause memory corruption. The next and final PR will be the final fix for gameflow bit shifting limiting savegame slots to 64.

@walkawayy walkawayy added TR1X bug A bug with TR1X Internal The invisible stuff labels Jun 27, 2024
@walkawayy walkawayy added this to the 4.2 milestone Jun 27, 2024
@walkawayy walkawayy requested review from rr- and lahm86 June 27, 2024 02:09
@walkawayy walkawayy self-assigned this Jun 27, 2024
src/game/requester.c Outdated Show resolved Hide resolved
src/game/requester.c Outdated Show resolved Hide resolved
src/game/phase/phase_pause.c Outdated Show resolved Hide resolved
src/game/savegame/savegame.c Outdated Show resolved Hide resolved
src/game/savegame/savegame.c Outdated Show resolved Hide resolved
src/game/savegame/savegame.c Show resolved Hide resolved
src/game/requester.c Outdated Show resolved Hide resolved
src/game/requester.c Outdated Show resolved Hide resolved
@walkawayy walkawayy requested a review from rr- June 27, 2024 19:38
Copy link
Collaborator

@rr- rr- left a comment

Choose a reason for hiding this comment

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

I'd like to propose one last improvement - currently we have three array of pointers:

    bool *item_blocked;
    char **item_texts;
    TEXTSTRING *texts[MAX_REQLINES];

I think it'd be elegant to wrap these together in a new structure, REQUESTER_ITEM, and then allocate it once like so:

    req->items = Memory_Alloc(sizeof(REQUESTER_ITEM) * max_items);

src/game/requester.c Show resolved Hide resolved
src/game/requester.c Show resolved Hide resolved
src/game/option/option_passport.c Outdated Show resolved Hide resolved
src/game/option/option_passport.c Outdated Show resolved Hide resolved
src/game/phase/phase_pause.c Outdated Show resolved Hide resolved
src/game/requester.c Outdated Show resolved Hide resolved
src/game/requester.c Outdated Show resolved Hide resolved
src/game/requester.c Outdated Show resolved Hide resolved
src/game/requester.h Outdated Show resolved Hide resolved
@walkawayy walkawayy requested a review from rr- June 28, 2024 17:18
@walkawayy
Copy link
Collaborator Author

Ok I think this is looking a lot better. I removed even more unused vars, malloc the text header, and renamed the blocks for clarity. Tested in valgrind and this code has no memory leaks.

@walkawayy walkawayy linked an issue Jun 30, 2024 that may be closed by this pull request
@walkawayy walkawayy added Priority: high This has a high impact on the core gameplay, or blocks other work. and removed Internal The invisible stuff labels Jun 30, 2024
@rr- rr- merged commit ffdd7bb into LostArtefacts:develop Jul 1, 2024
1 check passed
@walkawayy walkawayy deleted the requester-rework branch July 25, 2024 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: high This has a high impact on the core gameplay, or blocks other work. TR1X bug A bug with TR1X
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Game can crash with more than 16 savegame slots
2 participants