Misra fix or suppress remaining violations #529

alfred2g · 2022-07-29T00:16:38Z

Fix Remaining Misra violations

Description

Fix Remaining and new misra violations

Test Steps

Misra checker + unit tests

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

paulbartell · 2022-08-03T18:27:18Z

source/include/FreeRTOSIPConfigDefaults.h

    #define ipconfigNETWORK_MTU    1500
 #else
    /* A sanity check to avoid a possible overflow of size_t. */
    #if ipconfigNETWORK_MTU > ( SIZE_MAX >> 1 )


This should just trigger a #error.

source/include/FreeRTOSIPConfigDefaults.h

wrong user

MISRA.md

Co-authored-by: Aniruddha Kanhere <[email protected]>

paulbartell · 2022-08-03T19:43:06Z

MISRA.md

+#### Rule 8.13
+_Ref 8.13.1_
+
+- MISRA C-2012 Rule 8.13 Parameter passed is never used, should be declared as


This should be re-worded to be more generic. Something like:

MISRA C-2012 Rule 8.13 encourages the use of const-qualified types whenever possible. However, the FreeRTOS API requires that all task entry functions conform to the same function signature (TaskFunction_t) which includes a single void * pointer argument. Due to this restriction, it is not possible to use a const qualifier for the entry point function to any FreeRTOS task.

I think it was fixed with this commit: 81e90d8

IMO: It's too specific to the particular instance. We're likely to run into this exact violation quite frequently.

it is ok to be specific might be even desirable, we do support multiple References for specific cases

MISRA.md

source/FreeRTOS_Sockets.c

source/portable/BufferManagement/BufferAllocation_2.c

Co-authored-by: Aniruddha Kanhere <[email protected]>

paulbartell · 2022-08-03T20:03:18Z

MISRA.md

+#### Rule 21.6
+_Ref 21.6.1_
+
+- MISRA C-2012 Rule 21.6 warns about the use of standard library input/output


This justification should mention how we prevent misuse of printf-style functions. Mentioning "-Wformat-security" and/or specific Wformat options that are used when building unit tests would be helpful here.

paulbartell · 2022-08-03T20:09:29Z

MISRA.md

+
 #### Rule 11.3
 _Ref 11.3.1_



What about for 64-bit targets that do not allow 32-bit aligned access?

We currently do not support any hardware with that feature/drawback the only 64 bit we currently support is RiscV and they do support unaligned access (if i am not mistaken)
when we support more 64bit hardware we will have to add code to cover that case.

@paulbartell wrote:

What about for 64-bit targets that do not allow 32-bit aligned access?

A 64-bit target that do not allow 32-bit aligned access? I am not sure what you mean here.

@Alfred2 wrote:

We currently do not support any hardware with that feature/drawback the only 64 bit we currently support is RiscV

As for FreeRTOS+TCP, we also support the 64-bit UltraScale platform with a Cortex-A53.

I have never seen alignment problems with the casting of types.

The Cortex-A53 is only strict about alignment when configured such in the SCTLR. It's pretty common to configure it to allow unaligned access.

In contrast, a Cortex-M0 (ARMv6-M) will always fail unaligned accesses.. but is 4-byte aligned.

SPARC and MIPS are both example of a 64bit architectures that do not support unaligned access... So de-referencing a 32-bit aligned pointer as a 64bit type could cause a fault at runtime.

In the case of FR+TCP, we're unlikely to run into this problem in particular.

source/FreeRTOS_IP.c

htibosch

@alfred2g It sounds like the MISRA checks are almost all ready!
I only have two comments, mostly about 64-bit platforms.
I approve it, thanks

htibosch · 2022-08-05T04:19:09Z

MISRA.md

+
 #### Rule 11.3
 _Ref 11.3.1_



@paulbartell wrote:

What about for 64-bit targets that do not allow 32-bit aligned access?

A 64-bit target that do not allow 32-bit aligned access? I am not sure what you mean here.

@Alfred2 wrote:

We currently do not support any hardware with that feature/drawback the only 64 bit we currently support is RiscV

As for FreeRTOS+TCP, we also support the 64-bit UltraScale platform with a Cortex-A53.

I have never seen alignment problems with the casting of types.

htibosch · 2022-08-05T07:27:45Z

source/portable/BufferManagement/BufferAllocation_2.c

+                    /* MISRA Ref 11.3.1 [Misaligned access] */
+                    /* More details at: https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/blob/main/MISRA.md#rule-113 */
+                    /* coverity[misra_c_2012_rule_11_3_violation] */
                    *( ( NetworkBufferDescriptor_t ** ) ( pxReturn->pucEthernetBuffer ) ) = pxReturn;


*( ( NetworkBufferDescriptor_t ** ) ( pxReturn->pucEthernetBuffer ) ) = pxReturn;

In other places in the library, I have put asserts before assuming that a pointer can be written at a byte address. In this case, the character pointer was returned by pvPortMalloc(), and it is assumed that it is 4-byte aligned.

On my Xilinx UltraScale, the size of a pointer is 8 and pvPortMalloc() will make sure that only 64-bit aligned pointers are returned, see the portBYTE_ALIGNMENT macros for ARM_CA53_64_BIT:

