Perform explicit tests to determine success, true/false, and equality to improve the readability and maintainability of code and for compatibility with common conventions.
In the case that the return value will never change, an explicit test is still preferable to clearly communicate the numeric, rather than boolean, value of the test.In particular, do not default the test for non-zerononzero. For example, testing instance, suppose a foo()
function returns 0 to indicate failure or a nonzero value to indicate success. Testing for inequality with zero:0,
Code Block | ||||
---|---|---|---|---|
| ||||
if (foo() != 0) ...
|
is preferable to
Code Block | ||||
---|---|---|---|---|
| ||||
if (foo()) ...
|
despite the fact convention that C considers 0 to be false0 indicates failure. Explicitly testing for inequality with zero 0 benefits maintainability if foo()
is later modified to return -1 −1 rather than 0 on failure.
This recommendation is derived from and considers the implications of the following common conventions:(1)
- Functions return 0 if false
...
- and
...
- nonzero if true [
...
...
- Function failures can typically be indicated by
...
- −1 or any nonzero number.
...
- Comparison functions
...
- (such as the standard library function
strcmp()
, which has a trinary return value
...
- ) return 0 if the arguments are equal and nonzero otherwise (see strcmp function).
Noncompliant Code Example
In this noncompliant code example, is_banned()
return returns 0 if false , and non-zero nonzero if true.:
Code Block | ||||
---|---|---|---|---|
| ||||
LinkedList bannedUsers; int is_banned(User usr) { int x = 0; Node cur_node = (bannedUsers->head); while (cur_node != NULL) { if(!strcmp((char *)cur_node->data, usr->name)) { x++; } cur_node = cur_node->next; } return x; } void processRequest(User usr) { if(is_banned(usr) == 1) { return; } serveResults(); } |
The call function processRequest()
ignores the convention that most functions in C only guarantee a non-zero return value to indicate "true".
If a banned user is listed twice, the user is granted access. Although is_banned()
follows the common convention of returning non-zero nonzero for true, processRequest
only checks for equality only with 1.
Compliant Solution
Because most functions only guarantee a return value of non-zero nonzero only for " true, " the the preceding code above is better written by checking for inequality with 0 ("false"), as follows.:
Code Block | ||||
---|---|---|---|---|
| ||||
LinkedList bannedUsers; int is_banned(User usr) { int x = 0; Node cur_node = (bannedUsers->head); while(cur_node != NULL) { if (strcmp((char *)cur_node->data, usr->name)==0) { x++; } cur_node = cur_node->next; } return x; } void processRequest(User usr) { if (is_banned(usr) != 0) { return; } serveResults(); } |
Noncompliant Code Example
In this noncompliant code example, function failures status can typically be indicated by one of the following return values: -1, a non-zero numberreturning −1 on failure or any nonnegative number on success. This is a common convention in the standard C library, but it is discouraged in ERR02-C. Avoid in-band error indicators.
Although failures are frequently indicated by a return value of zero (which C considers to be false), there are 0, some common conventions that may conflict in the future with code where in which the test for non-zero nonzero is not explicit. In this case, defaulting the test for non-zero nonzero welcomes bugs if and when a developer modifies foo validateUser()
to return an error code or -1 −1 rather than 0 to indicate a failure (all of which are also common conventions).
Code Block | ||||
---|---|---|---|---|
| ||||
int validateUser(User usr) { if(listContains(validUsers, usr)) { return 1; } return 0; } void processRequest(User usr, Request request) { if(!validateUser(usr)) { return "invalid user"; } else { serveResults(); } } |
The Although the code above will work as intended. However, it is very feasible possible that a future modification will result in the following.:
Code Block | ||||
---|---|---|---|---|
| ||||
errno_t validateUser(User usr) { if(list_contains(allUsers, usr) == 0) { return 303; //* userUser not found error code */ } if(list_contains(validUsers, usr) == 0) { return 304; //* invalidInvalid user error code */ } return 0; } void processRequest(User usr, Request request) { if(!validateUser(usr)) { return "invalid user"; } else { serveResults(); } } |
In this code, the programmer intended to add error code functionality to indicate the cause of a validation failure. The new code, however, validates any invalid or non-existing nonexisting user. Because there is not an no explicit test in processRequest()
, the logical error is not obvious (the implied "if not validated" seems correct) and seems correct by certain conventions.
Compliant Solution
The following This compliant code is preferable for code improved maintenance. By defining what constitutes a failure and explicitly testing for it, the behavior is clearly implied, and future modifications are more likely to preserve it. If there is a future modification like the one aboveis made, such as in the previous example, it is immediately obvious that the if
statement in processRequest()
does not correctly utilize the specification of validateUser
correctly()
.
Code Block | ||||
---|---|---|---|---|
| ||||
int errno_t validateUser(User usr) { if(list_contains(validUsers, usr)) { return 1; } return 0; } void processRequest(User usr, Request request) { if(validateUser(usr) == 0) { return "invalid user"; } else { serveResults(); } } |
Noncompliant Code Example
Comparison functions (such as the standard library function strcmp()
function) return 0 if the arguments are equal and non-zero nonzero otherwise.
Because many comparison functions (like strcmp) return 0 for equality and non-zero nonzero for inequality, they can cause confusion when used to test for equality. If someone were to switch the following strcmp()
call with an equals function, they a function testing for equality, but the programmer did not follow the same convention as strcmp()
, the programmer might instinctively just replace the function name. Also, when quickly reviewed, the code could easily appear to test for inequality.
Code Block | ||||
---|---|---|---|---|
| ||||
void login(char *usr, char *pw) { User user = find_Useruser(usr); if (!strcmp((user->password),pw_given)) { grantAccess(); } else { denyAccess("Incorrect Password"); } } |
The preceding code above works correctly.
However, in order to simplify the login code or to facilitate checking a user's password more than once, a programmer might can separate the password-checking code from the login function . He/she might do so in the following way.:
Code Block | ||||
---|---|---|---|---|
| ||||
int check_password(User *user, char *pw_given) { if (!strcmp((user->password),pw_given)) { return 1; } return 0; } void login(char *usr, char *pw) { User user = find_Useruser(usr); if (!check_password(user, pw)) { grantAccess(); } else { denyAccess("Incorrect Password"); } } |
In an attempt to leave the previous logic intact, he the developer just replaced the replaces strcmp()
with a call to his the new function. However, doing so would produce produces incorrect behavior. In the this case above, any user which who inputs an incorrect password is granted access. Again, two conventions conflict and produce code that is easily corrupted when modified. To make code maintainable and to avoid these conflicts, such a result should never be defaulted.
Compliant Solution
The following approach to This compliant solution, using a comparison function for this purpose, is the preferred approach. By performing an explicit test, any programmer that who wishes to modify the equality test can clearly see the implied behavior and convention that is being followed.
Code Block | ||||
---|---|---|---|---|
| ||||
void login(char *usr, char *pw) { User user = find_Useruser(usr); if (strcmp((user->password),pw_given) == 0) { grantAccess(); } else { denyAccess("Incorrect Password"); } } |
Risk Assessment
Code which that does not conform to the common practices presented will be presented is difficult to maintain. Bugs may can easily arise when modifying helper functions which that evaluate true/false or success/failure. Bugs may can also easily arise when modifying code that tests for equality using a comparison function that obeys the same conventions as standard library functions like such as strcmp
.
Recommendation | Severity | Likelihood | Remediation Cost | Priority | Level |
---|---|---|---|---|---|
EXP20-C | Medium | Probable | Low | P12 | L1 |
Automated Detection
Tool | Version | Checker | Description | ||||||
Astrée |
| Supported indirectly via MISRA C |
medium
probable
low
P8
L2
References
:2004 Rule 13.2. | |||||||||
Axivion Bauhaus Suite |
| CertC-EXP20 | Fully implemented | ||||||
Helix QAC |
| C3344, C4116 | |||||||
LDRA tool suite |
| 114 S | Partially implemented | ||||||
Parasoft C/C++test |
| CERT_C-EXP20-a | Avoid comparing values with TRUE macro/enum constant using equality operators ("==", "!=") | ||||||
PC-lint Plus |
| 697 | Partially supported: reports comparisons of Boolean values to constants other than 0 |
Bibliography
[StackOvflw 2009] | "Should I Return TRUE/FALSE Values from a C Function?" |
...
[\[StackOvflw 09\]|AA. References#StackOvflw 09] "Should I return TRUE / FALSE values from a C function?
\[[ISO/IEC 9899:1999|AA. References#ISO/IEC 9899-1999]\] Wiki Markup