search

Azure / azure-iot-middleware-freertos

Azure IoT Middleware for FreeRTOS
https://azure.github.io/azure-iot-middleware-freertos/
MIT License
82 stars 24 forks source link

This library assumes a 32-bit processor for all length fields on API. #253

Closed phelter closed 2 years ago

phelter commented 2 years ago

When compiling for a 64-bit there's a lot of additional casting due to the API definition of all "uint32_t ul.*Length" parameters

An example is: AzureIoTProvisioningClient_SetSymmetricKey

AzureIoTResult_t AzureIoTProvisioningClient_SetSymmetricKey( AzureIoTProvisioningClient_t * pxAzureProvClient,
                                                             const uint8_t * pucSymmetricKey,
                                                             uint32_t ulSymmetricKeyLength,
                                                             AzureIoTGetHMACFunc_t xHmacFunction );

Should be:

AzureIoTResult_t AzureIoTProvisioningClient_SetSymmetricKey( AzureIoTProvisioningClient_t * pxAzureProvClient,
                                                             const uint8_t * pucSymmetricKey,
                                                             size_t ulSymmetricKeyLength,
                                                             AzureIoTGetHMACFunc_t xHmacFunction );

List of functions that should change are:

Suggest also changing the structures to use size_t as well, but If there is a requirement not to change the types for the structures eg: uint32_t ulSymmetricKeyLength; - to size_t ulSymmetricKeyLength then this library can do the conversion to the uint32_t when they are assigned.

danewalton commented 2 years ago

Hi @phelter This was done for parity with FreeRTOS. Example here: https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/b0a8bd8f28d0138b5eb70e8b53da3e9d17ce8d40/include/task.h#L474

I don't see us changing this at all in this SDK but we appreciate the interest!

phelter commented 2 years ago

@danewalton - You've provided an example which is not relevant to the use case here. A stackDepth is not a Length.

That's like saying a network port which is a uint16_t - what it is defined on the network layer stack is a size.

In FreeRTOS-Kernel there are the following examples of using size_t: The most identifiable one is here where config_MESSAGE_BUFFER_LENGTH_TYPE is defined as size_t https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/11c72bc075b49bfdeb73d7e4bd94fd5a7d0a5e77/include/FreeRTOS.h#L994 - This example is a LENGTH_TYPE defined as size_t.

There are 51 other examples in the code with this search FreeRTOS-Kernel

So right now there are 3 different types used for lengths inside the API's uint32_t uint16_t and size_t.

Best practice is to use size_t - since then you don't have to cast when using sizeof(array) or when using strnlen() or strlen() output or other length checking standard C library functions. Having various different types means the user of your API now needs to cast to the particular type every time it interacts with other functions. In addition - it provides better packing inside structs for the cases where you include a pointer and size back-to-back since in 64-bit machines identifying the size as a uint32_t instead of a size_t will cause a pack warning, but when using a size_t won't - both will have the same size since there is padding added for a uint32_t anyways.

Please consider fixing since this is an annoyance using this API. Any *Length parameter that is associated with a pointer or struct size - the best practice is to use a size_t.