Circular buffer writing randomly terminates the program?

Question

"The program has unexpectedly finished."

I have a class that is calling CMem::Write(). And it Displays the iteration to the screen. Sometimes it reaches 140, others... 12, 3, 42, falls out right away... very random.
If I remove the call to CMem::Write(), the program will run forever.

Not sure why it the program termination? All I can assume is that something is not write in CMem::Write() method.

CMem::CMem()//sets up the "static" stack memory and the pointers to it; also the "static" positionIndex
{
    m_nBufferLength = sizeof(char); //short int
    static char *cMessageCB = new char[m_nBufferLength];
    static double *dTimeCB = new double[m_nBufferLength];

    m_cMessageCB = cMessageCB;
    m_dTimeCB = dTimeCB;


    ////////////////////////////////////////
    static char *cMessageReadList = new char[m_nBufferLength]; //max size can be the CB
    static double *dTimeReadList = new double[m_nBufferLength]; //max size can be the CB

    m_cMessageReadList = cMessageReadList;
    m_dTimeReadList = dTimeReadList;

    static int firstInstance = 0;

    if(firstInstance == 0){
        m_posRead = 0;//only on first instance
        m_posWrite = 0;//only on first instance

        firstInstance++;//check to see if multiple threads entered at the same time and look at the count
    }
}


void CMem::Write()
{//double dTime, char cMessage
//only one thread can write at a time... so lock... (make other threads with various random delays)

    static bool bUse = false;
    bool bDone = false;


    while(bDone == false){

        if(bUse == false){
            bUse = true;


            m_cMessageCB[m_posWrite] = m_cMessageWrite;
            m_dTimeCB[m_posWrite] = m_dTimeWrite;

            m_posWrite = (unsigned char)(m_posWrite + 1);


            static char cFlag = 0;
            //if writing position == reading position then flag
            if(m_posWrite == m_posRead){
                cFlag = 1;
            }

            bDone = true;
            bUse = false;
        }else if(bUse == true){
            printf("SUSPEND ");
        }
    }
}


void CMem::Read()
{//get the whole block of memory and increment the m_posRead accordingly
    unsigned char j = 0;

    while( (m_posRead + 1) != (m_posWrite + 1) ){
        m_cMessageReadList[j] = m_cMessageCB[m_posRead];//inc m_posRead at the end
        m_dTimeReadList[j] = m_dTimeCB[m_posRead];//inc m_posRead at the end

        m_posRead = (unsigned char)(m_posRead + 1);//circulate around
        j++;// 'j' is not circulating back around
    }

    //write to file
}

Typically, if you run the codee in a debugger, it will stop where the problem occurs when a program "unexpectedly finished". — Mats Petersson, Aug 13 '13 at 17:32
Does *nobody* post [minimal complete examples](http://sscce.org)? — Beta, Aug 13 '13 at 17:33
You say this is a circular queue, but when do you wrap around? I only ever see the write position being increased in the call to CMem::Write(). You should do something like: `if (m_posWrite == m_nBufferLength) m_posWrite = 0;` — Trenin, Aug 13 '13 at 17:36
You are using a lot of static variables. I would suggest making them member variable instead. In some cases, the static variables will cause problems. For example, are these values common to all classes? Or just this one instance of the CMem? If for the instance, then they should not be static. — Trenin, Aug 13 '13 at 17:38
@Trenin, when you declar a type a unsigned char and you have that value at 255, what happens when you increment it by 1? It circles back around to -1... then you cast it because the operation was done as an integer, and nowthe -1 becomes 0. — jdl, Aug 13 '13 at 17:49
That is a convenience made possible only when your buffer is exactly the size of a single type. Also, you are relying on the compiler to cast -1 to 0, which is not a good idea. What if on another platform, the size of char increases to 2 bytes? Then your buffer is now taking up 65356 entries instead of 256. — Trenin, Aug 13 '13 at 17:58
Actually this is done on purpose... we wanted to avoid the condition statement to speed it up, but what we didn't know initially was that the operation was done as an integer and we had to cast it. — jdl, Aug 13 '13 at 19:08
For the love of everything that is holy, do not use statics in a class that is instantiated from multiple threads unless you know exactly what you are doing. Immutables/consts, maybe. Buffer pointers, absolutely not. — Martin James, Aug 13 '13 at 19:21

BartoszKP · Answer 1 · 2013-08-13T17:48:57.487

0

These lines seem to one of problems with this code:

if(firstInstance == 0){
m_posRead = 0;//only on first instance
m_posWrite = 0;//only on first instance

Why do you initialize indices only on first instance? Other instances will have these members uninitialized so obviously they can corrupt the memory.

EDIT (regarding comment):

OK, you can make them static, however this reveals a serious problem with your design. But it's off-topic here. After you make them static another problem remains: the m_posWrite variable is only incremented and never reset/decremented - how do you expect it not to go out of bounds?

edited Aug 13 '13 at 17:48

answered Aug 13 '13 at 17:37

BartoszKP

34,786
15
102
130

I needed to make them static as well... but problem is still not fixed – jdl Aug 13 '13 at 17:45
I've edited the answer with reply to your comment. Let me know if it's still crashing. – BartoszKP Aug 13 '13 at 17:49
the issue is I have multiple threads calling these methods dumping in data to write across a socket. I am using static so things are kept consistant from thread to thread. Member variables assigned to a non static address will vary on every instantiation of this class within each thread. – jdl Aug 13 '13 at 17:51
'multiple threads', 'static'. Get rid of the statics, immediate. – Martin James Aug 13 '13 at 19:12
@ Martin James, I need to make a relationship between threads, static is the only way that is working. If you have an idea let me know. – jdl Aug 13 '13 at 20:15
You are leaking like a colander every time you instantiate this class. – Martin James Aug 14 '13 at 00:00

score 0 · Accepted Answer · answered Aug 13 '13 at 17:49

With whatever code you have provided, it seems it is a clear case of memory corruption

First

if(firstInstance == 0){
    m_posRead = 0;//only on first instance
    m_posWrite = 0;//only on first instance

    firstInstance++;//check to see if multiple threads entered at the same time and look at the count
}

Above code initializes m_posRead and m_posWrite to 0 only for first instance. FOr all other instances, it is undefined.

Secondly, in the constructor you are doing

m_nBufferLength = sizeof(char); //short int
static char *cMessageCB = new char[m_nBufferLength];
m_cMessageCB = cMessageCB;

Now, this makes m_cMessageCB only 1 byte wide. While, in CMem::Write you are doing

m_cMessageCB[m_posWrite] = m_cMessageWrite;
m_dTimeCB[m_posWrite] = m_dTimeWrite;

m_posWrite = (unsigned char)(m_posWrite + 1);

Here m_posWrite has been incremented. First time it will write on 0th index. Next time you call CMem:Write and it will try to write on 1st index which is "Array Out Of Bound Write" (because m_cMessageCB is only 1 byte wide)and it's behaviour is undefined. It may terminate on the next write or any other future writes.

Circular buffer writing randomly terminates the program?

2 Answers2