Strange AnsiString corruption when terminating a thread

[Richard M. Ulrich]

Hello,

I'm using C-Builder 2007, Windows XP SP3. I have a program that contains a
relatively simple thread, and it also contains a module that defines a
global variable which is a structure, several entries of which are
AnsiStrings. One of those AnsiStrings are getting corrupted when the thread
is terminated by the following function, and I can't see why.

void __fastcall TMainForm::RestartMonitoring(void)
{
CheckMemory(); //This passes, gl_Options.caseFileName is intact
if (OutputMonitor) {
try {
OutputMonitor->OnTerminate = NULL; //We don't want to trigger the
auto-restart code
OutputMonitor->Terminate();
CheckMemory(); //this fails, gl_Options.caseFileName has been
mangled
} catch (...) {
}
}
...
}

More data: FreeOnTerminate is true; the corruption appears to involve just
two bytes getting changed; if I comment out the OnTerminate = NULL line,
the corruption is just one byte; the thread class does not include the
header file that defines gl_Options, so it ought not be able to touch that
memory; setting a data breakpoint does not trap the change.

I suppose I'll just stop using AnsiStrings and see if the problem goes away,
but I wonder if anyone has run into something like this before, where the
shared memory pool for AnsiStrings appears to get corrupted (assuming that
is the problem).

R

[Richard M. Ulrich]

The problem seems to be that the thread had already been terminated. I had
assumed that calling Terminate() on an already terminated (and freed) thread
would result in an exception being thrown, but evidently not. Instead it
corrupts the stack.

R

[Remy Lebeau (TeamB)]

Post by Richard M. Ulrich
The problem seems to be that the thread had already been terminated.
I had assumed that calling Terminate() on an already terminated (and
freed) thread would result in an exception being thrown, but evidently
not.

Terminate() sets the TThread::Terminated property to False and nothing else.
It does not perform any kind of object or thread validation at all. If the
thread is already terminated, then Terminate() will simply write a byte to
whereever the thread object used to reside in memory. If someone else
occupied that memory now, it will be overwritten.

Post by Richard M. Ulrich
Instead it corrupts the stack.

It corrupts the heap, not the stack.


Gambit

[Richard M. Ulrich]

Post by Remy Lebeau (TeamB)

Post by Richard M. Ulrich
Instead it corrupts the stack.

It corrupts the heap, not the stack.

Right.

R

[Clayton Arends]

Post by Richard M. Ulrich
void __fastcall TMainForm::RestartMonitoring(void)
{
CheckMemory(); //This passes, gl_Options.caseFileName is intact
if (OutputMonitor) {
try {
OutputMonitor->OnTerminate = NULL; //We don't want to trigger
the auto-restart code
OutputMonitor->Terminate();
CheckMemory(); //this fails, gl_Options.caseFileName has
been mangled
} catch (...) {
}
}
...
}

I assume your OnTerminate handler sets OutputMonitor to NULL. You have a
thread synchronization bug. There is nothing here that guarantees that
between "if (OutputMonitor)" and "OutputMonitor->OnTerminate = NULL;" the
termination handler isn't called. You need to synchronize access to the
thread object to guarantee that your logic will work. Use of a
TCriticalSection object will suffice.

You need to sync before the first CheckMemory() and then release the sync
after the "if" statement block. Likewise, in your OnTerminate handler you
need to sync on the first line of the handler.

Clayton

[Richard M. Ulrich]

Hi,

Thanks for your reply. Yes, my OnTerminate handler set OutputMonitor to
NULL. It turns out that the offending piece of code was getting invoked
after the thread was told to terminate, but before the OnTerminate callback
function got executed.

So while setting a critical section is a good idea to guard against the
small chance of the thread dying right in the middle, it wouldn't have
helped in this case. The thread was in an in-between state (waiting for the
OnTerminate handler to run) start to finish. My fix was to set
OutputMonitor to NULL immediately after issuing the Terminate() command.

While I concede that my design was flawed to let this happen, it still seems
to me that there should have been an exception thrown in this case, rather
than just stack corruption.

Related thread compiler question. I now have

