A received packet is outputted in DEBUG _after_ it was already parsed,
but with a reference to the already parsed header. The result is that
there can be some garbage in the output and the packet is not dumped in
total. As without parsing we do not have access to the header yet, we
use the `gnrc_netif_addr_to_str()` helper function instead of parsing
the destination address by hand.
before this commit the src address was checked for multicast, but the dst address should be checked. Therefore udp multicast packets would be flooded back to the src as ICMPv6 error, as not all nodes had a UDP receiver registered.
When a keepalive timeout occurs keepalive_retry_cnt remains zero,
so when the connection is re-established _on_keepalive_evt will
immediately disconnect instead of actually sending a keepalive ping.
The sequence looks like:
1. _on_connack: start con->keepalive_timer
2. Server does not respond to keepalive pings
3. _on_keepalive_evt: con->keepalive_retry_cnt reaches zero
4. Connection torn down and ASYMCUTE_DISCONNECTED sent to application
5. Application starts reconnection
6. _on_connack: start con->keepalive_timer again
7. First _on_keepalive_evt: con->keepalive_retry_cnt is still zero
8. Repeat from 4.
So this simply resets keepalive_retry_cnt in _on_connack when
the keepalive timer is restarted. It's a new connection, so
resetting the keepalive retry counter make senses regardless.
Signed-off-by: Derek Hageman <hageman@inthat.cloud>
From the gnrc_pktbuf_mark documentation:
It's not guaranteed that `result->data` points to the
same address as the original `pkt->data.
Thus it should be necessary to update the `hdr` pointer.
If an address was pre-configured by the upper layer its validity is
currently ignored. It is neither checked if the address is on the
interface at all nor is it checked if it is valid.
This change provides a fix for that by checking both facts.
The length field in an MQTT packet carries the _total_ length of the
packet. If it is below 256 (i.e. fits in one byte) only one byte is
used for the length field. If it is larger than that 3 bytes are used,
with the first byte having the value `0x01` and the remaining bytes
representing the length in as a 2 byte unsigned integer in network byte
order. Resulting from that it can be assessed that the check in
`emcutes`'s `set_len()` function is wrong as it needs to be checked if
`len` is lesser or equal to `0xff - 1`. `len <= (0xff - 1)` can be
simplified to `len < 0xff`. For some larger packages this safes 2 bytes
of wasted packet space.
`len` is used with the `memcpy()` to copy the payload to `tbuf`. With a
payload provided that is just long enough to fill `tbuf`, `len += 6`
leads to the `memcpy()` overriding data after `tbuf` (e.g. the
`mutex` that is unlocked right after) and thus resulting in potential
segmentation faults.
Additionally `+ 6` can only be applied if the total packet length is
below 256 (see spec), so `len + pos` is what needs to be provided to the
corresponding send functions instead (`pos` adapts to the header length
of the PUBLISH message).
When reworking the reception of IPv6 packets I reset a previously set
`ipv6` snip as follows when the IPv6 extension handler returns a
packet (see first hunk of this commit):
```C
ipv6 = pkt->next->next
```
With `gnrc_ipv6_ext` this makes *somewhat* sense, `pkt->next` was
previously equal to `ipv6` and after the function call `pkt->next`
is the marked extension header, while `pkt->next->next` is the IPv6
header. However, since `ipv6` is already write-protected i.e.
`ipv6->users == 1` (see ll. 665-675), any additional call of
`gnrc_pktbuf_start_write()` [won't][start-write-doc] duplicate the
packet. In fact, the only `gnrc_pktbuf_start_write()` in
`gnrc_ipv6_ext` is used to send the *result* to the subscribers of that
extension header type, leaving the original packet unchanged for the
caller. As such `ipv6` remains the pointer to the IPv6 header whether
we set it in the line above or not. So we actually don't need that
line.
However, the extension header handling also returns a packet when
`gnrc_ipv6_ext` is not compiled in. In that case it is just a dummy
define that returns the packet you give provide it which means that
this still holds true: `pkt->next == ipv6`.
So setting `ipv6` in this case is actually harmful, as `ipv6` now
points to the NETIF header [following the IPv6 header][pkt-structure]
in the packet and this causes the `user` counter of that NETIF header
`hdr` to be decremented if `hdr->users > 1` in the write-protection I
removed in hunk 2 of this commit:
```C
/* pkt might not be writable yet, if header was given above */
ipv6 = gnrc_pktbuf_start_write(ipv6);
if (ipv6 == NULL) {
DEBUG("ipv6: unable to get write access to packet: dropping it\n");
gnrc_pktbuf_release(pkt);
return;
}
```
But as we already established, `ipv6->users` is already 1, so we don't
actually need the write protection here either.
Since the packet stays unchanged after the `ipv6` snip, we also don't
need to re-search for `netif_hdr` after the other two lines are
removed.
[start-write-doc]: https://doc.riot-os.org/group__net__gnrc__pktbuf.html#ga640418467294ae3d408c109ab27bd617
[pkt-structure]: https://doc.riot-os.org/group__net__gnrc__pkt.html#ga278e783e56a5ee6f1bd7b81077ed82a7
