Why doesn't malloc allocate enough memory?

Question

I am really stuck with one very simple piece of code. This program takes argument like ./a.out -t=1,32,45,2 and prints quantity of commas in stdout. But from time to time execution works correctly and and more often throws segmentation fault.

I figured out that problem in this line of function substr_cnt (I also placed corresponding commentaries in code below):

target_counting = (char *)malloc(sizeof(char)*(strlen(target)));

In fact malloc returns NULL. If I change sizeof(char) by sizeof(char *) all starts work like a charm but I can't understand why is that. Furthermore in main function I also use malloc, and even with the same line

arg_parameter = (char *) malloc(sizeof(char)*(strlen(argv[1] - 3)));

all works just fine.

#include <stdlib.h>
#include <stdio.h>
#include <string.h>

#define strindex(target, source) ((size_t) strstr(target, source) - (size_t) target)

int substr_cnt( char *target, char *source ) {
    int i=0;
    int cnt=0;
    char *target_counting;
    //this is NOT working
    target_counting = (char *)malloc(sizeof(char)*(strlen(target)));
    //this is working
    //target_counting = (char *)malloc(sizeof(char *)*(strlen(target)));

    if (target_counting == NULL) {
        printf("malloc failed\n");
        return -1;
    }
    strcpy(target_counting, target);
    while ((i=strindex(target_counting, source)) > 0) {
        strncpy(target_counting, target_counting + i + 1, strlen(target_counting));
        cnt++;
    }
    free(target_counting);
    return cnt;
}

int main( int argc, char *argv[] )
{
    int i;
    int default_behavior = 0;
    int arg_parametr_cnt;
    char *arg_parameter; 
    if (argc == 1) {
        default_behavior = 1;
    } else if (argv[1][0] == '-' && argv[1][1] == 't' && argv[1][2] == '=') {
        //this is working
        arg_parameter = (char *) malloc(sizeof(char)*(strlen(argv[1] - 3)));
        strncpy(arg_parameter, argv[1]+3, strlen(argv[1]));
        printf("%s\n", arg_parameter);
        arg_parametr_cnt = substr_cnt(arg_parameter, ",");
        printf("commas: %d\n", arg_parametr_cnt);
    }
    else {
        printf("wrong command line");
        return 1;
    } 
    return 0;
}

Answer 1

You have several issues here, the main point, you don't need to allocate memory at all. You can implement searching for a given substring without modifying the string and therefore work directly on the given argv parameters, e.g.

int substr_cnt(const char *haystack, const char *needle)
{
    int cnt = 0;
    const char *found = haystack;
    while ((found = strstr(found, needle)) != NULL) {
        ++found;
        ++cnt;
    }

    return cnt;
}

Same for the call in main, just pass argv directly

arg_parametr_cnt = substr_cnt(argv[1] + 3, ",");

Now to answer your question, unless you really see the output of

printf("malloc failed\n");

I don't believe, malloc returns NULL, because when you allocate an even larger amount of memory, sizeof(char*) vs sizeof(char), it works.

The reasons, why your program crashes, are already covered in the other answers. To summarize

• target_counting = (char *)malloc(sizeof(char)*(strlen(target))); allocates one char less than it should
• while ((i=strindex(target_counting, source)) > 0) I'm not sure, what happens, when the result of strstr is NULL. strindex might return a negative number, depending on your memory layout, but I am not sure.
• strncpy(target_counting, target_counting + i + 1, strlen(target_counting)); This is not really an issue, but since you copy the rest of the string, you could use strcpy(target_counting, target_counting + i + 1) instead.
• arg_parameter = (char *) malloc(sizeof(char)*(strlen(argv[1] - 3))); this should be malloc(sizeof(char) * strlen(argv[1]) - 3 + 1)
• strncpy(arg_parameter, argv[1]+3, strlen(argv[1])); again strcpy(arg_parameter, argv[1]+3) would be sufficient

Update:

In this version

int strindex(char *target, char *source)
{
    char *idx;
    if ((idx = strstr(target, source)) != NULL) {
        return idx - target;
    } else {
        return -1;
    }
}

you have an explicit test for NULL and act accordingly.

In the macro version

#define strindex(target, source) ((size_t) strstr(target, source) - (size_t) target)

there is no such test. You determine the index by calculating the difference between strstr() and the base address target. This is fine so far, but what happens, when strstr() returns NULL?

Pointer arithmetic is defined with two pointers, pointing into the same array. As soon as the two pointers point into different arrays, or one pointing into an array and the other somewhere else, the behaviour is undefined.

Technically, when you calculate NULL - target, it might yield a negative value, but it also might not. If target points to the address of 0x0f0a3a90, you could have 0x0 - 0x0f0a3a90 and get a negative value. If target points to 0xfe830780 however, it might be interpreted as a negative number, and then 0x0 - 0xfe830780 could result in a positive number.

But the main point is, you have undefined behaviour. For further reading look for pointer arithmetic, e.g. C++: Pointer Arithmetic

Answer 2

I think your main issue is here :

 arg_parameter = (char *) malloc(sizeof(char)*(strlen(argv[1] - 3)));

especially here

 strlen(argv[1] - 3)

you pass to strlen address of argv[1]-3 which is not valid address. actually what you meant is strlen(argv[1]) - 3. As others said you also should add one char for \0 so strlen(argv[1]) - 2

Answer 3

There are several buffer overruns, but I think that the bug that makes you program crash is the following:

    strncpy(target_counting, target_counting + i + 1, strlen(target_counting));

Note that the strings in strncpy may not overlap!

I suggest that you do a memmove instead, because memmove can handle overlapping buffers:

    memmove(target_counting, target_counting + i + 1, strlen(target_counting + i + 1) + 1);

Answer 4

The problem may lie here: malloc(sizeof(char)*(strlen(argv[1] - 3)) in main. You are subtracting 3 from argv[1]. I think you intended to use:

malloc(sizeof(char)*(strlen(argv[1]) - 2)); // Allocate one more space for '\0' character

Doing this makes strlen to access unallocated memory.

Your program may not fail here, but later, because it is simply undefined behavior.

Answer 5

your malloc is not allocating space for the null terminator, you need to malloc (strlen(string)+1). The malloc with a char* works because a pointer (is normal) 4 bytes long, so you are allocating 4 times more memory than required - minus the 1 byte need for a null terminator.