2115 - Bug (and comments) for widget/public/StringUtil.h

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[Daniel van der Zee]

Hi,
I've een looking in nsStringUtils.h and found a number of problems.
A new version of the macros with some fixes is below. Please note that the only
system i have is an NT machine with Visual C++ 6, so you might want to check if
it doesn't
break the builds on other configurations.

1. SCOPE OF STACK BUFFER

When NS_ALLOC_CHAR_BUF uses the static buffer, the buffer is declared within
the scope of the if statement. This is not a problem for DEBUG builds, but for
optimized builds the buffer space will be reused for other variables.
Fix: Move the allocation outside of the body of the if statement.

2. NEW[] AND DELETE[]
When allocating a buffer with new[], it should be deallocated with delete[].
Same for NS_FREE_STR_BUF, the function nsString::ToNewCString returns a pointer
to a buffer allocated with new[].

3. TWO VERSIONS
There are two similar versions of nsStringUtil.h on my tree: one in
/widget/public and one in /widget/src/windows. I removed the latter and rebuild
the files in widget, no errors whatsoever.

4. _ns_smallBufUsed not needed
The variable _ns_smallBufUsed is not needed, to check if the buffer should be
deallocated, just compare the value of aBuf with the address of the static
buffer.

5. < and <=
For NS_ALLOC_CHAR_BUF, the argument aActualSize should probably contain the
number
of bytes requested. The if-statement should check for <= instead of "aActualSize
< _ns_kSmallBufferSize"

6. CONST-NESS
Because the delete statement frees the buffer aBuf is pointing to, the user of
the
function should not change the location aBuf is pointing to, to prevent this
aBuf can be
declared as:
   char * const aBuf;

7. Two function calls
In NS_ALLOC_CHAR_BUF the argument aActualSize will probably be a function call,
since
it is used twice, it makes sense of use a temporary variable to prevent the
second call.
NS_ALLOC_STR_BUF already does this (but why is that variable static?).

8. Merging
I think it makes sense to call NS_ALLOC_CHAR_BUF from NS_ALLOC_STR_BUF.

General comment: why not use alloca?

---

#define NS_ALLOC_CHAR_BUF(aBuf, aSize, aActualSize)         \
	int _ns_tmpActualSize = aActualSize;			    	\
	char _ns_smallBuffer[aSize];
\
	char * const aBuf = _ns_tmpActualSize <= aSize ?	    \
		_ns_smallBuffer : new char[_ns_tmpActualSize];

#define NS_FREE_CHAR_BUF(aBuf)
\
	if (aBuf != _ns_smallBuffer)
\
		delete[] aBuf;


#define NS_ALLOC_STR_BUF(varName, strName, tempSize)		\
	NS_ALLOC_CHAR_BUF(varName, tempSize, strName.Length()+1);\
	strName.ToCString(varName, strName.Length()+1);

#define NS_FREE_STR_BUF(varName)                \
	NS_FREE_CHAR_BUF(varName)

Comment 1

[leger]

Setting all current Open/Normal to M4.

Comment 2

[Paul MacQuiddy]

per leger, assigning QA contacts to all open bugs without QA contacts according
to list at http://bugzilla.mozilla.org/describecomponents.cgi?product=Browser

Comment 3

[Daniel van der Zee]

I'm not sure if it's clear in the text, but point #1 means that stack space
will be re-used for other variables, overwriting the contents of a buffer
that could still be un use.
This will break release builds.

I suggest to at least move the
   char _ns_smallBuffer[_ns_kSmallBufferSize];
outside of the if statement ASAP as this might pop-up in strange and
unexpected places.

Comment 4

[Kevin McCluskey (gone)]

Fixed in 5-10-99 4:00pm build. Removed widget/src/windows/nsStringUtil.h,
nsStringUtil.cpp. widget/public now contains a nsStringUtil.h with the fixes
described in this bug report. Moved GetAPCString from nsStringUtil.h to nsMenu
since GetAPCString is WIN32 specific.

Comment 5

[Paul MacQuiddy]

zee, can you verify these fixes? (and then mark bug as VERIFIED). Thanks.

Comment 6

[leger]

Moving all Widget Set bugs, past and present, to new HTML Form Controls
component per request from karnaze.  Widget Set component will be retired
shortly.