From 553514e3b78b30c73849121694cdf2c52192b2c0 Mon Sep 17 00:00:00 2001 From: TheMalkavien Date: Tue, 1 Oct 2024 00:56:29 +0200 Subject: [PATCH] Fix #4911 : Partially rework some code to remove warnings about potential non-aligned memory accesses (#4912) * * Adding the -Wcast-align compilation flag for the rp2040. * * Some rework to use a struct to access radio data * Buffer will not be accessed by arithmetic pointer anymore * * Remplace arithmetic pointer to avoid Warning * * Avoid 2 little artitmetic pointer --------- Co-authored-by: Ben Meadors --- arch/rp2xx0/rp2040.ini | 2 +- src/gps/GPS.cpp | 4 ++-- src/gps/NMEAWPL.cpp | 7 +++++-- src/mesh/CryptoEngine.cpp | 11 +++++------ src/mesh/RadioInterface.cpp | 24 +++++++++++------------- src/mesh/RadioInterface.h | 17 +++++++++++++++-- src/mesh/RadioLibInterface.cpp | 26 ++++++++++++-------------- 7 files changed, 51 insertions(+), 40 deletions(-) diff --git a/arch/rp2xx0/rp2040.ini b/arch/rp2xx0/rp2040.ini index d3f27a676..5b4ec74d2 100644 --- a/arch/rp2xx0/rp2040.ini +++ b/arch/rp2xx0/rp2040.ini @@ -7,7 +7,7 @@ platform_packages = framework-arduinopico@https://github.com/earlephilhower/ardu board_build.core = earlephilhower board_build.filesystem_size = 0.5m build_flags = - ${arduino_base.build_flags} -Wno-unused-variable + ${arduino_base.build_flags} -Wno-unused-variable -Wcast-align -Isrc/platform/rp2xx0 -D__PLAT_RP2040__ # -D _POSIX_THREADS diff --git a/src/gps/GPS.cpp b/src/gps/GPS.cpp index b6d2776bc..f2b15ba78 100644 --- a/src/gps/GPS.cpp +++ b/src/gps/GPS.cpp @@ -91,9 +91,9 @@ void GPS::CASChecksum(uint8_t *message, size_t length) // Iterate over the payload as a series of uint32_t's and // accumulate the cksum - uint32_t const *payload = (uint32_t *)(message + 6); for (size_t i = 0; i < (length - 10) / 4; i++) { - uint32_t pl = payload[i]; + uint32_t pl = 0; + memcpy(&pl, (message + 6) + (i * sizeof(uint32_t)), sizeof(uint32_t)); // avoid pointer dereference cksum += pl; } diff --git a/src/gps/NMEAWPL.cpp b/src/gps/NMEAWPL.cpp index 71943b76c..f528c4607 100644 --- a/src/gps/NMEAWPL.cpp +++ b/src/gps/NMEAWPL.cpp @@ -75,10 +75,13 @@ uint32_t printWPL(char *buf, size_t bufsz, const meshtastic_Position &pos, const uint32_t printGGA(char *buf, size_t bufsz, const meshtastic_Position &pos) { GeoCoord geoCoord(pos.latitude_i, pos.longitude_i, pos.altitude); - tm *t = gmtime((time_t *)&pos.timestamp); + time_t timestamp = pos.timestamp; + + tm *t = gmtime(×tamp); if (getRTCQuality() > 0) { // use the device clock if we got time from somewhere. If not, use the GPS timestamp. uint32_t rtc_sec = getValidTime(RTCQuality::RTCQualityDevice); - t = gmtime((time_t *)&rtc_sec); + timestamp = rtc_sec; + t = gmtime(×tamp); } uint32_t len = snprintf( diff --git a/src/mesh/CryptoEngine.cpp b/src/mesh/CryptoEngine.cpp index 535e11e9b..fd3dd7a54 100644 --- a/src/mesh/CryptoEngine.cpp +++ b/src/mesh/CryptoEngine.cpp @@ -69,9 +69,8 @@ bool CryptoEngine::encryptCurve25519(uint32_t toNode, uint32_t fromNode, uint64_ uint32_t *extraNonce; long extraNonceTmp = random(); auth = bytesOut + numBytes; - extraNonce = (uint32_t *)(auth + 8); - memcpy(extraNonce, &extraNonceTmp, - 4); // do not use dereference on potential non aligned pointers : *extraNonce = extraNonceTmp; + memcpy((uint8_t *)(auth + 8), &extraNonceTmp, + sizeof(uint32_t)); // do not use dereference on potential non aligned pointers : *extraNonce = extraNonceTmp; LOG_INFO("Random nonce value: %d\n", extraNonceTmp); meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(toNode); if (node->num < 1 || node->user.public_key.size == 0) { @@ -88,8 +87,8 @@ bool CryptoEngine::encryptCurve25519(uint32_t toNode, uint32_t fromNode, uint64_ printBytes("Attempting encrypt using shared_key starting with: ", shared_key, 8); aes_ccm_ae(shared_key, 32, nonce, 8, bytes, numBytes, nullptr, 0, bytesOut, auth); // this can write up to 15 bytes longer than numbytes past bytesOut - memcpy(extraNonce, &extraNonceTmp, - 4); // do not use dereference on potential non aligned pointers : *extraNonce = extraNonceTmp; + memcpy((uint8_t *)(auth + 8), &extraNonceTmp, + sizeof(uint32_t)); // do not use dereference on potential non aligned pointers : *extraNonce = extraNonceTmp; return true; } @@ -105,7 +104,7 @@ bool CryptoEngine::decryptCurve25519(uint32_t fromNode, uint64_t packetNum, size uint32_t extraNonce; // pointer was not really used auth = bytes + numBytes - 12; #ifndef PIO_UNIT_TESTING - memcpy(&extraNonce, auth + 8, 4); // do not use dereference on potential non aligned pointers : (uint32_t *)(auth + 8); + memcpy(&extraNonce, auth + 8, sizeof(uint32_t)); // do not use dereference on potential non aligned pointers : (uint32_t *)(auth + 8); LOG_INFO("Random nonce value: %d\n", extraNonce); meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(fromNode); diff --git a/src/mesh/RadioInterface.cpp b/src/mesh/RadioInterface.cpp index 6fca67188..fe24624ce 100644 --- a/src/mesh/RadioInterface.cpp +++ b/src/mesh/RadioInterface.cpp @@ -595,26 +595,24 @@ size_t RadioInterface::beginSending(meshtastic_MeshPacket *p) lastTxStart = millis(); - PacketHeader *h = (PacketHeader *)radiobuf; - - h->from = p->from; - h->to = p->to; - h->id = p->id; - h->channel = p->channel; - h->next_hop = 0; // *** For future use *** - h->relay_node = 0; // *** For future use *** + radioBuffer.header.from = p->from; + radioBuffer.header.to = p->to; + radioBuffer.header.id = p->id; + radioBuffer.header.channel = p->channel; + radioBuffer.header.next_hop = 0; // *** For future use *** + radioBuffer.header.relay_node = 0; // *** For future use *** if (p->hop_limit > HOP_MAX) { LOG_WARN("hop limit %d is too high, setting to %d\n", p->hop_limit, HOP_RELIABLE); p->hop_limit = HOP_RELIABLE; } - h->flags = p->hop_limit | (p->want_ack ? PACKET_FLAGS_WANT_ACK_MASK : 0) | (p->via_mqtt ? PACKET_FLAGS_VIA_MQTT_MASK : 0); - h->flags |= (p->hop_start << PACKET_FLAGS_HOP_START_SHIFT) & PACKET_FLAGS_HOP_START_MASK; + radioBuffer.header.flags = p->hop_limit | (p->want_ack ? PACKET_FLAGS_WANT_ACK_MASK : 0) | (p->via_mqtt ? PACKET_FLAGS_VIA_MQTT_MASK : 0); + radioBuffer.header.flags |= (p->hop_start << PACKET_FLAGS_HOP_START_SHIFT) & PACKET_FLAGS_HOP_START_MASK; // if the sender nodenum is zero, that means uninitialized - assert(h->from); + assert(radioBuffer.header.from); - memcpy(radiobuf + sizeof(PacketHeader), p->encrypted.bytes, p->encrypted.size); + memcpy(radioBuffer.payload, p->encrypted.bytes, p->encrypted.size); sendingPacket = p; return p->encrypted.size + sizeof(PacketHeader); -} +} \ No newline at end of file diff --git a/src/mesh/RadioInterface.h b/src/mesh/RadioInterface.h index 129861441..6df51ce1a 100644 --- a/src/mesh/RadioInterface.h +++ b/src/mesh/RadioInterface.h @@ -45,6 +45,20 @@ typedef struct { uint8_t relay_node; } PacketHeader; +/** + * This structure represent the structured buffer : a PacketHeader then the payload. The whole is + * MAX_LORA_PAYLOAD_LEN + 1 length + * It makes the use of its data easier, and avoids manipulating pointers (and potential non aligned accesses) + */ +typedef struct { + /** The header, as defined just before */ + PacketHeader header; + + /** The payload, of maximum length minus the header, aligned just to be sure */ + uint8_t payload[MAX_LORA_PAYLOAD_LEN + 1 - sizeof(PacketHeader)] __attribute__ ((__aligned__)); + +} RadioBuffer; + /** * Basic operations all radio chipsets must implement. * @@ -91,8 +105,7 @@ class RadioInterface /** * A temporary buffer used for sending/receiving packets, sized to hold the biggest buffer we might need * */ - uint8_t radiobuf[MAX_LORA_PAYLOAD_LEN + 1]; - + RadioBuffer radioBuffer __attribute__ ((__aligned__)); /** * Enqueue a received packet for the registered receiver */ diff --git a/src/mesh/RadioLibInterface.cpp b/src/mesh/RadioLibInterface.cpp index aee43676d..647add0e5 100644 --- a/src/mesh/RadioLibInterface.cpp +++ b/src/mesh/RadioLibInterface.cpp @@ -387,7 +387,7 @@ void RadioLibInterface::handleReceiveInterrupt() } #endif - int state = iface->readData(radiobuf, length); + int state = iface->readData((uint8_t*)&radioBuffer, length); if (state != RADIOLIB_ERR_NONE) { LOG_ERROR("ignoring received packet due to error=%d\n", state); rxBad++; @@ -397,7 +397,6 @@ void RadioLibInterface::handleReceiveInterrupt() } else { // Skip the 4 headers that are at the beginning of the rxBuf int32_t payloadLen = length - sizeof(PacketHeader); - const uint8_t *payload = radiobuf + sizeof(PacketHeader); // check for short packets if (payloadLen < 0) { @@ -405,10 +404,9 @@ void RadioLibInterface::handleReceiveInterrupt() rxBad++; airTime->logAirtime(RX_ALL_LOG, xmitMsec); } else { - const PacketHeader *h = (PacketHeader *)radiobuf; rxGood++; // altered packet with "from == 0" can do Remote Node Administration without permission - if (h->from == 0) { + if (radioBuffer.header.from == 0) { LOG_WARN("ignoring received packet without sender\n"); return; } @@ -418,22 +416,22 @@ void RadioLibInterface::handleReceiveInterrupt() // nodes. meshtastic_MeshPacket *mp = packetPool.allocZeroed(); - mp->from = h->from; - mp->to = h->to; - mp->id = h->id; - mp->channel = h->channel; + mp->from = radioBuffer.header.from; + mp->to = radioBuffer.header.to; + mp->id = radioBuffer.header.id; + mp->channel = radioBuffer.header.channel; assert(HOP_MAX <= PACKET_FLAGS_HOP_LIMIT_MASK); // If hopmax changes, carefully check this code - mp->hop_limit = h->flags & PACKET_FLAGS_HOP_LIMIT_MASK; - mp->hop_start = (h->flags & PACKET_FLAGS_HOP_START_MASK) >> PACKET_FLAGS_HOP_START_SHIFT; - mp->want_ack = !!(h->flags & PACKET_FLAGS_WANT_ACK_MASK); - mp->via_mqtt = !!(h->flags & PACKET_FLAGS_VIA_MQTT_MASK); + mp->hop_limit = radioBuffer.header.flags & PACKET_FLAGS_HOP_LIMIT_MASK; + mp->hop_start = (radioBuffer.header.flags & PACKET_FLAGS_HOP_START_MASK) >> PACKET_FLAGS_HOP_START_SHIFT; + mp->want_ack = !!(radioBuffer.header.flags & PACKET_FLAGS_WANT_ACK_MASK); + mp->via_mqtt = !!(radioBuffer.header.flags & PACKET_FLAGS_VIA_MQTT_MASK); addReceiveMetadata(mp); mp->which_payload_variant = meshtastic_MeshPacket_encrypted_tag; // Mark that the payload is still encrypted at this point assert(((uint32_t)payloadLen) <= sizeof(mp->encrypted.bytes)); - memcpy(mp->encrypted.bytes, payload, payloadLen); + memcpy(mp->encrypted.bytes, radioBuffer.payload, payloadLen); mp->encrypted.size = payloadLen; printPacket("Lora RX", mp); @@ -475,7 +473,7 @@ void RadioLibInterface::startSend(meshtastic_MeshPacket *txp) size_t numbytes = beginSending(txp); - int res = iface->startTransmit(radiobuf, numbytes); + int res = iface->startTransmit((uint8_t*)&radioBuffer, numbytes); if (res != RADIOLIB_ERR_NONE) { LOG_ERROR("startTransmit failed, error=%d\n", res); RECORD_CRITICALERROR(meshtastic_CriticalErrorCode_RADIO_SPI_BUG);