Invalid access to memory location using GlobalFree

Under Microsoft, I use GlobalAlloc and GlobalFree to dynamically allocate and free memory. Intermittently GlobalFree is reporting error 998 (GetLastError() - Invalid access to memory location). I've checked my logs of addresses returned by GlobalAlloc against those being freed by GlobalFree and when the problem occurs the address causing the error has been allocated by GlobalAlloc and has not been freed before since allocation.

I can't reproduce it. Currently I'm checking for error 998 from GlobalFree and ignoring it if it occurs - which is not satisfactory. I have one function (AllocMem) that calls GlobalAlloc and checks for errors and another function (FreeMem) which calls GlobalFree and checks for errors. My program uses AllocMem and FreeMem everywhere memory is required or freed. AllocMem and FreeMem are in a DLL. The program is heavily threaded and a different thread may call AllocMem to the one that calls FreeMem. I don't believe I've got a race condition etc as all the code in the program containing the AllocMem and FreeMem calls are part of a CriticalSection using the same variable.

Could some kind guru explain what is a possible cause of this error?
Any help appreciated.

first of all ( and for curiosity :) ), why are you using GlobalAlloc ? ( instead of the standard allocation functions and/or the Windows heap allocation APIs )

free doesn't return an error whilst GlobalFree does. The Windows heap allocation APIs could now be used but historically GlobalAlloc/GlobalFree were used before know about the Windows heap allocation. They have been used in the DLL that the program link to for many years.

Quote:

---

Originally Posted by 2kaud

Could some kind guru explain what is a possible cause of this error?

---

A bug in your program.

Seriously, no one would know what is wrong unless they have your code, run the program, duplicate the error, and start debugging. All we have is your word that your logs and whatever else you're using shows you that things are "OK". So the only conclusion is that your logs are wrong or inadequate in some way. Otherwise, you've discovered a bug in the Windows OS that no one else has encountered (which of course has a probability of occurring at close to 0).

Quote:

---

I don't believe I've got a race condition etc

---

When you are dealing with multithreaded apps, you have to be sure that your code is safe -- not just "believe", but be 100% sure. If it isn't safe code, then you need to be experienced enough to identify and fix any synchronization issues.

Regards,

Paul McKenzie

The problem I've got is that I can't duplicate the error outside of the program (which is a few thousand lines) and even within the program it is very intermittent. I accept a bug in the program. What could be the possible cause of error 998 Invalid access to memory location?

On a slightly higher level, normal debug techniques are not really going to help here. Does anyone know of 3rd party program analysis software that might be of help? If the problem is a multi-threading sync issue, again are there 3rd party analysis software that might pick up these rare intermittent problems?

Quote:

---

Originally Posted by 2kaud

The problem I've got is that I can't duplicate the error outside of the program (which is a few thousand lines) and even within the program it is very intermittent

---

That has all the tell-tale signs of a synchronization issue.

This is not easy to identify, so you need to do a complete code review and understand where, when, how and who accesses global variables, whether functions themselves are thread-safe, etc. Maybe it's just wise to identify why you need these global variables, and possibly change it to local variables or objects in some way so as to avoid needing critical sections.

Quote:

---

What could be the possible cause of error 998 Invalid access to memory location?

---

The "cause" is just that -- an invalid access to a memory location. Why read any more into it than what the error is stating?

Quote:

---

On a slightly higher level, normal debug techniques are not really going to help here. Does anyone know of 3rd party program analysis software that might be of help? If the problem is a multi-threading sync issue, again are there 3rd party analysis software that might pick up these rare intermittent problems?

---

I believe Intel has one.

Regards,

Paul McKenzie

You're right of course! I was hoping it wasn't a sync issue but it's looking more likely. I've dug out an old Windows 2000 single core computer and the program hasn't reported any memory errors - yet! So it looks like an issue with multi-threading running on multi-cores - oh dear.

This program is several years old and has been amended, enhanced and 'maintained' by a various collection of dubious 'competent' programmers - non of which are now around. This mess has been dropped into my lap (its not of my making) as I've been programming for over 30 years, no-one else will touch it and I didn't offer a quick enough excuse - don't blame the others really. The program is really just c using WIN32 APIs without a class in sight! There are 20 structures dynamically allocated with pointers linking various parts of various structures, together with 40 global variables with a few thousand lines of code spread over 12 source files. Threads are started almost at a whim with threads freeing memory allocated by other threads and then allocating their own memory and relying upon another thread to free it somewhere somehow. I'm going to rewrite it from the ground up using classes and put all the memory management into the classes where it belongs.

