u-blox / ubxlib

Portable C libraries which provide APIs to build applications with u-blox products and services. Delivered as add-on to existing microcontroller and RTOS SDKs.
Apache License 2.0
287 stars 82 forks source link

ESP-IDF 5.1.2 Optimization fails to compile UBXLIB #198

Closed synologic closed 5 months ago

synologic commented 5 months ago

Hi all,

I'm using ESP-IDF 5.1.2 and CONFIG_COMPILER_OPTIMIZATION_PERF=y (menuconfig -> Compiler Options/Optimization Level -O2) which results in compilation error

ubxlib/wifi/src/u_wifi_mqtt.c:1277:29: error: 'strncpy' output truncated before terminating nul copying as many bytes from a string as its length [-Werror=stringop-truncation] 1277 | strncpy(pTopicNameStr, pFoundTopicStr, foundTopicLen);

ubxlib/cell/src/u_cell_http.c:348:9: error: 'strncpy' specified bound 249 equals destination size [-Werror=stringop-truncation] 348 | strncpy(pHttpInstance->fileNameResponse, pFileNameGiven, | ^~~~~~~~~~~~ 349 | sizeof(pHttpInstance->fileNameResponse)); | ~~~~~~~~

ubxlib/cell/src/u_cell_http.c:348:9: error: 'strncpy' specified bound 249 equals destination size [-Werror=stringop-truncation] 348 | strncpy(pHttpInstance->fileNameResponse, pFileNameGiven, | ^~~~~~~~~~~~ 349 | sizeof(pHttpInstance->fileNameResponse));

ubxlib/cell/src/u_cell_cfg.c:1030:9: error: 'strncpy' specified bound 65 equals destination size [-Werror=stringop-truncation] 1030 | strncpy(buffer + 2, pStr, sizeof(buffer) - 2); | ^~~~~~~~~~~~~

ubxlib/cell/src/u_cell_cfg.c:1002:9: error: 'strncpy' specified bound 65 equals destination size [-Werror=stringop-truncation] 1002 | strncpy(buffer + 2, pStr, sizeof(buffer) - 2); | ^~~~~~~~~~~~~

Not sure if this happens on lower versions of IDF though as i have none installed, i understand 5.1.2 is not supported, i was just wondering if this is something that happens in 5.0 or even 4.x

Thanks

RobMeades commented 5 months ago

That's odd: and this only happens with O2 optimization on? We compile that code under GCC, Xtenza, MSVC, apply two forms of Clang analysis and run it under Valgrind on Linux! I will take a look.

synologic commented 5 months ago

Only O2, the rest work fine

RobMeades commented 5 months ago

I can see what it means with the cellular ones, but I can't follow why it thinks the WiFi one is wrong:

memset(pTopicNameStr, 0, topicNameSizeBytes); // pTopicNameStr is memset to zero for length topicNameSizeBytes
err = uShortRangePktListConsumePacket(&pMqttSession->rxPkt, pMessage, pMessageSizeBytes,
                                      &edmChannel);
pMqttSession->unreadMsgsCount = pMqttSession->rxPkt.pktCount;
if (err == 0) {
    err = (int32_t)U_ERROR_COMMON_NO_MEMORY;
    pFoundTopicStr = getTopicStrForEdmChannel(pMqttSession, edmChannel);
    if (pFoundTopicStr != NULL) {
        foundTopicLen = strlen(pFoundTopicStr);
        if ((foundTopicLen + 1) <= topicNameSizeBytes) { // Check that the buffer length, topicNameSizeBytes, is at least one bigger than foundTopicLen
            strncpy(pTopicNameStr, pFoundTopicStr, foundTopicLen);
            err = (int32_t)U_ERROR_COMMON_SUCCESS;
        }
    }
}

I guess we could change it to:

strncpy(pTopicNameStr, pFoundTopicStr, topicNameSizeBytes);

...if that would make the compiler happy.

RobMeades commented 5 months ago

Now fixed: it will get pushed here after review but, if you need something that works under -O2 on ESP-IDF quickly, you can find a preview branch here:

https://github.com/u-blox/ubxlib/tree/preview_fix_buffer_strings_rmea

I will update this issue once the commit gets to master and delete the preview branch some time after that.

RobMeades commented 5 months ago

The fix is now pushed here in commit https://github.com/u-blox/ubxlib/commit/ed16997a3cd576cae3371e69a545fdf411e29a4b; I will delete the preview branch some time next week. Closing this issue: please feel free to re-open, or open a new issue, if there is more to discuss.