diff --git a/src/kits/interface/Deskbar.cpp b/src/kits/interface/Deskbar.cpp index 50ca6f12a3..e91c4ecd14 100644 --- a/src/kits/interface/Deskbar.cpp +++ b/src/kits/interface/Deskbar.cpp @@ -139,6 +139,38 @@ status_t BDeskbar::Expand(bool expand) //------------------------------------------------------------------------------ status_t BDeskbar::GetItemInfo(int32 id, const char **name) const { + /* NOTE: Be's implementation returned B_BAD_VALUE if *name was NULL, + not just if name was NULL. This doesn't make much sense. Be's + implementation means you cannot do the following: + + const char *buffer = NULL; + myDeskbar.GetItemInfo(id, &buffer); + + Instead, you are forced to write code that looks like: + + char tmpBuf[10]; + const char *buffer = tmpBuf; + myDeskbar.GetItemInfo(id, &buffer); + + There are a couple of issues with this: + - Be's implementation does not use the space pointed to in buffer. + It cannot since it can't tell how big that space is and it won't + know whether the item's name will fit without overflowing the + buffer. + - Worse, if the code looked like: + + const char *buffer = new char[5]; + myDeskbar.GetItemInfo(id, &buffer); + + The code will result in a memory leak. The problem here is that + what buffer points to is changed by GetItemInfo(). If buffer + points to dynamically allocated memory, there is a good chance + the result is a memory leak. + + The OpenBeOS implementation will allow *name to point to NULL or + non-NULL. If anything, we should consider forcing *name to point to + NULL for safety. + */ if (name == NULL) { return(B_BAD_VALUE); } diff --git a/src/kits/interface/interface.src b/src/kits/interface/interface.src index 504f73c092..eeb91a8072 100644 --- a/src/kits/interface/interface.src +++ b/src/kits/interface/interface.src @@ -5,6 +5,7 @@ INTERFACE_KIT_SOURCE = Button.cpp CheckBox.cpp Control.cpp + Deskbar.cpp PictureButton.cpp Point.cpp RadioButton.cpp diff --git a/src/tests/kits/interface/bdeskbar/DeskbarGetItemTest.cpp b/src/tests/kits/interface/bdeskbar/DeskbarGetItemTest.cpp index b82735b8b2..4d9ee6fc9e 100644 --- a/src/tests/kits/interface/bdeskbar/DeskbarGetItemTest.cpp +++ b/src/tests/kits/interface/bdeskbar/DeskbarGetItemTest.cpp @@ -1,5 +1,5 @@ /* - $Id: DeskbarGetItemTest.cpp,v 1.2 2002/09/13 03:51:09 jrand Exp $ + $Id: DeskbarGetItemTest.cpp,v 1.3 2002/09/25 03:24:17 jrand Exp $ This file implements tests for the following use cases of BDeskbar: - Count Items @@ -68,8 +68,17 @@ if (myDeskbar.HasItem(id)) { itemCount--; + /* In Be's implementation, if the char * points to NULL, it + returns B_BAD_VALUE. However, no matter what it points to + before you start, it is changed by the call to GetItemInfo(). + So, if it points to allocated memory, there is a good chance + for a leak. I would argue that Be should return B_BAD_VALUE + if it points to non-NULL. The OpenBeOS implementation does + not return B_BAD_VALUE in this case so this assert would fail + from OpenBeOS. However, this is considered to be an acceptable + deviation from Be's implementation. name = NULL; - assert(myDeskbar.GetItemInfo(id, &name) == B_BAD_VALUE); + assert(myDeskbar.GetItemInfo(id, &name) == B_BAD_VALUE); */ name = buffer; assert(myDeskbar.GetItemInfo(id, &name) == B_OK);