Search code examples
cstringmallocstrlencoverity

Coverity deflect: - String length miscalculation (BAD_ALLOC_STRLEN)


I have a coverity deflect to be fixed but I am not sure about it. I have a function (void my_function(a_type *my_variable)) with the following problematic line of code:

body = malloc(strlen(&((my_type*) *my_variable)->Param2.body[1]) +1);

where body is an unsigned char*.

The Coverity message:

String length miscalculation (BAD_ALLOC_STRLEN)
Using "strlen(((my_type *)*my_variable)->Param2.body + 1)" 
instead of "strlen(((my_type *)*my_variable)->Param2.body) + 1" 
as an argument to "malloc" might be an under-allocation.

Now, given the strlen function call, which looks like this:

strlen(&((my_type*) *my_variable)->Param2.body[1])

and this line is identical to:

strlen(&((my_type*) *my_variable)->Param2.body + 1)

So this should be changed according to the message, and the result would be:

body = malloc((strlen(&((my_type*) *my_variable)->Param2.body)+1) +1);

Why is bad such an argument for malloc? I do not see what is the actual problem here so I am unsure about this solution and/or its necessity.

Additional information is that, &((my_type*) *my_variable)->Param2.body[1] (simply &Param2.body[1]) will be copied into body using strcpy, like:

strcpy(body, &((my_type *) *my_variable)->Param2.body[1]);

Solution

  • I have reached that conclusion @rici was correct. Considering the following simulation:

    #include <stdio.h>
    #include <string.h>
    #include <stdlib.h>
    
    typedef struct Param2{
        char* body;
    }Param2;
    
    int main()
    {
        Param2 test;
        test.body = "test_string\0";
        printf("%s, size: %d + 1 terminating null\n\n",test.body, strlen(test.body));
    
        printf("original:                %d \n", (strlen(&test.body[1]) + 1));
        printf("what coverity thinks:    %d \n", strlen(test.body + 1));
        printf("what coverity suggests:  %d \n", (strlen(test.body) + 1));
        printf("a more transparent way:  %d \n\n", (strlen(test.body + 1) + 1));
    
        return 1;
    }
    

    This is the output: ![enter image description here

    There are three cases (4th is the same as 1st). The allocated memory can be seen on the image above for all cases. Now, if we want to copy the source string from the 2. byte (&body[1]), that would mean 10 bytes of data in the example. And according to the strcpy documentation:

    To avoid overflows, the size of the array pointed by destination shall be long enough to contain the same C string as source (including the terminating null character), and should not overlap in memory with source.

    We need one more for the null termination giving us 11 bytes to be allocated. Coverity believes that we are allocating 10 bytes, and suggest to allocate 12.

    But as we can see, the original code allocates 11 bytes which is the exact number of bytes we need here, making the coverity deflect false positive.