Skip to content

bugfix(worldbuilder): Check list size before accessing first element in ObjectOptions::_FindOrDont #2306

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

Open
Caball009 wants to merge 2 commits into
base: main
Choose a base branch
from
+2 −2
Open

bugfix(worldbuilder): Check list size before accessing first element in ObjectOptions::_FindOrDont #2306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7262b0d
bugfix(worldbuilder): Check list size before accessing first element …
Caball009 Feb 15, 2026
2f5d827
Changed loops.
Caball009 Feb 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Generals/Code/Tools/WorldBuilder/src/ObjectOptions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ HTREEITEM ObjectOptions::_FindOrDont(const char* pLabel, HTREEITEM startPoint)
std::list<HTREEITEM> itemsToEx;
itemsToEx.push_back(startPoint);

while (itemsToEx.front()) {
while (!itemsToEx.empty() && itemsToEx.front()) {
Copy link

@xezon xezon Feb 15, 2026

Choose a reason for hiding this comment

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

Is HTREEITEM a pointer?

Maybe just do while (!itemsToEx.empty())? Why test for front() here?

Copy link

@Mauller Mauller Feb 15, 2026

Choose a reason for hiding this comment

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

I agree, only testing for empty should resolve this here, as when not empty you should always have a front iterator.

Copy link
Author

@Caball009 Caball009 Feb 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

It's not clear to me if the code can handle a nullptr as element of itemsToEx, so this seemed to me the safest option.

Copy link
Author

@Caball009 Caball009 Feb 15, 2026

Choose a reason for hiding this comment

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

Is HTREEITEM a pointer?

Yes. typedef struct _TREEITEM *HTREEITEM;

Copy link

@xezon xezon Feb 15, 2026

Choose a reason for hiding this comment

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

Is there any evidence that null is added into the container?

Copy link
Author

@Caball009 Caball009 Feb 15, 2026

Choose a reason for hiding this comment

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

https://learn.microsoft.com/en-us/cpp/mfc/reference/ctreectrl-class?view=msvc-170#getchilditem
https://learn.microsoft.com/en-us/cpp/mfc/reference/ctreectrl-class?view=msvc-170#getnextsiblingitem
can return a nullptr in theory. I don't know if that's possible in our use case.

Copy link

@xezon xezon Feb 15, 2026

Choose a reason for hiding this comment

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

Perhaps prevent it from adding null to the list? What would be the point of a null entry in the list? Would it do anything?

Copy link
Author

@Caball009 Caball009 Feb 15, 2026

Choose a reason for hiding this comment

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

Perhaps prevent it from adding null to the list?

That requires 2 checks, or 3 if we can't guarantee that the function parameter is never null.

What would be the point of a null entry in the list? Would it do anything?

I'm not sure and frankly I'm not sure it's worth my time to find out. I'd assume that replacing the MFC code is going to happen if we decide to revamp the World Builder.

char buffer[_MAX_PATH];
HTREEITEM hItem = itemsToEx.front();
itemsToEx.pop_front();
Expand Down
2 changes: 1 addition & 1 deletion GeneralsMD/Code/Tools/WorldBuilder/src/ObjectOptions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ HTREEITEM ObjectOptions::_FindOrDont(const char* pLabel, HTREEITEM startPoint)
std::list<HTREEITEM> itemsToEx;
itemsToEx.push_back(startPoint);

while (itemsToEx.front()) {
while (!itemsToEx.empty() && itemsToEx.front()) {
char buffer[_MAX_PATH];
HTREEITEM hItem = itemsToEx.front();
itemsToEx.pop_front();
Expand Down
Loading