Skip to content

Commit

Permalink
Bugfix/v2.0 crash switch (letscontrolit#682)
Browse files Browse the repository at this point in the history
* [crashes] Added constructors to initialize all members in structs

Numerous structs are defined, but none of them have default constructors and there is no guarantee the members will be set when used. 
With these default constructors, the parameters at least have an initialized value.

* [PubSubClient] Add bound checks on the internal buffer

Not sure if this was really causing an issue, but proper bound checks are always a good thing.

* [Crash Switch] Disabled delayBackground and added yield() calls

Something really fishy is going on with the delayBackground function, which will result in crashes when pressing the switch multiple times, with Domoticz MQTT enabled as first controller.
Disabled for now and delay(1) added to give background tasks a chance to do their work and make sure the watchdog doesn't perform a reset.

* [CI build errors] Commented out some unused variables

Travis considers them as error and fails the checks.

* [CI check] Out-of-bounds check fix
  • Loading branch information
TD-er committed Jan 10, 2018
1 parent 05a94df commit 9b8f940
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 3 deletions.
4 changes: 2 additions & 2 deletions lib/pubsubclient/src/PubSubClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ uint16_t PubSubClient::readPacket(uint8_t* lengthLength) {
buffer[len++] = digit;
length += (digit & 127) * multiplier;
multiplier *= 128;
} while ((digit & 128) != 0);
} while ((digit & 128) != 0 && len < (MQTT_MAX_PACKET_SIZE -2));
*lengthLength = len-1;

