Wróciłem ostatnio do jednego ze swoich starych projektów, zrealizowanych
w oparciu o PIC32. Chciałem dodać do niego jedną dość ważną
funkcjonalność - możliwość wrzucania danych za pomocą MQTT.
Udało mi się znaleźć bibliotekę napisaną w oparciu o starzy stos
Microchipa (biblioteki MLA), który użyłem w swoim projekcie. Zależało mi
na tym, bo nie chciałem przenosić całego kodu na nowsze biblioteki Harmony.

https://github.com/dominicusplatus/mqttx

Potrzeba było trochę eksperymentów, jednak w końcu udało mi się
uruchomić bibliotekę. Dość szybko okazało się, że daleko jej do ideału.
Działa całkiem nieźle, jeśli serwer ma wyłączoną autoryzację, albo w
pakiecie CONNECT wysyłamy prawidłowy login i hasło.
Problemy zaczynają się w momencie, gdy próbujemy podać nieprawidłowe
dane do logowania. Sprawdziłem komunikację Wiresharkiem - serwer
odpowiada prawidłowo, jednak klient zupełnie to ignoruje, przystępując
do wysyłania komunikatu. Po paru podejściach powoduje to wykrzaczenie
programu i reset mikrokontrolera.

Przyjrzałem się bliżej kodowi i znalazłem przyczynę. Podczas parsowania
pakietu CONNACK wywoływana jest funkcja MQTTReadPacket(), która pobiera
dane z socketa do bufora MQTTBuffer. Potem jednak kod sprawdza stan stan
bajtu rxBF[1], gdzie spodziewa się znaleźć Return Code i na jego
podstawie podjąć decyzję do co dalszego działania.

Problem polega na tym, że w obecnej wersji kodu tablica rxBF nie jest
nigdzie wykorzystywana. Nie trafiają do niej żadne dane (a więc również
i pakiet CONNACK) i cały czas znajdują się w niej same zera. Program
interpretuje więc odczytaną wartość jako Connection Accepted i
przechodzi do dalszych czynności.

Chciałem się bliżej przyjrzeć funkcji MQTTReadPacket() i tutaj trafiłem
do króliczej nory. ;)

Okazuje się, że została ona zdefiniowana i zdeklarowana z pustą listą
parametrów (bez void), podobnie jak to się robi z bezparametrowymi
funkcjami w C++. Widzę jednak, że w paru miejscach w kodzie funkcja
przyjmuje parametr w postaci wskaźnika na BYTE, przykładowo:

BYTE llen;
WORD len= MQTTReadPacket(&llen);

Potem zawartość takiej zmiennej jest wykorzystywana w kodzie jako
element indeksu tablicy MQTTBuffer - również w tych częściach kodu,
które działały prawidłowo. Szybkie poszukiwania w internecie ujawniły,
że możliwość zdeklarowania pustej listy argumentów to historyczna
zaszłość. Wszyscy przestrzegają przed robieniem tego. Natomiast nigdzie
nie mogę znaleźć informacji o tym, w jaki sposób to działa i co
właściwie robią te kawałki kodu. Ktoś ma jakiś pomysł?

do góry

On 08/08/2022 19:14, Atlantis wrote:
> Natomiast nigdzie
> nie mogę znaleźć informacji o tym, w jaki sposób to działa i co
> właściwie robią te kawałki kodu. Ktoś ma jakiś pomysł?

Podziel się większym kontekstem tego kodu.

do góry