Sorry for the rant but you might feel the same if this mess was dropped in your lap.:cry: Well at least I know what I'm doing for the next few weeks!!

Quote:

---

Originally Posted by 2kaud

I'm going to rewrite it from the ground up using classes and put all the memory management into the classes where it belongs.

---

If you are willing to change to C++ and not stick with 'C', I would start out with the small things first. For example, if they coded their own linked list or string handling, replacing those with standard C++ classes that handle these issues. Dynamically allocated pointers usually can be changed to some sort of smart pointer, i.e. shared_ptr, unique_ptr, etc, so that you don't get involved too much in memory management.

Believe it or not, it isn't that difficult to change such a program -- just change one or two definitions to C++ classes, compile the app, fix the expected compile errors, test. That's how I've approached changing large, procedural C++ programs that are coded in a C-like manner (or just straight 'C' programs into C++) into one that is maintainable. In my experience, the threading issues seems to work itself out naturally once the underlying infrastructure is built on a more stable and maintainable foundation. You may find you need less critical sections once the globals or static variables are removed.

Another thing to consider (if you switch to C++) is using RAII to control your critical sections, i.e. you create a class that always unlocks the critical section on destruction if the function or code block exits for any reason whatsoever. To do this in 'C' or non-OO C++ is not easy, but by using classes, becomes very simple.

Regards,

Paul McKenzie

Thanks for the advice. Much appreciated. It's definately going to be changed to c++ if I'm to sort it out!!!!!

In addition to what Paul has said, with respect to management try to follow the following guidelines:

Use RAII classes to manage resources (not only memory but all resources).
Using memory managing classes that adhere to the RAII idiom ensures that memory is allocated and deallocated at the correct scope and that memory is not leaked in the event where an exception within the thread is thrown, unwinding the stack past the scope of the memory managing class instance. In addition, if the class is written correctly, scope for double deletion errors, or just generally leaking the memory are removed.

Ensure that a piece of memory is allocated and deallocated by the same thread
Moving ownership of resources between threads (especially memory) is generally never a good idea. It is asking for trouble, because it removes allocation/deallocation synchronicity - and that is bad. Try writing a thread-safe smart pointer and you will find out how hard it is to control the timing of allocation and deallocation between threads on a multicore system! In fact, I'm pretty convinced this second point is the problem with the program you have inherited.

Thanks for your comments and suggestions. Yes, I think your second point is the problem as well - although the program has got a load of other problems besides, now that I've had time to look more closely through the code to try to understand the flow. I have never seen anything like it. I'm doing a complete redesign and c++ coding from the ground up - it's so bad!! I have found this (below). I don't know whether to laugh or cry. I thought you might get a smile from it. Please feel free to pass to other colleagues for a laugh:lol:.

In WinMain function before message loop

Code:

---

//Start heartbeat
_beginthread(StatusCheck, 0, NULL);

---

In WinMain message loop

Code:

---

//Timer for heartbeat
case WM_TIMER:
        if (p_wParam == TIM_STATS) {
                StatusSet();
        }
        break;

---

Code:

---

//Thread for status check for heartbeat
VOID StatusCheck(PVOID p_void)
{
        while (TRUE) {
                EnterCriticalSection(&l_statcheck);
                l_State = TRUE;
                LeaveCriticalSection(&l_statcheck);

                SetTimer(g_hmain, TIM_STATS, 4000, NULL);
                Sleep(6000);

                EnterCriticalSection(&l_statcheck);
                if (l_State == TRUE) {
                        exit(ShowError(ERR_HEARTBEAT));
                }
                LeaveCriticalSection(&l_statcheck);

                /*if (l_State == TRUE) {
                        DisplayMessage("Problem with heart-beat!");
                }*/

                //Check to make sure global variables OK
                if (g_lastone != INT_MAX) {
                        exit(ShowError(ERR_ZERO));
                }

        }
}

//Clear state flag to indicate heartbeat running
void StatusSet(void)
{
        KillTimer(g_hmain, TIM_STATS);

        EnterCriticalSection(&l_statcheck);
        l_State = FALSE;
        LeaveCriticalSection(&l_statcheck);

        return;
}

