Fix heap leak mentioned in #2358

[mverch67]

This is a fix for #2358.

In MeshService::sendToPhone() a copy of a meshPacket is created. This copy is sent into the phoneQueue where it eventually gets deleted when fetched by the phone. The original packet is only released when it is sent over the mesh, which will not happen if DeviceTelemetryModule enters the upper branch in method EnvironmentTelemetrieModule::sendTelemetry():

    lastMeasurementPacket = packetPool.allocCopy(*p); // first copy
    if (phoneOnly) {
        LOG_INFO("Sending packet to phone\n");
        service.sendToPhone(p);                       // another copy
    } else {
        LOG_INFO("Sending packet to mesh\n");
        service.sendToMesh(p, RX_SRC_LOCAL, true);
    }

The fix avoids the copying in sendToPhone(). The copying is better done in sendToMesh(), just before invoking sendToPhone():

    if (ccToPhone) {
        sendToPhone(packetPool.allocCopy(*p));
    }

Note: the relevant code in AirQualityTelemetryModule looks exactly the same as in EnvironmentTelemetryModule, so there should be no issue. However, I was not able to test it due to lack of such equipment.

[github-actions]

🤖 Pull request artifacts

file	commit
pr2359-firmware-2.1.3.df029f5.zip	df029f5

[thebentern]

Great catch! AirQualityTelemetry was based on this module as well, so it will need to be patched

[mverch67]

Unfortunately I overlooked one invocation of sendToPhone() in line 85 where it should now also read
sendToPhone(packetPool.allocCopy(*mp));

I just added this in an additional commit to this PR.

[mverch67]

Great catch! AirQualityTelemetry was based on this module as well, so it will need to be patched

There is no need to change AirQualityTelemetry because I decided to fix this issue in sentToPhone() which leaked the memory. Otherwise, if a fix had been done only in DeviceTelemetry (and AirQualityTelemetry) the interface of MeshService would look very awkward because sendToMesh() would have to be called with a copied package (because it deletes it) while sendToPhone() had to be called without a copy (because it did do a copy for itself). Now, with the change in MeshService the interfaces are more clean. It may have other side-effects, though (e.g. SimRadio which also calls sendToPhone() directly).

[thebentern]

There is no need to change AirQualityTelemetry because I decided to fix this issue in sentToPhone() which leaked the memory. Otherwise, if a fix had been done only in DeviceTelemetry (and AirQualityTelemetry) the interface of MeshService would look very awkward because sendToMesh() would have to be called with a copied package (because it deletes it) while sendToPhone() had to be called without a copy (because it did do a copy for itself). Now, with the change in MeshService the interfaces are more clean. It may have other side-effects, though (e.g. SimRadio which also calls sendToPhone() directly).

I wasn't paying enough attention to your original change. Makes sense. Looks good to me

[GUVWAF]

Great catch indeed!

It may have other side-effects, though (e.g. SimRadio which also calls sendToPhone() directly).

I wouldn’t worry too much about SimRadio. This is only used for simulating the radio on a native Linux system, so it will not be used for long consecutive periods and only on more powerful machines. I quickly checked it with the interactive simulator and this at least doesn’t break it :)