[BOptionPopUp] can crash when containing a SeparatorItem

[ttcoder]

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.

[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

[korli]

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

[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"..!

[axeld]

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

[ttcoder]

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

[ttcoder]

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

[ttcoder]

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

[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..

[ttcoder]

Patch applied in hrev47375 thanks!

[jscipione]

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.