Closed BadWolf42 closed 11 months ago
Thanks for pointing that out, any idea @h2zero ?
I just re-tested with v0.9.14 to be sure.
Actually I was wrong.
I did not test in this order and can tel that each time gateway_name
is set, base topic becomes <gateway_name>/
.
So (Each time on a fresh install with base topic set to home/OpenMQTTGateway/
):
{"mqtt_topic":"bt/"}
on home/OpenMQTTGateway/commands/MQTTtoSYS/config
Base topic becomes bt/OpenMQTTGateway/
-> OKDebug log:
N: [ MQTT->OMG ]: {"mqtt_topic":"bt/"}
{
"mqtt_server": "192.168.11.3",
"mqtt_port": "1883",
"mqtt_user": "",
"mqtt_pass": "",
"mqtt_topic": "bt/",
"gateway_name": "OpenMQTTGateway",
"mqtt_broker_secure": false,
"mqtt_broker_cert": "",
"mqtt_ss_index": 0,
"ota_server_cert": ""
}
{"gateway_name":"OMG"}
on home/OpenMQTTGateway/commands/MQTTtoSYS/config
Base topic becomes OMG/
-> KODebug log:
N: [ MQTT->OMG ]: {"gateway_name":"OMG"}
{
"mqtt_server": "192.168.11.3",
"mqtt_port": "1883",
"mqtt_user": "",
"mqtt_pass": "",
"mqtt_topic": "",
"gateway_name": "OMG",
"mqtt_broker_secure": false,
"mqtt_broker_cert": "",
"mqtt_ss_index": 0,
"ota_server_cert": ""
}
{"mqtt_topic":"bt/","gateway_name":"OMG"}
on home/OpenMQTTGateway/commands/MQTTtoSYS/config
Base topic becomes OMG/
-> KODebug log:
N: [ MQTT->OMG ]: {"mqtt_topic":"bt/","gateway_name":"OMG"}
{
"mqtt_server": "192.168.11.3",
"mqtt_port": "1883",
"mqtt_user": "",
"mqtt_pass": "",
"mqtt_topic": "",
"gateway_name": "OMG",
"mqtt_broker_secure": false,
"mqtt_broker_cert": "",
"mqtt_ss_index": 0,
"ota_server_cert": ""
}
Owwww got it !
gateway_name
char* is statically declared with a size of parameters_size
here:
https://github.com/1technophile/OpenMQTTGateway/blob/c04c62916fceb5dbb7d834b035d353af178d7492/main/main.ino#L171
But strncpy
uses a max length of parameters_size * 2
to copy gateway_name
here:
https://github.com/1technophile/OpenMQTTGateway/blob/c04c62916fceb5dbb7d834b035d353af178d7492/main/main.ino#L2075
As gateway_name
is declared next to mqtt_topic
, I supposed that mqtt_topic
was overwritten by strncpy (as it fills the end of dest char* with \0
).
So I tested with a 80 char long gateway_name
and as I suspected this happened:
N: [ MQTT->OMG ]: {"gateway_name":"01234567890123456789012345678901234567890123456789012345678901234567890123456789","mqtt_topic":"bt/"}
{
"mqtt_server": "192.168.11.3",
"mqtt_port": "1883",
"mqtt_user": "",
"mqtt_pass": "",
"mqtt_topic": "01234567890123456789",
"gateway_name": "01234567890123456789012345678901234567890123456789012345678901234567890123456789",
"mqtt_broker_secure": false,
"mqtt_broker_cert": "",
"mqtt_ss_index": 0,
"ota_server_cert": ""
}W: MQTT connection...
N: Connected to broker
Stack smashing protect failure!
abort() was called at PC 0x40190827 on core 1
ELF file SHA256: 0000000000000000
Backtrace: 0x4008fac4:0x3ffcdd60 0x4008fd41:0x3ffcdd80 0x40190827:0x3ffcdda0 0x400d412a:0x3ffcddc0 0x400dac3d:0x3ffcded0 0x401068a4:0x3ffcdef0 0x40090d52:0x3ffcdf10
Rebooting...
I'm recompiling OMG without *2
to test.
BTW, if source char is the same size as max len strncpy
does not terminate the dest char with \0
, so all static allocation should be at least len+1 and last char forced to \0
.
Bad
Thanks for pointing that out, any idea @h2zero ?
Bad looks to have sorted it out 😄
Hello Guys,
I had quite a struggle with PlatformIO... but anyway:
As I suspected, gw & topic change is working without the *2
, but still some overflow with long names:
N: [ MQTT->OMG ]: {"gateway_name":"omg34567890123456789012345678901234567890123456789012345678901234567890123456789","mqtt_topic":"home/"}
{
"mqtt_server": "192.168.11.3",
"mqtt_port": "1883",
"mqtt_user": "your_username",
"mqtt_pass": "your_password",
"mqtt_topic": "home/",
"gateway_name": "omg345678901234567890123456789012345678901234567890123456789home/",
"mqtt_broker_secure": false,
"mqtt_broker_cert": "",
"mqtt_ss_index": 0,
"ota_server_cert": ""
}
There are a lot of unsafe strcpy/strcat/strcmp especially regarding mqtt_topic
& gateway_name
...
But I will suggest a PR to fix explicitly this issue first by:
mqtt_topic
& gateway_name
to parameters_size
instead of mqtt_topic_max_size
, andmqtt_topic
& gateway_name
with a size of parameters_size + 1
,mqtt_topic_max_size
from 100
to 150
for common platforms (ESP8266
, ESP32
, __AVR_ATmega2560__
& __AVR_ATmega1280__
) and from 50
to 75
for others (even if max size for a MQTT topic is 65536 bytes).
So that at least <max mqtt_topic><max gateway_name>/commands/MQTTtoSYS/config
could fit in it.With that even with a very large mqtt_topic
& gateway_name
, topic can still be redefined:
N: [ MQTT->OMG ]: {"gateway_name":"omg34567890123456789012345678901234567890123456789012345678901234567890123456789","mqtt_topic":"01234567890123456789012345678901234567890123456789012345678/"}
{
"mqtt_server": "192.168.11.3",
"mqtt_port": "1883",
"mqtt_user": "your_username",
"mqtt_pass": "your_password",
"mqtt_topic": "01234567890123456789012345678901234567890123456789012345678/",
"gateway_name": "omg345678901234567890123456789012345678901234567890123456789",
"mqtt_broker_secure": false,
"mqtt_broker_cert": "",
"mqtt_ss_index": 0,
"ota_server_cert": ""
}
And is correctly truncated:
N: [ MQTT->OMG ]: {"gateway_name":"omg345678901234567890123456789012345678901234567890123456789012345678901234567890123456789","mqtt_topic":"0123456789012345678901234567890123456789012345678901234567890123456789/"}
{
"mqtt_server": "192.168.11.3",
"mqtt_port": "1883",
"mqtt_user": "your_username",
"mqtt_pass": "your_password",
"mqtt_topic": "012345678901234567890123456789012345678901234567890123456789",
"gateway_name": "omg345678901234567890123456789012345678901234567890123456789",
"mqtt_broker_secure": false,
"mqtt_broker_cert": "",
"mqtt_ss_index": 0,
"ota_server_cert": ""
}
Are you ok with that, or should I address at the same time unsafe strcpy/strcat/strcmp ?
I would also suggest to reduce parameters_size
, or (m)allocate it on the heap, or simply use C++ String.
Bad
Hello,
PR #1242 has been merged and integrated in v0.9.15.
It handles almost all points:
mqtt_topic
& gateway_name
to parameters_size
instead of mqtt_topic_max_size
,mqtt_topic
& gateway_name
with a size of parameters_size + 1
,mqtt_topic_max_size
from 100
to 150
for common platforms and from 50
to 75
for others,But not:
mqtt_topic
& gateway_name
,mqtt_topic
& gateway_name
by (m)allocating it on the heap, or using C++ String.Those points are minor from my perspective. So, should we close this issue or do you want me to address them in a new PR?
Bad
Hello,
@h2zero what are your thoughts here?
This issue is stale because it has been open for 30 days with no activity.
This issue was closed because it has been inactive for 14 days since being marked as stale.
Describe the bug Using
home/OpenMQTTGateway/commands/MQTTtoSYS/config
topic : Ifmqtt_topic
andgateway_name
are changed at the same time, then onlygateway_name
is set, Resulting in base topic<gateway_name>/
, instead of<mqtt_topic>/<gateway_name>/
.To Reproduce Each time on a fresh install with base topic set to
home/OpenMQTTGateway/
:Publish
{"mqtt_topic":"bt/"}
onhome/OpenMQTTGateway/commands/MQTTtoSYS/config
Base topic becomesbt/OpenMQTTGateway/
-> OKPublish
{"gateway_name":"OMG"}
onhome/OpenMQTTGateway/commands/MQTTtoSYS/config
Base topic becomeshome/OMG/
-> OKPublish
{"mqtt_topic":"bt/","gateway_name":"OMG"}
onhome/OpenMQTTGateway/commands/MQTTtoSYS/config
Base topic becomesOMG/
-> KOExpected behavior When publishing
{"mqtt_topic":"bt/","gateway_name":"OMG"}
onhome/OpenMQTTGateway/commands/MQTTtoSYS/config
Base topic should becomesbt/OMG/
Environment (please complete the following information): OpenMQTTGateway version used V0.9.13, esp32-lolin32lite-ble on ESP32
Checks: Code responsible of this feature is really straightforward PR #1053: https://github.com/1technophile/OpenMQTTGateway/blob/v0.9.13/main/main.ino#L2070-L2080
I don't understand why only 1 parameter works at the time. Maybe
stopProcessing()
is required beforesaveMqttConfig()
andstartProcessing()
afterclient.disconnect()
...