if (isPublish) {
Expand Down Expand Up @@ -525,7 +525,7 @@ uint16_t PubSubClient::writeString(const char* string, uint8_t* buf, uint16_t po
const char* idp = string;
uint16_t i = 0;
pos += 2;
while (*idp) {
while (*idp && pos < (MQTT_MAX_PACKET_SIZE - 2)) {
buf[pos++] = *idp++;
i++;
}
Expand Down
9 changes: 8 additions & 1 deletion src/Controller.ino
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ void sendData(struct EventStruct *event)
// if (!Settings.TaskDeviceSendData[event->TaskIndex])
// return false;

delay(1); // Make sure the yield() is called even when flooded with messages.
/*
if (Settings.MessageDelay != 0)
{
const long dif = timePassedSince(lastSend);
Expand All @@ -24,13 +26,18 @@ void sendData(struct EventStruct *event)
uint16_t delayms = Settings.MessageDelay - dif;
//this is logged nowhere else, so might as well disable it here also:
// addLog(LOG_LEVEL_DEBUG_MORE, String(F("CTRL : Message delay (ms): "))+delayms);
delayBackground(delayms);
// Something really fishy is going on with the delayBackground function,
// which will result in crashes.
// Disabled for now and delay(1) added outside this if-scope.
// delayBackground(delayms);
// unsigned long timer = millis() + delayms;
// while (!timeOutReached(timer))
// backgroundtasks();
}
}
*/

LoadTaskSettings(event->TaskIndex); // could have changed during background tasks.

Expand Down
81 changes: 81 additions & 0 deletions src/ESPEasy.ino
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,47 @@ struct SecurityStruct

struct SettingsStruct
{
SettingsStruct() :
PID(0), Version(0), Build(0), IP_Octet(0), Unit(0), Delay(0),
Pin_i2c_sda(-1), Pin_i2c_scl(-1), Pin_status_led(-1), Pin_sd_cs(-1),
UDPPort(0), SyslogLevel(0), SerialLogLevel(0), WebLogLevel(0), SDLogLevel(0),
BaudRate(0), MessageDelay(0), deepSleep(0),
CustomCSS(false), DST(false), WDI2CAddress(0),
UseRules(false), UseSerial(false), UseSSDP(false), UseNTP(false),
WireClockStretchLimit(0), GlobalSync(false), ConnectionFailuresThreshold(0),
TimeZone(0), MQTTRetainFlag(false), InitSPI(false),
Pin_status_led_Inversed(false), deepSleepOnFail(false), UseValueLogger(false)
{
for (byte i = 0; i < CONTROLLER_MAX; ++i) {
Protocol[i] = 0;
ControllerEnabled[i] = false;
for (byte task = 0; task < TASKS_MAX; ++task) {
TaskDeviceID[i][task] = 0;
TaskDeviceSendData[i][task] = false;
}
}
for (byte task = 0; task < TASKS_MAX; ++task) {
TaskDeviceNumber[task] = 0;
OLD_TaskDeviceID[task] = 0;
TaskDevicePin1PullUp[task] = false;
for (byte cv = 0; cv < PLUGIN_CONFIGVAR_MAX; ++cv) {
TaskDevicePluginConfig[task][cv] = 0;
}
TaskDevicePin1Inversed[task] = false;
for (byte cv = 0; cv < PLUGIN_CONFIGFLOATVAR_MAX; ++cv) {
TaskDevicePluginConfigFloat[task][cv] = 0.0;
}
for (byte cv = 0; cv < PLUGIN_CONFIGLONGVAR_MAX; ++cv) {
TaskDevicePluginConfigLong[task][cv] = 0;
}
OLD_TaskDeviceSendData[task] = false;
TaskDeviceGlobalSync[task] = false;
TaskDeviceDataFeed[task] = 0;
TaskDeviceTimer[task] = 0;
TaskDeviceEnabled[task] = false;
}
}

unsigned long PID;
int Version;
int16_t Build;
Expand Down Expand Up @@ -500,6 +541,7 @@ struct SettingsStruct

struct ControllerSettingsStruct
{
ControllerSettingsStruct() : UseDNS(false), Port(0) {}
boolean UseDNS;
byte IP[4];
unsigned int Port;
Expand Down Expand Up @@ -563,6 +605,21 @@ struct NotificationSettingsStruct

struct ExtraTaskSettingsStruct
{
ExtraTaskSettingsStruct() : TaskIndex(0) {
TaskDeviceName[0] = 0;
for (byte i = 0; i < VARS_PER_TASK; ++i) {
for (byte j = 0; j < 41; ++j) {
TaskDeviceFormula[i][j] = 0;
TaskDeviceValueNames[i][j] = 0;
TaskDeviceValueDecimals[i] = 0;
}
}
for (byte i = 0; i < PLUGIN_EXTRACONFIGVAR_MAX; ++i) {
TaskDevicePluginConfigLong[i] = 0;
TaskDevicePluginConfig[i] = 0;
}
}

byte TaskIndex;
char TaskDeviceName[41];
char TaskDeviceFormula[VARS_PER_TASK][41];
Expand All @@ -574,6 +631,10 @@ struct ExtraTaskSettingsStruct

struct EventStruct
{
EventStruct() :
Source(0), TaskIndex(0), ControllerIndex(0), ProtocolIndex(0), NotificationIndex(0),
BaseVarIndex(0), idx(0), sensorType(0), Par1(0), Par2(0), Par3(0), Par4(0), Par5(0),
OriginTaskIndex(0), Data(NULL) {}
byte Source;
byte TaskIndex; // index position in TaskSettings array, 0-11
byte ControllerIndex; // index position in Settings.Controller, 0-3
Expand All @@ -598,13 +659,19 @@ struct EventStruct

struct LogStruct
{
LogStruct() : timeStamp(0), Message(NULL) {}
unsigned long timeStamp;
char* Message;
} Logging[10];
int logcount = -1;

struct DeviceStruct
{
DeviceStruct() :
Number(0), Type(0), VType(0), Ports(0),
PullUpOption(false), InverseLogicOption(false), FormulaOption(false),
ValueCount(0), Custom(false), SendDataOption(false), GlobalSyncOption(false),
TimerOption(false), TimerOptional(false), DecimalsOnly(false) {}
byte Number;
byte Type;
byte VType;
Expand All @@ -623,6 +690,9 @@ struct DeviceStruct

struct ProtocolStruct
{
ProtocolStruct() :
Number(0), usesMQTT(false), usesAccount(false), usesPassword(false),
defaultPort(0), usesTemplate(false), usesID(false) {}
byte Number;
boolean usesMQTT;
boolean usesAccount;
Expand All @@ -634,13 +704,20 @@ struct ProtocolStruct

struct NotificationStruct
{
NotificationStruct() :
Number(0), usesMessaging(false), usesGPIO(0) {}
byte Number;
boolean usesMessaging;
byte usesGPIO;
} Notification[NPLUGIN_MAX];

struct NodeStruct
{
NodeStruct() :
age(0), build(0), nodeName(NULL), nodeType(0)
{
for (byte i = 0; i < 4; ++i) ip[i] = 0;
}
byte ip[4];
byte age;
uint16_t build;
Expand All @@ -650,6 +727,9 @@ struct NodeStruct

struct systemTimerStruct
{
systemTimerStruct() :
timer(0), plugin(0), Par1(0), Par2(0), Par3(0) {}

unsigned long timer;
byte plugin;
byte Par1;
Expand All @@ -659,6 +739,7 @@ struct systemTimerStruct

struct systemCMDTimerStruct
{
systemCMDTimerStruct() : timer(0) {}
unsigned long timer;
String action;
} systemCMDTimers[SYSTEM_CMD_TIMER_MAX];
Expand Down

0 comments on commit 9b8f940

Please sign in to comment.