freing _frame too early (writeMultipleCoils sets wrong values)

[ESP-O-MAT]

In this function for example _frame is freed and after that frame[6+i] is read. But frame actually points to _frame: void Modbus::writeMultipleCoils(byte* frame,word startreg, word numoutputs, byte bytecount)

See code changes below:

Original code:

//Clean frame buffer free(_frame); _len = 5; _frame = (byte *) malloc(_len); if (!_frame) { this->exceptionResponse(MB_FC_WRITE_COILS, MB_EX_SLAVE_FAILURE); return; }

_frame[0] = MB_FC_WRITE_COILS; _frame[1] = startreg >> 8; _frame[2] = startreg & 0x00FF; _frame[3] = numoutputs >> 8; _frame[4] = numoutputs & 0x00FF;

byte bitn = 0;
word totoutputs = numoutputs;
word i;
while (numoutputs--) {
    i = (totoutputs - numoutputs) / 8;
    this->Coil(startreg, bitRead(frame[6+i], bitn));
    //increment the bit index
    bitn++;
    if (bitn == 8) bitn = 0;
    //increment the register
    startreg++;
}

Changed code:

byte bitn = 0;
word totoutputs = numoutputs;
word i;
word tempNumoutputs = numoutputs;
word tempStartreg = startreg;
while (tempNumoutputs) {
    i = (totoutputs - tempNumoutputs) / 8;
    this->Coil(tempStartreg, bitRead(frame[6+i], bitn));
    //increment the bit index
    bitn++;
    if (bitn == 8) bitn = 0;
    //increment the register
    tempStartreg++;
    tempNumoutputs--;
}

//Clean frame buffer
**free(_frame);**
_len = 5;
_frame = (byte *) malloc(_len);
if (!_frame) {
    this->exceptionResponse(MB_FC_WRITE_COILS, MB_EX_SLAVE_FAILURE);
    return;
}

_frame[0] = MB_FC_WRITE_COILS;
_frame[1] = startreg >> 8;
_frame[2] = startreg & 0x00FF;
_frame[3] = numoutputs >> 8;
_frame[4] = numoutputs & 0x00FF;

See issue https://github.com/andresarmento/modbus-arduino/issues/35 as well.

[volkerdh]

You are right, this is a severe issue if your system (e.g. esp32) is capable of running multiple processes and uses dynamic memory more than a small embedded with no other processes running. That may be the reason why not all users do have reported problems. I have got the same problems with this issue and needed to change the code. But your way to change it is in my eyes not the best way to correct this issue. By placing the whole response section behind the register writing processes (btw. this issue is also present to write multiple coils) you are not acting according to Modbus error compliance: When the malloc() function in allocating the frame for the answer does give an error back you will answer the function call from the Modbus master with an error code although you have changed the registers successfully according to the write function call. To avoid this scenario there is no other way than using a second frame buffer for input. So I defined an "_iframe" in Modbus.h: protected: byte *_frame; byte *_iframe; byte _len; byte _reply; void receivePDU(byte* frame); Then I'm using this frame for the receivePDU() function in "ModbusIP.cpp": ` if (_MBAP[2] !=0 || _MBAP[3] !=0) return; //Not a MODBUSIP packet if (_len > MODBUSIP_MAXFRAME) return; //Length is over MODBUSIP_MAXFRAME

        _iframe = (byte*) malloc(_len);
        i = 0;
        while (client.available()){
            _iframe[i] = client.read();
            i++;
            if (i==_len) break;
        }

        this->receivePDU(_iframe);
        free(_iframe);
        if (_reply != MB_REPLY_OFF) {

` With this construction, you are freeing the _iframe at the right point: after finishing the call to receivePDU(). This means you need to remove all the early free(_frame) lines in Modbus.ccp and leave just the single one at the end of "void ModbusIP::task()" after sending the response in ModbusCPP. Please search for all of the "free(_frame)" in Modbus.cpp and comment them ALL! In my eyes, Modbus.cpp should not deal with freeing the output frame buffer but the piece of software which is sending (ModbusIP.cpp). I know that this way of solving the problem does require additional dynamic buffer memory. But to me, it seems to be the only clear and compliant way of solving the issue.