#define portBYTE_ALIGNMENT 16 #define portBYTE_ALIGNMENT_MASK 0x000f

In case of a 64-bit platform, the macro ipconfigBUFFER_PADDING will be checked with asserts: it must be at least 14, and it must be a multiple of 4 plus 2 bytes:

/* This is a 64-bit platform, make sure there is enough space in * pucEthernetBuffer to store a pointer. */ configASSERT( ( sizeof( uintptr_t ) != 8U ) || ( ipconfigBUFFER_PADDING >= 14 ) ); /* But it must have this strange alignment: */ configASSERT( ( sizeof( uintptr_t ) != 8U ) || ( ( ( ( ipconfigBUFFER_PADDING ) + 2 ) % 4 ) == 0 ) );

When ipconfigBUFFER_PADDING is defined, it will be assigned to ipBUFFER_PADDING.

alfred2g requested a review from a team as a code owner July 29, 2022 00:16

alfred2g marked this pull request as draft July 29, 2022 00:16

alfred2g force-pushed the misra_remaining branch 2 times, most recently from 8d2dcf3 to 2b59b8a Compare July 29, 2022 18:59

alfred2g marked this pull request as ready for review August 1, 2022 22:01

alfred2g changed the title ~~Misra remaining~~ Misra fix remaining violations Aug 1, 2022

alfred2g changed the title ~~Misra fix remaining violations~~ Misra fix or suppress remaining violations Aug 2, 2022

aws-afr-bot previously approved these changes Aug 3, 2022

View reviewed changes

paulbartell reviewed Aug 3, 2022

View reviewed changes

source/include/FreeRTOSIPConfigDefaults.h Outdated Show resolved Hide resolved

paulbartell reviewed Aug 3, 2022

View reviewed changes

source/include/FreeRTOSIPConfigDefaults.h Outdated Show resolved Hide resolved

alfred2g added 14 commits August 3, 2022 11:35

Fix Remaning misra issues

35d787d

Suppress rule 8.6

a76d481

Fix/Suppress more misra violations

18dfde3

Style: for formatting

e0c458c

Style: fix formatting

68d597c

Style: fix spelling

9541e0d

Fix Rule 11.1

13c58cb

Fix undeteced suppressions

57caf76

Enable 32 bits

a9dd58d

Fix more misra leftover violations

d2f5158

Add justification for a missed violation

1567226

Fix comment for rule 8.13

2d2e745

Fix comment

40c82c9

fix misra comments

e3156cc

alfred2g force-pushed the misra_remaining branch from 4debf6f to e3156cc Compare August 3, 2022 18:37

AniruddhaKanhere reviewed Aug 3, 2022

View reviewed changes

MISRA.md Outdated Show resolved Hide resolved

Update MISRA.md

81e90d8

Co-authored-by: Aniruddha Kanhere <[email protected]>

alfred2g requested a review from paulbartell August 3, 2022 18:57

alfred2g requested review from AniruddhaKanhere and aws-afr-bot August 3, 2022 18:57

paulbartell reviewed Aug 3, 2022

View reviewed changes

AniruddhaKanhere requested changes Aug 3, 2022

View reviewed changes

MISRA.md Outdated Show resolved Hide resolved

MISRA.md Outdated Show resolved Hide resolved

source/FreeRTOS_Sockets.c Outdated Show resolved Hide resolved

source/portable/BufferManagement/BufferAllocation_2.c Outdated Show resolved Hide resolved

alfred2g and others added 5 commits August 3, 2022 12:55

Update source/FreeRTOS_Sockets.c

0ece037

Co-authored-by: Aniruddha Kanhere <[email protected]>

Update source/portable/BufferManagement/BufferAllocation_2.c

6f6d630

Co-authored-by: Aniruddha Kanhere <[email protected]>

Update MISRA.md

fb24ef8

Co-authored-by: Aniruddha Kanhere <[email protected]>

Update MISRA.md

46705be

Co-authored-by: Aniruddha Kanhere <[email protected]>

Suppress Rule 8.9

561419c

paulbartell reviewed Aug 3, 2022

View reviewed changes

alfred2g requested a review from AniruddhaKanhere August 3, 2022 20:03

paulbartell reviewed Aug 3, 2022

View reviewed changes

source/FreeRTOS_IP.c Show resolved Hide resolved

Fix build error

464ff7b

alfred2g force-pushed the misra_remaining branch from 0fc6dc7 to 464ff7b Compare August 3, 2022 20:19

alfred2g added 2 commits August 3, 2022 13:23

fix build error

c945cde

Fix coverity supression bugs

5ab12ef

alfred2g requested a review from paulbartell August 4, 2022 22:05

htibosch approved these changes Aug 5, 2022

View reviewed changes

paulbartell approved these changes Aug 8, 2022

View reviewed changes

AniruddhaKanhere approved these changes Aug 8, 2022

View reviewed changes

AniruddhaKanhere merged commit 4ac10c8 into FreeRTOS:main Aug 8, 2022

Misra fix or suppress remaining violations #529

Misra fix or suppress remaining violations #529

Uh oh!

Conversation

alfred2g commented Jul 29, 2022

Description

Test Steps

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htibosch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants