Opened 5 years ago

Closed 5 years ago

#10733 closed bug (fixed)

[BOptionPopUp] can crash when containing a SeparatorItem

Reported by: ttcoder Owned by: jscipione
Priority: normal Milestone: R1
Component: Kits/Interface Kit Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

BOptionPopUp does not provide explicit separator-item support, but it exposes MenuField()->Menu()->AddSeparatorItem() to do so. I used that to make a menu look better/more structured, but realized that all further calls to GetOptionAt() will crash, probably because unlike BMenuItem, SeparatorItem lacks a Message() field.

Attachments (3)

0001_1-BOptionPopUp-check-for-non-NULL-Message.patch (1.4 KB) - added by ttcoder 5 years ago.
Version 1 (most inclusive, tries hard to return all items as options)
0001_1-BOptionPopUp-check-for-non-NULL-Message.2.patch (1.4 KB) - added by ttcoder 5 years ago.
Version 1 (most inclusive, tries hard to return all items as options)
0001_2-BOptionPopUp-check-for-non-NULL-Message.patch (879 bytes) - added by ttcoder 5 years ago.
Version 2 (consistent with SetValue(), which checks for Message() at the same early stage too).

Download all attachments as: .zip

Change History (12)

comment:1 Changed 5 years ago by ttcoder

I guess this diff would be best, as this returns false when the field is not present (by opposed to testing for the field only later down in the inner if() ):

--- a/src/kits/interface/OptionPopUp.cpp
+++ b/src/kits/interface/OptionPopUp.cpp
@@ -104,7 +104,7 @@ BOptionPopUp::GetOptionAt(int32 index, const char** outName,
 
        if (menu != NULL) {
                BMenuItem* item = menu->ItemAt(index);
-               if (item != NULL) {
+               if (item != NULL && item->Message() != NULL) {
                        if (outName != NULL)
                                *outName = item->Label();
                        if (outValue != NULL)

Here's the stack crawl BTW:

		state: Exception (Segment violation)

		Frame		IP			Function Name
		-----------------------------------------------
		0x635ecbf8	0xf3cc20	BMessage::_FindField(BMessage, char*, uint32, BMessage::field_header*) + 0x28
			Frame memory:
			[0x635ecbd8]  .x........^c6...   f8 78 13 01 00 00 00 00 e0 cc 5e 63 36 ce f3 00
			[0x635ecbe8]  .....Z..0.^cm...   8d 9c a8 03 d4 5a 9a 00 30 cc 5e 63 6d d3 f3 00
		0x635ecc38	0xf3d368	BMessage::FindData(BMessage, char*, uint32, int32, void*, void*) + 0x48
		0x635ecc88	0xf3ecd2	BMessage::FindInt32(BMessage, char*, void*) + 0x46
		0x635eccb8	0xfaff5f	BOptionPopUp::GetOptionAt(int32, char*, void*) + 0x83

comment:2 Changed 5 years ago by korli

I bet we don't want to skip options without message. So, no, the check should happen on the "if (outValue != NULL)" line.

comment:3 Changed 5 years ago by ttcoder

That would fix the crash in my case just as well indeed.

One might want to update the BeBook (if necessary) in that case, to mention that GetOptionAt() would return true not only for menu items that were created as actual "options" (with the AddOptionAt() method) but also for custom-made menu items and for separator items. Thus it would rarely if ever return 'false'. Probably makes sense. I'm happy either way.. With one caveat:

Beware that SelectOptionFor()..

http://cgit.haiku-os.org/haiku/tree/src/kits/interface/OptionControl.cpp#n95

... does not initialize its "int32 optionValue" parameter (that's an awful practice that plagues the whole Haiku source tree, but that's a discussion for another time and place..) when passing it to GetOptionAt(), so one would need to either 1) initialize that variable, knowing it won't be affected in GetOptionAt(), or 2) change GetOptionAt() so that it sets it to 0 (or -1) if it cannot find the Message() field in the menu item, otherwise we get the dreaded "undefined behavior"..!

comment:4 Changed 5 years ago by axeld

Can I drag you into submitting a patch for this? :-)

Changed 5 years ago by ttcoder

Version 1 (most inclusive, tries hard to return all items as options)

comment:5 Changed 5 years ago by ttcoder

Has a Patch: set

Changed 5 years ago by ttcoder

Version 1 (most inclusive, tries hard to return all items as options)

Changed 5 years ago by ttcoder

Version 2 (consistent with SetValue(), which checks for Message() at the same early stage too).

comment:6 Changed 5 years ago by ttcoder

Heck why not :-) It's in an area I am skilled enough for, and you guys need encouragement/contribs to keep up the good work. Attached two versions, the latter being my personal favorite, but both of them fix the crash. Let's see if I messed up following the directions in the wiki or not..

comment:7 Changed 5 years ago by jscipione

Owner: changed from axeld to jscipione
Status: newassigned

comment:8 Changed 5 years ago by ttcoder

Patch applied in hrev47375 thanks!

comment:9 Changed 5 years ago by jscipione

Resolution: fixed
Status: assignedclosed

Fixed in hrev47375 thanks to ttcoder for reporting and supplying a patch, Axel and korli for reviewing and waddlesplash for bringing the bug to my attention.

Note: See TracTickets for help on using tickets.