0

Both the LHS and RHS variables are uint8_t variable, but the issue is reported as " casting from int to unsigned char". I am not understanding how this can be an issue?

The same is applicable for an 8-bit numbers

All the variables listed in both issues are uint8_t

Issue 1)

CID 147563 (#2 of 2): Coding standard violation (CERT INT31-C)3. cert_violation: 
Casting (uint8_t)apX_compY_bitmask from int to unsigned char without checking its 
value may result in lost or misinterpreted data.

/* AP_X_Flash_Component_Y_Authenticated */
static uint8_t AP_component_require_auth; 

//Local variable:

uint8_t apX_compY_bitmask = 0u, port;

// other operations

AP_component_require_auth |= (uint8_t)apX_compY_bitmask;

Issue 2)

CID 148170 (#1 of 1): Coding standard violation (CERT INT31-C)5. cert_violation: 
Casting major_revision >> 3 from int to unsigned char without checking its 
value may result in lost or misinterpreted data.

Function argument:

void sb_rollb_prot_AP_FW_in_use_update(uint8_t img_idx, uint8_t port, uint8_t major_revision, bool primary_image)

//Local Variable
uint8_t x_loc, y_loc;
y_loc = major_revision >> 3;
Clifford
  • 88,407
  • 13
  • 85
  • 165
Kanni1303
  • 73
  • 11
  • Did you read the CERT rules? You can't have a static analyser checking for violations of a coding standard you don't know about, that's plain dangerous. [Read the Friendly CERT-C Manual](https://wiki.sei.cmu.edu/confluence/display/c/INT31-C.+Ensure+that+integer+conversions+do+not+result+in+lost+or+misinterpreted+data) which is available for free online. And yes, wild implicit conversions between signed `int` and `uint8_t` is dangerous and will eventually become a source for subtle bugs. – Lundin Apr 22 '20 at 10:09
  • I even tried with adding u for 3 (3u) the issue persists same.. – Kanni1303 Apr 22 '20 at 10:17
  • @Lundin Also, I am not getting one point, you mean to say I can't use Coverity for checking Cert C rules...??? – Kanni1303 Apr 22 '20 at 10:21
  • I'm saying: what exactly about the mentioned rules is it that you don't understand? Or if you want someone to tell you what's wrong with your code, you need to post the code, including variable declarations. – Lundin Apr 22 '20 at 10:35
  • I understand the rule, but not understanding why it is showing for this line, as I mentioned all the variables are declared in uint8_t, Added the variable declarations is enough, can you point me what is the problem with respect to checker ID INT31-C – Kanni1303 Apr 22 '20 at 13:22
  • 1
    I suppose they are concerned about the [implicit type promotions](https://stackoverflow.com/questions/46073295/implicit-type-promotion-rules) present in both your examples. `AP_component_require_auth |= (uint8_t)apX_compY_bitmask;` = `AP_component_require_auth = AP_component_require_auth | (uint8_t)apX_compY_bitmask;` where both operands of `|` are implicitly promoted to `int`. And in `major_revision >> 3;`, the operand `major_revision` is implicitly promoted to `int`. – Lundin Apr 22 '20 at 14:19
  • tried with type casting both the variables, but no use. – Kanni1303 Apr 23 '20 at 08:30

1 Answers1

2

To understand what has caused teh warning, you have to understand (or at least be aware) of C's somewhat arcane and sometimes surprising type promotion rules.

The C bit-wise and arithmetic operators operate on int or unsigned int or larger types, so when presented with operands of a a smaller type an implicit promotion occurs:

Consider this "experiment" for example:

#include <stdint.h>
#include <stdio.h>

int main()
{
    uint8_t a ;
    uint8_t b ;

    printf( "sizeof(a) = %zu\n", sizeof(a) ) ;
    printf( "sizeof(b) = %zu\n", sizeof(b) ) ;
    printf( "sizeof(a | b) = %zu\n", sizeof(a | b) ) ;
    printf( "sizeof((uint8_t)(a | b)) = %zu\n", sizeof((uint8_t)(a | b)) ) ;
    printf( "sizeof(a >> 3) = %zu\n", sizeof(a >> 3) ) ;
    printf( "sizeof((uint8_t)(a >> 3)) = %zu\n", sizeof((uint8_t)(a >> 3)) ) ;


    return 0;
}

The output (where int is 32-bit) is:

sizeof(a) = 1
sizeof(b) = 1
sizeof(a | b) = 4
sizeof((uint8_t)(a | b)) = 1
sizeof(a >> 3) = 4
sizeof((uint8_t)(a >> 3)) = 1

So in the first case:

AP_component_require_auth |= (uint8_t)apX_compY_bitmask;

The uint8_t cast serves no purpose since it already is that type, and certainly does not defeat the implicit conversion.

I am not familiar with CERT-C or Coverity, but in similar tools I have used, an implicit cast may be used to assert that the expression is deliberate:

AP_component_require_auth = (uint_8_t)(AP_component_require_auth | apX_compY_bitmask) ;

y_loc = (uint8_t)(major_revision >> 3) ;

As you can see it is not possible to resolve this using |= because you cannot then cast the result of the | expression before assignment.

However often it is better to maintain type agreement and avoid either implicit or explicit conversions and use int, unsigned or equal/larger sized integer type if there is no compelling reason to use a smaller type.

The issue in both cases is the assignment of an int sized type to a uint8_t. Though the first warning is somewhat confusing - probably due to the use of |= - preventing it form presenting the implicitly cast expression; you should get the same error without the unnecessary cast I think. The static analysis tool I am familiar with, would say something like:

implicit conversion to smaller type in assignment

in both these cases, which is I think is much clearer.

The Coverity warning is terse and minimal; if you go directly to the standard it is enforcing, it is much more explicit and gives rationale, examples and solutions: https://wiki.sei.cmu.edu/confluence/display/c/INT31-C.+Ensure+that+integer+conversions+do+not+result+in+lost+or+misinterpreted+data

Clifford
  • 88,407
  • 13
  • 85
  • 165
  • Implicit cast is not solving the issue CID 154581 (#1 of 1): Coding standard violation (CERT INT31-C)3. cert_violation: Casting AP_component_require_auth | apX_compY_bitmask from int to unsigned char without checking its value may result in lost or misinterpreted data. `AP_component_require_auth = (uint8_t)(AP_component_require_auth | apX_compY_bitmask); ` is there any settings or something else is missing in my code? – Kanni1303 Apr 23 '20 at 04:55
  • I did not suggest *implicit* cast as a solution. Quite the opposite. You need an *explicit* cast. I assume from the error that that is what you meant. It seems that the tool expects you to test for range, which is unreasonable in this instance. To be fair, your question asked for an explanation rather than a solution. Did you get a similar error for the y_loc assignment? – Clifford Apr 23 '20 at 07:21
  • If the tool is going to make you jump through unnecessary hoops, then perhaps: `AP_component_require_auth = (uint8_t)(0xff & (AP_component_require_auth | apX_compY_bitmask));`? Failing that it would be reasonable to get a concession and use whatever Coverity uses to suppress warnings for specific lines. The other solution I suggested was to ensure type agreement - use `uint32_t` rather than `uint8_t`. – Clifford Apr 23 '20 at 08:04
  • The explanation should lead to a solution, right? Are you saying the Coverity tool itself has some bug, hence it is showing the error, not analyzing properly? I can use uint32_t, but the problem is size. This is for embedded platform, and nearly 204 errors like this, adding 3 bytes each, will lead to 600bytes additional, which is huge for 32KB data ram system – Kanni1303 Apr 23 '20 at 08:29
  • @Kanni1303 Unless your compiler is crap, it will be able to optimize the code better than that. However, this particular CERT-C rule appears a bit cumbersome, when compared to other such rules in other coding standards like MISRA-C:2012, which I find far more precise. CERT themselves even refer to no less than 5 different MISRA rules to cover this single CERT rule. – Lundin Apr 23 '20 at 08:41
  • @Kanni1303 I doubt that, many of the warnings will be for expressions involving the same variables. It would only add 600 bytes if the warnings related to 200 different variables with static storage class. In most cases it will increase stack usage somewhat. – Clifford Apr 23 '20 at 14:19
  • @Kanni1303 As I said, I am not familiar with either the tool or the standard, but the link I included in the answer presents an explicit cast as a compliant solution, so you will have to ask the tool vendor why it rejects it - it is paid for software with support after all. – Clifford Apr 23 '20 at 14:24
  • Thank you for the guidance... it is better to query on the tool vendor... may be a bug on Tool itself... – Kanni1303 Apr 27 '20 at 04:33