Atlantis <m...@w...pl> wrote:
> Wróciłem ostatnio do jednego ze swoich starych projektów, zrealizowanych
> w oparciu o PIC32. Chciałem dodać do niego jedną dość ważną
> funkcjonalność - możliwość wrzucania danych za pomocą MQTT.
> Udało mi się znaleźć bibliotekę napisaną w oparciu o starzy stos
> Microchipa (biblioteki MLA), który użyłem w swoim projekcie. Zależało mi
> na tym, bo nie chciałem przenosić całego kodu na nowsze biblioteki Harmony.
>
> https://github.com/dominicusplatus/mqttx
>
> Potrzeba było trochę eksperymentów, jednak w końcu udało mi się
> uruchomić bibliotekę. Dość szybko okazało się, że daleko jej do ideału.
> Działa całkiem nieźle, jeśli serwer ma wyłączoną autoryzację, albo w
> pakiecie CONNECT wysyłamy prawidłowy login i hasło.
> Problemy zaczynają się w momencie, gdy próbujemy podać nieprawidłowe
> dane do logowania. Sprawdziłem komunikację Wiresharkiem - serwer
> odpowiada prawidłowo, jednak klient zupełnie to ignoruje, przystępując
> do wysyłania komunikatu. Po paru podejściach powoduje to wykrzaczenie
> programu i reset mikrokontrolera.
>
> Przyjrzałem się bliżej kodowi i znalazłem przyczynę. Podczas parsowania
> pakietu CONNACK wywoływana jest funkcja MQTTReadPacket(), która pobiera
> dane z socketa do bufora MQTTBuffer. Potem jednak kod sprawdza stan stan
> bajtu rxBF[1], gdzie spodziewa się znaleźć Return Code i na jego
> podstawie podjąć decyzję do co dalszego działania.
>
> Problem polega na tym, że w obecnej wersji kodu tablica rxBF nie jest
> nigdzie wykorzystywana. Nie trafiają do niej żadne dane (a więc również
> i pakiet CONNACK) i cały czas znajdują się w niej same zera. Program
> interpretuje więc odczytaną wartość jako Connection Accepted i
> przechodzi do dalszych czynności.
>
> Chciałem się bliżej przyjrzeć funkcji MQTTReadPacket() i tutaj trafiłem
> do króliczej nory. ;)
>
> Okazuje się, że została ona zdefiniowana i zdeklarowana z pustą listą
> parametrów (bez void), podobnie jak to się robi z bezparametrowymi
> funkcjami w C++. Widzę jednak, że w paru miejscach w kodzie funkcja
> przyjmuje parametr w postaci wskaźnika na BYTE, przykładowo:
>
> BYTE llen;
> WORD len= MQTTReadPacket(&llen);
>
> Potem zawartość takiej zmiennej jest wykorzystywana w kodzie jako
> element indeksu tablicy MQTTBuffer - również w tych częściach kodu,
> które działały prawidłowo. Szybkie poszukiwania w internecie ujawniły,
> że możliwość zdeklarowania pustej listy argumentów to historyczna
> zaszłość. Wszyscy przestrzegają przed robieniem tego. Natomiast nigdzie
> nie mogę znaleźć informacji o tym, w jaki sposób to działa i co
> właściwie robią te kawałki kodu. Ktoś ma jakiś pomysł?
>

Nie możesz po prostu poprawić tego kodu.
Ja, implementujące mqtt, po prostu powyginalem jakaś niedorobioną
bibliotekę i będzie git. Przynajmniej w zakresie w jakim tego
potrzebowalem.

jp

do góry

On 08.08.2022 21:00, heby wrote:

> Podziel się większym kontekstem tego kodu.

W poprzedniej wiadomości był link do repozytorium na GitHubie. Tam jest
całość.

do góry

On 08.08.2022 19:14, Atlantis wrote:
[...]
> BYTE llen;
> WORD len= MQTTReadPacket(&llen);
>
Ewidentny błąd - pokazuje dlaczego należy kompilować z -Wall (oraz
ewentualnie -pedantic) i nie ignorować ostrzeżeń. Aczkolwiek w
przytoczonym kontekście nie ma znaczenia - zmienna llen ma zasięg
lokalny ograniczony do wnętrza if-a i poza wywołaniem MQTTReadPacket()
nigdzie nie jest tam później używana.

> Potem zawartość takiej zmiennej jest wykorzystywana w kodzie jako
> element indeksu tablicy MQTTBuffer - również w tych częściach kodu,
> które działały prawidłowo. Szybkie poszukiwania w internecie ujawniły,
> że możliwość zdeklarowania pustej listy argumentów to historyczna
> zaszłość. Wszyscy przestrzegają przed robieniem tego. Natomiast nigdzie
> nie mogę znaleźć informacji o tym, w jaki sposób to działa i co
> właściwie robią te kawałki kodu. Ktoś ma jakiś pomysł?
Nie jestem pewny co oznaczają ,,te kawałki kodu", ale w ramach testu
proponuję odnaleźć ten kontekst:

