- Failure to Distinguish Scalar and Array Allocation
- Checking for Allocation Failure
- Replacing Global New and Delete
- Confusing Scope and Activation of Member new and delete
- Throwing String Literals
- Improper Exception Mechanics
- Abusing Local Addresses
- Failure to Employ Resource Acquisition Is Initialization
- Improper Use of auto_ptr
This chapter is from the book
This chapter is from the book
This chapter is from the book
Gotcha #66: Abusing Local Addresses
Don't return a pointer or reference to a local variable. Most compilers will warn about this situation; take the warning seriously.
Disappearing Stack Frames
If the variable is an automatic, the storage to which it refers will disappear on return:
char *newLabel1() {
static int labNo = 0;
char buffer[16]; // see Gotcha #2
sprintf( buffer, "label%d", labNo++ );
return buffer;
}
This function has the annoying property of working on occasion. After return, the stack frame for the newLabel1 function is popped off the execution stack, releasing its storage (including the storage for buffer) for use by a subsequent function call. However, if the value is copied before another function is called, the returned pointer, though invalid, may still be usable:
char *uniqueLab = newLabel1(); char mybuf[16], *pmybuf = mybuf; while( *pmybuf++ = *uniqueLab++ );
This is not the kind of code a maintainer will put up with for very long. The maintainer might decide to allocate the buffer off the heap:
char *pmybuf = new char[16];
The maintainer might decide not to hand-code the buffer copy:
strcpy( pmybuf, uniqueLab );
The maintainer might decide to use a more abstract type than a character buffer:
std::string mybuf( uniqueLab );
Any of these modifications may cause the local storage referred to by uniqueLab to be modified.
Static Interference
If the variable is static, a later call to the same function will affect the results of earlier calls:
char *newLabel2() {
static int labNo = 0;
static char buffer[16];
sprintf( buffer, "label%d", labNo++ );
return buffer;
}
The storage for the buffer is available after the function returns, but any other use of the function can affect the result:
//case 1 cout << "first: " << newLabel2() << ' '; cout << "second: " << newLabel2() << endl; // case 2 cout << "first: " << newLabel2() << ' ' << "second: " << newLabel2() << endl;
In the first case, we'll print different labels. In the second case, we'll probably (but not necessarily) print the same label twice. Presumably, someone who was intimately aware of the unusual implementation of the newLabel2 function wrote case 1 to break up the label output into separate statements, to take that flawed implementation into account. A later maintainer is unlikely to be as familiar with the implementation vagaries of newLabel2 and is likely to merge the separate output statements into one, causing a bug. Worse, the merged output statement could continue to exhibit the same behavior as the separate statements and change unpredictably in the future. (See Gotcha #14.)
Idiomatic Difficulties
Another danger is lurking as well. Keep in mind that users of a function generally do not have access to its implementation and therefore have to determine how to handle a function's return value from a reading of the function declaration. While a comment may provide this information (see Gotcha #1), it's also important that the function be designed to encourage proper use.
Avoid returning a reference that refers to memory allocated within the function. Users of the function will invariably neglect to delete the storage, causing memory leaks:
int &f()
{ return *new int( 5 ); }
// . . .
int i = f(); // memory leak!
The correct code has to convert the reference to an address or copy the result and free the memory. Not on my shift, buddy:
int *ip = &f(); // one horrible way int &tmp = f(); // another int i = tmp; delete &tmp;
This is a particularly bad idea for overloaded operator functions:
Complex &operator +( const Complex &a, const Complex &b )
{ return *new Complex( a.re+b.re, a.im+b.im ); }
// . . .
Complex a, b, c;
a = b + c + a + b; // lots of leaks!
Return a pointer to the storage instead, or don't allocate storage and return by value:
int *f() { return new int(5); }
Complex operator +( Complex a, Complex b )
{ return Complex( a.re+b.re, a.im+b.im ); }
Idiomatically, users of a function that returns a pointer expect that they might be responsible for the eventual deletion of the storage referred to by the pointer and will make some effort to determine whether this is actually the case (say, by reading a comment). Users of a function that returns a reference rarely do.
Local Scope Problems
The problems we encounter with lifetimes of local variables can occur not only on the boundaries between functions but also within the nested scopes of an individual function:
void localScope( int x ) {
char *cp = 0;
if( x ) {
char buf1[] = "asdf";
cp = buf1; // bad idea!
char buf2[] = "qwerty";
char *cp1 = buf2;
// . . .
}
if( x-1 ) {
char *cp2 = 0; // overlays buf1?
// . . .
}
if( cp )
printf( cp ); // error, maybe . . .
}
Compilers have a lot of flexibility in how they lay out the storage for local variables. Depending on the platform and compiler options, the compiler may overlay the storage for buf1 and cp2. This is legal, because buf1 and cp2 have disjoint scope and lifetime. If the overlay does occur, buf1 will be corrupted, and the behavior of the printf may be affected (it probably just won't print anything). For the sake of portability, it's best not to depend on a particular stack frame layout.
The Static Fix
When faced with a difficult bug, sometimes the problem "goes away" with an application of the static storage class specifier:
// . . .
char buf[MAX];
long count = 0;
// . . .
int i = 0;
while( i++ <= MAX )
if( buf[i] == '\0' ) {
buf[i] = '*';
++count;
}
assert( count <= i );
// . . .
This code has a poorly written loop that will sometimes write past the end of the buf array into count, causing the assertion to fail. In the wild thrashing that sometimes accompanies attempts to bug fix, the programmer may declare count to be a local static, and the code will then work:
char buf[MAX];
static long count;
// . . .
count = 0;
int i = 0;
while( i++ <= MAX )
if( buf[i] == '\0' ) {
buf[i] = '*';
++count;
}
assert( count <= i );
Many programmers, not willing to question their good luck in fixing the problem so easily, will leave it at that. Unfortunately, the problem has not gone away; it has just been moved somewhere else. It's lying in wait, ready to strike at a future time.
Making the local variable count static has the effect of moving its storage out of the stack frame of the function and into an entirely different region of memory, where static objects are located. Because it has moved, it will no longer be overwritten. However, not only is count now subject to the problems mentioned under "Static Interference" above; it's also likely that another local variable—or a future local variable—is being overwritten. The proper solution is, as usual, to fix the bug rather than hide it:
char buf[MAX];
long count = 0;
// . . .
for( int i = 1; i < MAX; ++i )
if( buf[i] == '\0' ) {
buf[i] = '*';
++count;
}
// . . .