OutputMonitor->Terminate();
OutputMonitor = NULL;

as two consecutive lines in the main thread space. Certainly all sorts of
things could happen in other threads in between those two lines, but am I
correct that we are guaranteed no other main thread code chunk (i.e.,
anything invoked via TThread::Synchronize()) can be inserted between?

R

Post by Clayton Arends
I assume your OnTerminate handler sets OutputMonitor to NULL. You have a
thread synchronization bug. There is nothing here that guarantees that
between "if (OutputMonitor)" and "OutputMonitor->OnTerminate = NULL;" the
termination handler isn't called. You need to synchronize access to the
thread object to guarantee that your logic will work. Use of a
TCriticalSection object will suffice.
You need to sync before the first CheckMemory() and then release the sync
after the "if" statement block. Likewise, in your OnTerminate handler you
need to sync on the first line of the handler.
Clayton

[Alan Bellingham]

Post by Richard M. Ulrich
While I concede that my design was flawed to let this happen, it still seems
to me that there should have been an exception thrown in this case, rather
than just stack corruption.

Why do you say this? For an exception to be thrown, something has to
*actively* look for the problem that it's reporting on. Having code
constantly executing just in case someone has written bugs into their
code is not the way C or C++ tend to work.

If you're really lucky, the OS may notice certain types of corruption
(trying to write into non-write-enabled memory pages, for example) and
cause a hardware fault which gets translated into a C++ exception. But
you shouldn't expect that: write as if you were rock climbing, and don't
rely on your rope.

Post by Richard M. Ulrich
Related thread compiler question. I now have
OutputMonitor->Terminate();
OutputMonitor = NULL;
as two consecutive lines in the main thread space. Certainly all sorts of
things could happen in other threads in between those two lines, but am I
correct that we are guaranteed no other main thread code chunk (i.e.,
anything invoked via TThread::Synchronize()) can be inserted between?

Your main thread cannot be executing two things at once, by definition.
On the other hand, you *do* have to watch out for things like
reentrancy: perhaps Terminate() might cause RestartMonitoring() to be
called. To avoid stuff like that, you might prefer to do something like:

ptr_type LocalPointer = OutputMonitor;
OutputMonitor = NULL;
LocalPointer->OnTerminate = NULL; //We don't want ...
LocalPointer->Terminate();

Since LocalPointer is a local, stack-based variable that you're not
taking the address of in any way, nothing else can possible know about
it.

Alan Bellingham

[Remy Lebeau (TeamB)]

Post by Richard M. Ulrich
OutputMonitor->OnTerminate = NULL;
OutputMonitor->Terminate();
CheckMemory(); //this fails, gl_Options.caseFileName has been
mangled

Make sure your OutputMonitor pointer is valid. Otherwise, calling
Terminate() could be writing a value to random memory.

Post by Richard M. Ulrich
More data: FreeOnTerminate is true

Get rid of that. Leave it set to false. It is VERY DANGEROUS practice to
set FreeOnTerminate to true for any thread that you do any kind of
management on. You are not guaranteed the thread object will be valid at
any given moment. The fact that you want to access OnTerminate and
Terminate() in your code pretty much requires that FreeOnTerminate be false.

Post by Richard M. Ulrich
the corruption appears to involve just two bytes getting changed;

You are writing 8 bytes (two pointers) when setting OnTerminate to NULL, and
another 1 byte when calling Terminate(). If the pointer is not pointing at
a valid thread object, then those 9 bytes can be going anywhere.

Post by Richard M. Ulrich
the thread class does not include the header file that defines gl_Options,
so it ought not be able to touch that memory

Not true.

Post by Richard M. Ulrich
I suppose I'll just stop using AnsiStrings and see if the problem goes
away

Or, you could fix your code to use threads correctly.


Gambit

[Richard M. Ulrich]

Post by Remy Lebeau (TeamB)

Post by Richard M. Ulrich
I suppose I'll just stop using AnsiStrings and see if the problem goes
away

Or, you could fix your code to use threads correctly.

A much better solution! :-) The other may have just masked the problem.

R