---

g_lastone is the last global variable to be declared and is assigned at the start of WinMain

Code:

---

g_lastone = INT_MAX;                //Special one to make sure not overwriting global variables

---

I think on balance I'll cry:cry: Is the programmer who wrote this **** certifiably insane??? Why did I agree to have a 'quick' look at this garbage???

Quote:

---

Originally Posted by 2kaud

Thanks for your comments and suggestions. Yes, I think your second point is the problem as well - although the program has got a load of other problems besides, now that I've had time to look more closely through the code to try to understand the flow. I have never seen anything like it. I'm doing a complete redesign and c++ coding from the ground up - it's so bad!! I have found this (below). I don't know whether to laugh or cry. I thought you might get a smile from it. Please feel free to pass to other colleagues for a laugh:lol:.

---

I see the term "heartbeat" in the comments. Is this by any chance, a FIX protocol engine?

Regards,

Paul McKenzie

Someone needs to fire the guy that wrote that.

Oh... I guess they already did. :p

What's this critical section for?

Code:

---

EnterCriticalSection(&l_statcheck);
if (l_State == TRUE) {
        exit(ShowError(ERR_HEARTBEAT));
}
LeaveCriticalSection(&l_statcheck);

---

No variable is being directly being assigned to. A comparison is done, and then a call to exit().

If ShowError for some reason needs to be synchronized, then that is where the critical section would be placed, and not at this level. Otherwise I see no need for the critical section -- this is paranoid programming by someone who didn't understand how and when to use synchronization objects.

Regards,

Paul McKenzie

Quote:

---

Originally Posted by Paul McKenzie

I see the term "heartbeat" in the comments. Is this by any chance, a FIX protocol engine?

---

Even if it is... THe way it's implemented here is... downright silly.
That guy needs to be fired... and flogged.

Quote:

---

Originally Posted by Paul McKenzie

What's this critical section for?

---

but the l_State variable is assigned in the window message pump thread, so unless you can prove that such assignments and comparisons are atomic synchronization is needed here.

Quote:

---

Originally Posted by 2kaud

Thanks for your comments and suggestions. Yes, I think your second point is the problem as well - although the program has got a load of other problems besides, now that I've had time to look more closely through the code to try to understand the flow.

---

Maybe it would be easier to figure out what the ultimate goal of that piece of code is supposed to be instead of trying to understand the flow. The problem with trying to understand the flow of code that is, for all practical purposes with code like this, it leaves you open to miss something subtle (the subtlety occurs only because of the spaghetti-like logic that was used).

I recently had to change code that was so badly written, I had to redo the entire module. I wasn't going to waste time trying to figure out all the twists and turns, when I already knew what the code was supposed to accomplish.

Regards,

Paul McKenzie

Quote:

---

Originally Posted by superbonzo

but the l_State variable is assigned in the window message pump thread, so unless you can prove that such assignments and comparisons are atomic synchronization is needed here.

---

Correct.
Although there are better ways to achieve the same effect.

If you assume the VC compiler (and some others) then marking the bool as volatile and making sure it's properly aligned would be sufficient.
if you can't assume a particular brand of compiler and a particular way it handles volatile (the spec guarantees nothing) you need explicit synchronisation. Even in that case, there's better ways around it.

But the whole idea here of creating a thread and then communicating with the main thread via timer messages makes it obvious that the guy that wrote this was just clueless on what he was actually trying to do.

Yes well, I posted the above code snippets for you all to have a laugh - at the moment I could do with one myself. The guys that wrote it have all long gone and until I started to look in detail at the program no one had a clue it was so badly coded (which says something about management, project control etc etc - further heads are rolling over this). We've now looking at other programs these guys had messed with to get an idea of how big a problem we really have.

I won't try to disect the code snippets much. I'm writing a completely new program to replace this mess from the ground up and this sort of mad programming won't feature. As far as I can tell, this code had two purposes. One was to check that memory wasn't being corrupted - which comes back to my original posting question re error with GlobalFree which started off this whole episode - and second was to make sure that the message loop in the main program wasn't waiting around for processing to complete whilst dealing with a message and hence not making the program responsive to user input/mouse etc. If a message took too long to process then the hearbeat would fail and an error message displayed. Practically every message in the main WndProc function that is processed within the program starts a new thread!!!! As I said before, I've never in all my years programming seen anything like it - and I hope I never will.