7

In the following example:

void bad_function()
{  
  char_t * ptr = 0;

  // MISRA doesn't complains here, it allows cast of char* to void* pointer
  void* p2 = ptr;

  // the following 2 MISRA violations are reported in each of the casts bellow (two per code line)
  // (1) Event misra_violation:     [Required] MISRA C++-2008 Rule 5-2-7 violation: An object with pointer type shall not be converted to an unrelated pointer type, either directly or indirectly
  // (1) Event misra_violation:     [Required] MISRA C++-2008 Rule 5-2-8 violation: An object with integer type or pointer to void type shall not be converted to an object with pointer type
  ptr = (char_t*) (p2); 
  ptr = static_cast<char_t*> (p2); 
  ptr = reinterpret_cast<char_t*> (p2); 
}

MISRA 5-2-8 and 5-2-7 violations are reported.

How I can remove this violation ?

I need someone experienced with C++ static analysis to help me. I am hitting my head with this stupid rules from few days.

According to MISRA C++ standard (MISRA-Cpp-2008.pdf: Rule 5-2-7 (required): An object with pointer type shall not be converted to an unrelated pointer type, either directly or indirectly.

Ok but we have a lot of code which for example needs to convert address to char* and then to use it with std::ifstream, which read(char* buffer, int length) function requires to type cast the address to (char_t*). So how according to MISRA guys someone can program in C++ and not using at all any casts? The standard doesn't say HOW pointer conversion then must be done.

In my production code my problems are in file reading operations using read with std::ifstream from files in predefined data structures:

if (file.read((char_t*)&info, (int32_t)sizeof(INFO)).gcount() != (int32_t)sizeof(INFO)
{
                LOG("ERROR: Couldn't read the file info header\n");
                res = GENERAL_FAILURE;
}

How is supposed to do it according to MISRA?

So are there any solutions at all?

EDIT: Peter and Q.Q. answers are both correct, it seems that MISRA really wants to do everything without any casts which is hard to be done if the project is in the final stage. Therea are two options:

1 - document the MISRA deviations one by one and explain why casts are Ok, explain how this has been tested (Q.Q. suggestion)

2 - use byte array from char type for file.read(), then after safely reading the file content cast the byte array to the headers content, this must be done for each member one by one because if you cast char* to int32_t this is again Rule 5-2-7 violation. Sometimes it is too much work.

273K
  • 29,503
  • 10
  • 41
  • 64
Baj Mile
  • 750
  • 1
  • 8
  • 17
  • What is `char_t`? Do you have to use it? – MikeCAT Dec 09 '15 at 15:20
  • It is just typedef char char_t; MISRA doesn't like using base types. – Baj Mile Dec 09 '15 at 15:22
  • Why are you casting to a `void*`? – NathanOliver Dec 09 '15 at 15:23
  • There's a very sound reason why MISRA doesn't like the default `char` type for anything but C strings, namely the implementation-defined signedness. If you are only using `char` for strings, you are fine. If you are using it for arithmetic, other than what's listed as valid exceptions by MISRA, creating a `char_t` typedef is not the correct way to go: you should then have used `uint8_t` or `int8_t`. – Lundin Dec 09 '15 at 15:24
  • Yes, sorry, I am using int8_t. – Baj Mile Dec 09 '15 at 15:25
  • 3
    Casting from `void *` to other type assumes the pointer to be properly aligned, otherwise UB. – user3528438 Dec 09 '15 at 15:26
  • 1
    I am using void* - just to show that MISRA allows it. My problem in the production code is with converting pointers to data structures ;like (&header) to char* pointer which is then passed to file io functions. I am unable to find a way to do this. – Baj Mile Dec 09 '15 at 15:29
  • There is nothing wrong with the OPs code according to the C++ standard. Question is what to do about MISRA coding specification disallowing it. – Simple Dec 09 '15 at 15:32
  • 1
    @BajMile The key here is to read the MISRA rule and try to understand the rationale behind the rule. Because the problem is most often not with MISRA itself, but with the static analyser. If you have understood why MISRA created the rule and can deduct that the issue they were concerned about is not present in your code, you can likely just ignore the warning. I don't know the reason behind this rule so I can't help there. – Lundin Dec 09 '15 at 15:35
  • 1
    are you using an automated tool to find these guideline violations? Doesn't this tool have an instruction manual? – Richard Hodges Dec 09 '15 at 15:41
  • Technically pointer to POD is very related to pointer to char, so I suggest you look at the interpretation of the rules by the code checker. – user3528438 Dec 09 '15 at 16:20
  • Just a passing comment on the first comment "it allows cast of char* to void*" -- the **conversion** from `char*` to `void*` in the code does **not** use a cast. A cast is something you write in your source code to tell the compiler to do a conversion. – Pete Becker Dec 09 '15 at 16:44
  • Maybe trick with `enum` may remove Misra warning, but not sure it is better. – Jarod42 Dec 09 '15 at 17:45
  • @Lundin, no uint8_t or int8_t are not valid alternatives. The strict aliasing rule gives special behavior only to narrow character types and not extended integer types. – Ben Voigt Dec 10 '15 at 14:12
  • 1
    @BenVoigt That may be so, but any implementation which treats `uint8_t` differently than `unsigned char` when it comes to aliasing would be so utterly useless that everyone using it deserve all the bugs they can get. It doesn't matter what the standard says: just because an implementation conforms to the standard, it doesn't mean that the implementation should actively seek to be useless in real-world applications. If I encountered a compiler which gave alias-related bugs because I use `uint8_t` instead of `char`, I'd throw it out the window, the standard be damned. – Lundin Dec 10 '15 at 14:43
  • "cast the byte array to the headers content," - you really don't want to do that , so the MISRA restriction is good in this case. – M.M Feb 17 '16 at 20:16

3 Answers3

2

The basic reason for the MISRA rule is that converting any pointer/address to any non-void pointer allows using that address as if it is a different object than it actually is. The compiler would complain about an implicit conversion in those cases. Using a typecast (or C++ _cast operators) essentially stops the compile complaining and - in too many circumstances to count - dereferencing that pointer gives undefined behaviour.

In other words, by forcing a type conversion, you are introducing potential undefined behaviour, and turning off all possibility of the compiler alerting you to the possibility. MISRA think that is a bad idea .... not withstanding the fact that a lot of programmers who think in terms of ease of coding think it is a good idea in some cases.

You have to realise that the philosophy of MISRA checks is less concerned about ease of programming than typical programmers, and more concerned about preventing circumstances where undefined (or implementation defined or unspecified, etc) behaviours get past all checks, and result in code "in the wild" that can cause harm.

The thing is, in your actual use case, you are relying on file.read() correctly populating the (presumably) data structure named info. 

if (file.read((char_t*)&info, (int32_t)sizeof(INFO)).gcount() != (int32_t)sizeof(INFO)
{
            LOG("ERROR: Couldn't read the file info header\n");
            res = GENERAL_FAILURE;
}

What you need to do is work a bit harder to provide valid code that will pass the MISRA checker. Something like

std::streamsize size_to_read = whatever();
std::vector<char> buffer(size_to_read);  

if (file.read(&buffer[0], size_to_read) == size_to_read)
{
      //  use some rules to interpret contents of buffer (i.e. a protocol) and populate info
      // generally these rules will check that the data is in a valid form
      //   but not rely on doing any pointer type conversions
}
else
{
            LOG("ERROR: Couldn't read the file info header\n");
            res = GENERAL_FAILURE;
}

Yes, I realise it is more work than simply using a type conversion, and allowing binary saves and reads of structs. But them's the breaks. Apart from getting past the MISRA checker, this approach has other advantages if you do it right, such as the file format being completely independent of what compiler is used to build your code. Your code depends on implementation-defined quantities (the layout of members in a struct, the results of sizeof) so your code - if built with compiler A - may be unable to read a file generated by code built with compiler B. And one of the common themes of MISRA requirements is reducing or eliminated any code with behaviour that may be sensitive to implementation-defined quantities.

Note: you are also passing char_t * to std::istream::read() as first argument and an int32_t as the second argument. Both are actually incorrect. The actual arguments are of type char * and std::streamsize (which may be, but is not required to be an int32_t).

Peter
  • 35,646
  • 4
  • 32
  • 74
  • Peter, thanks for the great answer. I had doubts that someone reasonable has wrote this MISRA standard so now it seems that it is so. If I use byte arrays, no need by std::vector because in most cases headers sizes are small, I will have later other problems when converting the byte arrays to accual data - ints. shorts, etc. I know how to do this in the MISRA way, without any casts, but it will take too much time. I will keep in mind this. I have defined char_t as char to satisfy another MISRA rule and it doesn't complain about std::streamsize but you are generally right about std::streamsiz – Baj Mile Dec 14 '15 at 15:20
  • No problem. I used `std::vector` to create a buffer for convenience. However, since `std::vector` does dynamic memory allocation, that may not be acceptable depending on your system requirements. Just remember that most guidelines for a "safer" C++ (or a safer ) focus on safety first, programmer convenience/productivity second - because programmer convenience/productivity often means using techniques that compromise safety. That is as true of MISRA as it is for any other comparable guidelines you might pick. – Peter Dec 15 '15 at 12:49
  • I could be wrong, but does MISRA C++ really *force* the explicit type conversion? I appreciate that some implicit type conversions fall foul of other rules, but... – Andrew Nov 23 '22 at 12:49
  • @Andrew MISRA C++ is a set of guidelines, and not all projects have to comply with MISRA. The ones that do, normally do so to meet some specific objectives (e.g. related to independently verifiable system safety). If you are working on such a project, then you are required to either comply, refer to an approved exemption, or justify a new exemption in context of the project. Your justification would need to convince relevant decision makers (such as a regulator who has endorsed the objectives, or senior engineers responsible for ensuring the project meets its objectives). – Peter Nov 23 '22 at 14:44
  • I'm fully aware of what MISRA is ;-) My profile offers a hint... – Andrew Nov 25 '22 at 15:49
1

Converting unrelated pointers to char* is not a good practice. But, if you have a large legacy codebase doing this frequently, you can suppress rules by adding special comments.

Q Q
  • 263
  • 2
  • 10
  • Yes everyone uses it. For example std::ifstream has read() function which needs char*. Then if I try - file.read((char_t*)(&header), (int32_t)sizeof(HEADER)) I am getting MISRA violation. I don't know about any way this to be solved even if I rewrite the whole code to use only char buffers in all file read operations like this. – Baj Mile Dec 09 '15 at 15:35
  • 1
    I guess you need these convertions for serialization and deserialization.So, you can add serialization methods. In the methods you can suppress "Rule 5-2-7" explaining your argument in a commnet block. – Q Q Dec 09 '15 at 15:42
  • Just because "everyone uses it" does not mean it is a good idea... – Andrew Nov 23 '22 at 12:47
0

fread is a perfectly good C++ function for file input and it uses void*, which MISRA allows.

It also is good at reading binary data, unlike fstream which processes all data through localized character conversion logic (this is the "facet" on an iostream, which is configurable, but the Standard doesn't define any portable way to achieve a no-op conversion).

The C-style of fopen/fclose is unfortunate in a C++ program, since you might forget to cleanup your files. Luckily we have this std::unique_ptr which can add RAII functionality to an arbitrary pointer type. Use std::unique_ptr<FILE*, decltype(&fclose)> and have fast exception-safe binary file I/O in C++.

NB: A common misconception is that std::ios::binary gives binary file I/O. It does not. All it affects are newline conversion and (on some systems) end-of-file marker processing, but there is no effect on facet-driven character conversion.

Ben Voigt
  • 277,958
  • 43
  • 419
  • 720
  • 1
    Thanks for the answer. MISRA Rule 27-0-1 doesn't allow to use the stdio.h and fopen, fread. We have local policy that also forbid that. std:ifstream must be used. I will check anyway if I can trick it using std::unique_ptr . – Baj Mile Dec 09 '15 at 15:57
  • Well, if stdio is forbidden, I guess you can use the OS function `open`/`read`/`write`/`close`. Same approach using `std::unique_ptr` and a deleter function applies. – Ben Voigt Dec 09 '15 at 16:01
  • Doesn't MISRA-C++ also ban dynamic allocation, like MISRA-C does? If dynamic allocation is banned, then smart pointers are consequently banned too. – Lundin Dec 10 '15 at 12:46
  • @Lundin that does not follow. unique_ptr can manage resources other than memory, if you set a cleanup function which is not a deallocator – Ben Voigt Dec 10 '15 at 14:09
  • One of the compilers used are for RTOS and doesn't support unique_ptr and C++ x11. – Baj Mile Dec 14 '15 at 12:09