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

VERIFIED FIXED in M6

Status

()

P2
normal
VERIFIED FIXED
20 years ago
20 years ago

People

(Reporter: daniel, Assigned: kmcclusk)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

20 years ago
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)
(Assignee)

Updated

20 years ago
Status: NEW → ASSIGNED

Comment 1

20 years ago
Setting all current Open/Normal to M4.

Comment 2

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

Updated

20 years ago
Target Milestone: M4 → M5
(Reporter)

Comment 3

20 years ago
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.
(Assignee)

Updated

20 years ago
Target Milestone: M5 → M6
(Assignee)

Updated

20 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 20 years ago
Resolution: --- → FIXED
(Assignee)

Comment 4

20 years ago
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

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

Updated

20 years ago
Status: RESOLVED → VERIFIED

Comment 6

20 years ago
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.
You need to log in before you can comment on or make changes to this bug.