Skip to end of metadata
Go to start of metadata

This guideline has not been reviewed recently and may be outdated. Please review it and comment to reflect any newly available information.

Some functions, especially those originally specified for the C Language, will either return a valid value or a value of the correct return type that indicates an error (for example, -1 or a null pointer). It is important that these function return values are checked to ensure that an error has not occurred. Otherwise, unpredictable results are possible.

Non-Compliant Code Example

In this example, input_string is copied into dynamically allocated memory referenced by str. However, the result of malloc(input_string_size) is not checked before str is referenced. Consequently, if malloc() fails, the program will abnormally terminate.

char *str = (char *)malloc(strlen(input_string)+1);
strcpy(str, input_string); /* What if malloc() fails? */

Compliant Solution 1

The malloc() function, as well as the other memory allocation functions, returns either a null pointer or a pointer to the allocated space. Always test the returned pointer to make sure it is not equal to zero (NULL) before referencing the pointer. Handle the error condition appropriately when the returned pointer is equal to zero.

char *str = (char *)malloc(strlen(input_string)+1);
if (str == NULL) {
  /* Handle Allocation Error */
}
strcpy(str, input_string);

Compliant Solution 2

A better approach is to use C++ facilities that throw exceptions, rather than those that use error codes. For example, it is better to use new rather than the malloc series of memory allocation functions.

char *str = new char [strlen(input_string)+1];
strcpy(str, input_string);

Risk Assessment

Failing to detect error conditions can lead to unpredictable results, including abnormal program termination and denial-of-service attacks or, in some situations, could even allow an attacker to run arbitrary code.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

ERR10-CPP

high

likely

medium

P18

L1

Automated Detection

Tool

Version

Checker

Description

CodeSonar4.5p1

LANG.FUNCS.IRV

Ignored return value
Klocwork2017NPD.FUNC.MUST
SV.RVT.RETVAL_NOTTESTED
 
LDRA tool suite9.7.1

 

382 S, 121 D, 122 D

Partially implemented

PRQA QA-C++4.1

3508

 
Parasoft C/C++test9.5CODSTA-122_{a,b} 

Related Vulnerabilities

Search for other vulnerabilities resulting from the violation of this rule on the CERT website.

Bibliography

[CWE] CWE-252: Unchecked Error Condition
[Henricson 97] Recommendation 12.1 Check for all errors reported from functions


            

5 Comments

  1. Compliant Solution 2 is not equivalent to the previous code samples: the resulting pointer cannot be free()-d, but must be released with delete[]. This excludes its use in cases where, for example, ownership of the memory needs to be transferred to a legacy C library.

    This should be mentioned, to avoid leading readers into an even more serious and common trap than a malloc failure... 

  2. I changed this from a rule to a recommendation because it was overly strong and conflicts with similar rules and recommendations from the C standard.

  3. Since this rule is about checking for error conditions, Compliant Solution 2 isn't terribly relevant as it shows no error checking. A better example of a compliant solution would be one where the exception thrown by operator new[]() on allocation failure were caught (and either handled or rethrown).

    For instance, the following compliant solution shows how a function might check for memory allocation failures:

    void diff_files(const char *fname1, const char *fname2) {
    
        // no need to check for the first allocation failure
        // here, simply propagate the exception to the caller
        char *buf1 = new char [BUFSIZ];
        char *buf2;
    
        try {
          // check and handle exceptions thrown during
          // the second allocation to prevent memory leaks
          buf2 = new char [BUFSIZ];
        }
        catch(...) {
          // deallocate successfully allocated buffer and
          // propagate the original exception to the caller
          delete[] buf1;
          throw;
        }
    
        try {
          // read the contens of the two files one line at
          // a time into buf1 and buf2, computing the diff
          // between the two and writing it to stdout
        }
        catch(...) {
          // catch exceptions thrown during input or output,
          // clean up, and propagate the original exception
          // to the caller
          delete[] buf2;
          delete[] buf1;
          throw;
        }
    
        // clean up buffers and return
        delete[] buf2;
        delete[] buf1;
    }
    
    1. Your code does a better job of actually sending error conditions to be checked than compliant solution 2. But the point of CS 2 is to support ERR07-CPP. Use exception handling rather than error codes, because when unchecked, 'ignored' exceptions cause the program to abort.

      FTM, your code would be even simpler if you used RAII to handle deallocation...say by using std::string to handle the filenames rather than char*'s.

      Sigh. While technically correct, this rule (and whole section) needs a lot of work to adequately account for C++ exceptions.

      1. I definitely agree that RAII would be the way to go, but that would provide no opportunity for error checking.

        I hadn't realized that this rule was specifically about checking error codes rather than error (and exception) handling in general. Let me see if I can put together a C++ specific example that focuses on error codes and avoids involving exceptions.

        I also agree with your comment regarding this section. I plan on spending some time and enhancing the rest of if after the holidays.