Not null terminated string - a KlocWork error with no understandable reason.

Question

I've recently installed "klocwork" and am trying to get rid of bugs on an existing code. The error shown seems to be simple. No null at the termination of the char * _p_. I have manually added a null termination (even though there is no need), but it doesn't please the Klocwork. Any ideas?

The exact message is:-

Incorrectly terminated string 'p' causes a buffer overflow in p.

char *ptr;
int writtenchars = 0 ;
va_list args;  
char* destStr;

if (argc != 2) {
  printf(" wrong parameters number - %d instead of %d\n", argc, 2);
  char  str[25]="wrong parameters number ";
  char *_p_; /********************************************************/

  va_start(args, str);
  destStr = (char*) malloc(SNMP_BUF_LEN);
  _p_= destStr;
  if (destStr == NULL) {
    printf("WARNING: Failed to alloc memory in in function \"snmp_rebuildstringinbuf!!!\" \n");
    destStr="kukuRiko";
  }
  else {
    writtenchars = (int) vsnprintf(destStr, 4095, str, args);
    if (writtenchars>SNMP_BUF_LEN) {
      printf("WARNING: Too long string rebuilded in function \"snmp_rebuildstringinbuf!!!\" %d chars\n",writtenchars);
    }
    destStr[writtenchars] = '\0' ; //Moshe - making sure the last value of the string is null terminated in order to prevent future buffer overflows.
  }
  va_end(args);

  /******************************************************************************/
  //The KlocWork error relates to this line //

  logCWriteLog_msg(moduleId, level, __FILE__, __LINE__, _p_, ltrue); 
  free (_p_);   

=========================================================== Hi Guys, Thanks for your answers, but it seems a bit more obscure than that. I have refined the code to this simple case:- When the code is written all in one function there is no error, whereas, when the allocation section is wrapped in a function (and a text passed as parameter) the Klocwork error returns. See this code:- version without an error:-

char *_p_; /*+++++++++++++++++++*/

 int writtenchars = 0 ;
 va_list args;  
 char* destStr;
 char* str = "hello World"; 
 va_start(args, str);
 destStr = (char*)malloc(SNMP_BUF_LEN);
 if (destStr == NULL) {
   printf("WARNING: Failed to alloc memory in function \n");
 }
 else {
   writtenchars = (int) vsnprintf(destStr, (SNMP_BUF_LEN) - 1, str, args);
 }

 /*+++++++++++++++++++*/
 _p_ = destStr ;
 if (_p_ != NULL) {
   logCWriteLog_msg(moduleId, level, __FILE__, __LINE__, _p_, ltrue); 
 }
 free (_p_);
 /***********************************************************/

whereas when taking the code between /*++++ */ and wrapping it in a function returns the above KlocWork error.

Hence,

char *writingToSomeBuffer (char * str) {
  int writtenchars = 0 ;
  va_list args;  
  char* destStr;
  va_start(args, str);
  destStr = (char*)malloc(SNMP_BUF_LEN);
  if (destStr == NULL) {
    printf("WARNING: Failed to alloc memory in function \n");
  }
  else {
    writtenchars = (int) vsnprintf(destStr, (SNMP_BUF_LEN) - 1, str, args);
  }
  return destStr;
}

int main () {
  char *_p_;
  _p_ = writingToSomeBuffer("hello world");
  if (_p_ != NULL) {
    logCWriteLog_msg(moduleId, level, __FILE__, __LINE__, _p_, ltrue); 
  }
  free (_p_);
  return 0 ; 
}

any ideas?

Answer 1

Klocwork's error aside, I think this code is wrong. Why are you limiting the vsnprintf to 4096, while the buffer size is SNMP_BUF_LEN? How do those two related to each other? If SNMP_BUF_LEN < 4096, then you may have just overflowed your buffer. Why wouldn't you pass SNMP_BUF_LEN as the limiting argument in vsnprintf?

Also, the write to destStr[writtenchars] is suspect. Depending on the variant of vsnprintf (they do vary), writtenchars might be the number of characters it wanted to write, which would again cause you to write past the end of your buffer.

That all said, Klocwork isn't perfect. We had macros that were very explicitly trying to be safe, and Klocwork mis-detected them as potentially overrunning the string. I think that was a snprintf case as well.

Overall a good product, but it does have a few holes and you can't fix all it's complaints.

Answer 2

KlocWork is correctly diagnosing the problem that you can be writing with a null pointer if memory allocation fails:

_p_= destStr;
if (destStr == NULL)
{
    printf("WARNING: Failed to alloc memory in in function ...\n");
    destStr = "kukuRiko";

At this point, the (horribly named) '_p_' variable is still null, but you go ahead and use it in the printing operation below.

Also note that the 'trivial' fix of adding '_p_' after this breaks your memory management; you later do 'free(_p_);' which will lead to horrible problems if '_p_' points to the constant string.

You also have 'memory in in function' in the message. And 'wrong parameters number' does mean roughly the same as 'wrong number of parameters' but the latter is more idiomatic English. I'm not convinced any of the exclamation marks are helpful in the error message; there is a strong argument that they should go outside the double quotes surrounding the function name even if one of them is deemed desirable.

---

With the revised version of the problem, I wonder if Klocwork is diagnosing what Microsoft says of its vsnprintf(), that it does not guarantee null termination (which is different from what C99 and POSIX says).

Answer 3

Jonathan has it right. We've recently broken up this checker into two families that might explain it better:

http://www.klocwork.com/products/documentation/Insight-9.1/Checkers:NNTS.MIGHT http://www.klocwork.com/products/documentation/Insight-9.1/Checkers:NNTS.MUST

We are currently under development to clean this up and make it easier to understand. Not only the problem but the solution as well.