Skip to content

[GEN][ZH] Fix buffer overrun and memory leaks in listbox properties of GUIEdit #796

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

[GEN][ZH] Fix buffer overrun and memory leaks in listbox properties of GUIEdit #796

wants to merge 9 commits into from
+22 −18

Conversation

Caball009
Copy link

Fixes a potential 'heap' overflow in GUIEdit: the actual buffer size is smaller than the claimed buffer size passed to GetDlgItemTextA.

@xezon xezon added this to the Stability fixes milestone May 2, 2025
@xezon xezon added Minor Severity: Minor < Major < Critical < Blocker Tools Affects Tools only Fix Is fixing something Stability Concerns stability of the runtime Crash This is a crash and removed Crash This is a crash labels May 2, 2025
xezon
xezon previously approved these changes May 2, 2025
View reviewed changes
Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

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

Looks good. Also it looks like there are a ton of memory leaks in this code. Perhaps can tackle that in a follow up change. Some of these new char can also be changed to stack buffers.

@xezon xezon changed the title [GEN][ZH] Tools/GUIEdit - Listbox properties overflow fix [GEN][ZH] Fix listbox properties overflow in GUIEdit May 2, 2025
@xezon xezon changed the title [GEN][ZH] Fix listbox properties overflow in GUIEdit [GEN][ZH] Fix buffer overrun in listbox properties of GUIEdit May 2, 2025
@Caball009
Copy link
Author

Caball009 commented May 2, 2025 •
edited
Loading

Looks good. Also it looks like there are a ton of memory leaks in this code. Perhaps can tackle that in a follow up change. Some of these new char can also be changed to stack buffers.

Yeah, I noticed that as well. I'll try to make some time to open a new pr for the memory leaks.

Would you say that this would be the right approach? (Char vs. char and sizeof vs. 60)

Char percentages[60];
GetDlgItemText(... percentages,sizeof(percentages));

EDIT: On second thought, it's probably preferable to put those memory leak fixes in this pr instead of a separate one.

@xezon
Copy link

xezon commented May 2, 2025

Would you say that this would be the right approach? (Char vs. char and sizeof vs. 60)

Yes this looks good.

On second thought, it's probably preferable to put those memory leak fixes in this pr instead of a separate one.

You can do that yes.

@Caball009 Caball009 closed this May 3, 2025
@Caball009 Caball009 deleted the ListboxProperties_Overflow_Fix branch May 3, 2025 19:28
@Caball009 Caball009 restored the ListboxProperties_Overflow_Fix branch May 3, 2025 19:29
@Caball009 Caball009 reopened this May 3, 2025
@Caball009
Copy link
Author

I think the memory leaks are handled now.

@Caball009 Caball009 requested a review from xezon May 3, 2025 20:04
@Caball009 Caball009 changed the title [GEN][ZH] Fix buffer overrun in listbox properties of GUIEdit [GEN][ZH] Fix buffer overrun and memory leaks in listbox properties of GUIEdit May 5, 2025
xezon
xezon reviewed May 5, 2025
View reviewed changes
Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

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

Did you test this change? Does it work?

GeneralsMD/Code/Tools/GUIEdit/Source/Dialog Procedures/ListboxProperties.cpp
char *percentages = new char[60];
char *tempStr = new char[60];
Char percentages[100];
Char tempStr[100];
Copy link

Choose a reason for hiding this comment

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

33

GeneralsMD/Code/Tools/GUIEdit/Source/Dialog Procedures/ListboxProperties.cpp
@@ -941,8 +943,8 @@ HWND InitListboxPropertiesDialog( GameWindow *window )
SetDlgItemInt( dialog, EDIT_NUM_COLUMNS, listData->columns, FALSE );
if(listData->columns > 1)
{
char *percentages = new char[60];
char *tempStr = new char[60];
Char percentages[100];
Copy link

Choose a reason for hiding this comment

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

Why is the number 100 chosen?

Copy link
Author

Choose a reason for hiding this comment

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

No particular reason other than that it's slightly bigger than it used to be.

Generals/Code/Tools/GUIEdit/Source/Dialog Procedures/ListboxProperties.cpp
char *percentages = new char[60];
char *token;
GetDlgItemText(hWndDialog,EDIT_COLUMN_PERCENT,percentages,200);
Char percentages[100];
Copy link

Choose a reason for hiding this comment

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

Why is this 100? Prior sizes were 60 or 200.

Copy link
Author

Choose a reason for hiding this comment

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

No particular reason other than that it's slightly bigger than it used to be.

@xezon xezon dismissed their stale review May 5, 2025 16:49

Rereview

@Caball009
Copy link
Author

Did you test this change? Does it work?

I haven't been able to test, so I'm afraid I can't answer that.

@xezon
Copy link

xezon commented May 5, 2025

It would would be good to give this a brief test before commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xezon xezon xezon left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Fix Is fixing something Minor Severity: Minor < Major < Critical < Blocker Stability Concerns stability of the runtime Tools Affects Tools only
Projects
None yet
Milestone
Stability fixes
Development

Successfully merging this pull request may close these issues.
2 participants
@Caball009 @xezon