if(MQTTAvailable()) {
BYTE llen;
WORD len = MQTTReadPacket(&llen);
WORD msgId = 0;
BYTE *payload;

i zaraz po deklaracjach zmiennych dopisać llen = len.

No i proponuję też zamienić
switch(rxBF[1]) { //MQTTBuffer
na
switch(MQTTBuffer[1]) { //MQTTBuffer

do góry

On 09.08.2022 07:13, JDX wrote:

> Ewidentny błąd - pokazuje dlaczego należy kompilować z -Wall (oraz
> ewentualnie -pedantic) i nie ignorować ostrzeżeń. Aczkolwiek w

Przy próbie kompilacji z -Wall kompilator (xc32-gcc) wywala kilka błędów
w bibliotece GCC, ale dotyczą one takich rzeczy jak niewykorzystane
zmienne albo niejawne rzutowanie z char* na BYTE* w argumencie funkcji.
Do tej konkretnej konstrukcji z pustą listą parametrów się akurat nie
czepia.


> przytoczonym kontekście nie ma znaczenia - zmienna llen ma zasięg
> lokalny ograniczony do wnętrza if-a i poza wywołaniem MQTTReadPacket()
> nigdzie nie jest tam później używana.

Chodziło mi o inne miejsce - od linii 787. Tam zmienna llen jest
przekazywana do funkcji MQTTReadPacket w taki sam sposób, a potem bierze
udział w wyliczaniu indeksów do MQTTBuffer.
Chociaż z drugiej strony wszystkie z tych operacji to sumowanie. Zmienna
llen jest zmienną lokalną, a wiec jest domyślnie inicjalizowana
wartością 0. Jeśli przekazanie jej przez wskaźnik do funkcji o pustej
liście parametrów nie ma żadnego wpływu na jej wartość, to nie będzie
też miało na późniejsze wyliczenia.
Czyżby pozostałość po jakichś wcześniejszych wersjach kodu, gdzie
faktycznie w parametrze był wskaźnik? A potem autor to przepisał, na
wersję bezparametrową i zamiast dać void wyczyścił listę parametrów, nie
poprawiając wcześniejszych wywołań?


> No i proponuję też zamienić
>     switch(rxBF[1]) {   //MQTTBuffer
> na
>     switch(MQTTBuffer[1]) {   //MQTTBuffer

To była pierwsza rzecz jaką sprawdziłem. Wychodzi na to, że:
1) MQTTReadPacket() wołane bezpośrednio przed tym switchem zwraca 2.
2) MQTTBuffer[01] w tym miejscu zwraca wartość 0x02. Zawsze, niezależnie
od tego czy dane do logowania były prawidłowe, czy nie. Postanowiłem
więc sprawdzić co mamy w MQTTBuffer[0] i sprawa się rozjaśniła - mamy
tam 0x20. Razem te dwa bajty stanowią więc prawidłowy nagłówek
wiadomości CONNACK. Dalej powinny iść jeszcze dwa bajty, z których
ostatni stanowi reurn code informujący o stanie autoryzacji. Serwer
istotnie wysyła całą wiadomość - sprawdziłem tcpdump i wiresharkiem.
3) Spróbowałem więc po prostu sprawdzać MQTTBuffer[3] ale niestety - nie
znajduję tam return code. Wygląda to faktycznie tak, jakby
MQTTReadPacket w tym miejscu odczytywało tylko dwa pierwsze bajty, co
zgadzałoby się z wartością zwracaną przez funkcję.

do góry

Atlantis <m...@w...pl> napisał(a):
> Zmienna llen jest zmienną lokalną, a wiec jest domyślnie inicjalizowana
> wartością 0.

Odwrotnie. Nie jest inicjalizowana bo nie jest statyczna. Inicjalizacja
domyślna zmiennych odbywa się raz, przed uruchomieniem main(). Zmienne
automatyczne, alokowane na stosie, mają de facto przypadkowe wartości.

--
Grzegorz Niemirowski
https://www.grzegorz.net/

do góry

On 09.08.2022 08:36, Atlantis wrote:
[...]
> Czyżby pozostałość po jakichś wcześniejszych wersjach kodu, gdzie
> faktycznie w parametrze był wskaźnik? A potem autor to przepisał, na
> wersję bezparametrową i zamiast dać void wyczyścił listę parametrów, nie
> poprawiając wcześniejszych wywołań?
Tak, na to to właśnie dla mnie wygląda. Widać to zresztą po niektórych
komentarzach w kodzie.

> MQTTReadPacket w tym miejscu odczytywało tylko dwa pierwsze bajty, co
> zgadzałoby się z wartością zwracaną przez funkcję.
Jak dla mnie to ta funkcja to jakieś totalne, niedorobione gówno.
Prowizorka znaczy się. Tutaj bym szukał błędu. Kody stanów w switch-u
zakodowane na twardo, a nie za pomocą enumów/makr - jak mniemam chodzi o
kody wymienione w enum-ie na początku. Stany zmieniane za pomocą
m_state++ czy m_state=3 zamiast m_state=NAZWA_KOLEJNEGO_STANU - aż się
prosi o kłopoty. Nie chce mi się analizować tego kodu, ale wydaje mi
się, że nie wszystkie stany są obsługiwane.

do góry

On 09.08.2022 09:47, Grzegorz Niemirowski wrote:

> Odwrotnie. Nie jest inicjalizowana bo nie jest statyczna. Inicjalizacja
> domyślna zmiennych odbywa się raz, przed uruchomieniem main(). Zmienne
> automatyczne, alokowane na stosie, mają de facto przypadkowe wartości.

Masz rację, moja pomyłka.
W ramach eksperymentu usunąłem z kodu wszelkie przypadki zmiennych
przekazywanych do MQTTReadPacket() przez wskaźnik na pustej liście
parametrów. Takich sytuacji było dosłownie tylko kilka.
Najbardziej istotna wydawała się sytuacja, gdzie taka zmienna llen była
potem wykorzystywana do wyliczania indeksów tablicy MQTTBuffer. Usunąłem
wszelkie odwołania do tej zmiennej, ustawiłem parametr MQTTReadPacket na
void i skompilowałem program. Wszystko działa tak jak poprzednio - ani
lepiej, ani gorzej.
Wcześniej założyłem, że musi się tam dziać jakaś dziwna "ezoteryczna
magia" wynikająca ze specyfiki języka i w jakiś niewidoczny w kodzie
sposób odpowiednia wartość trafia jednak do zmiennej llen i wszystko
działa jak powinno.
Być może jednak po prostu w moim przypadki ten "if" nie jest nigdy
wywoływany, więc program nie wchodził nigdy w tę gałąź i losowe wartości
ze stosu trafiające do llen nie dawały o sobie znać.

do góry

On 09.08.2022 10:16, JDX wrote:

> Jak dla mnie to ta funkcja to jakieś totalne, niedorobione gówno.
> Prowizorka znaczy się. Tutaj bym szukał błędu. Kody stanów w switch-u
> zakodowane na twardo, a nie za pomocą enumów/makr - jak mniemam chodzi o
> kody wymienione w enum-ie na początku. Stany zmieniane za pomocą
> m_state++ czy m_state=3 zamiast m_state=NAZWA_KOLEJNEGO_STANU - aż się
> prosi o kłopoty. Nie chce mi się analizować tego kodu, ale wydaje mi
> się, że nie wszystkie stany są obsługiwane.

Też się skłaniam ku tej hipotezie. Będę musiał przyjrzeć się temu
bliżej, bo na razie wygląda na to, że ta funkcja nie jest kompatybilna z
wiadomością CONNACK i po natrafieniu na nią nie czyta wszystkiego, a
jedynie pobiera dwa pierwsze bajty.
Autor nie zauważył tego, bo przez przypadek tak się złożyło, że w
switch/case sprawdzał wartość komórki pamięci, która zawsze miała
wartość 0x00, więc zawsze wyglądało to tak, jakby połączenie zostało
autoryzowane. Najwyraźniej nie przetestował nigdy działania biblioteki z
błędnym loginem/hasłem.

Generalnie i tak prościej będzie usunąć z tego błędy o trochę wyczyścić
kod, niż pisać swoją własną bibliotekę albo portować inną pod stos
TCP/IP MLA od Microchipa...

To prawdopodobnie nie będzie ostatnia rzecz, bo w Wiresharku widzę
jeszcze jakieś retransmisje przy zamykaniu połączenia TCP po zakończonej
transmisji, ale najpierw chciałem się uporać z problemem z autoryzacją,

do góry