calloc issue on second time - on C

Question

bellow is the code:

from some reason the calloc inside the while loop, is failing on the second iteration. it looks the heap is corupted (not sure) but not clear the root cause. please also take a look on the comment added there. appriciate fast response.

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

struct User_
{
    char* id;
    char* firstName;
    char* lastName;
    int age;
    char gender[2];
    char* userName;
    char* password;
    char* description;
    char hobbies[2];
}typedef User;


void replaceEnterInString(int lengthString, char* string, int  maxChars);




int main()
    {
        char str1[500] = "012345678;danny;cohen;22;M;danny1993;123;1,2,4,8;Nice person";
        char str2[500] = "223325222;or;dan;25;M;ordan10;1234;3,5,6,7;Singer and dancer";


    int j = 0;
    char *token = NULL, arrangingHobbies;
    int lengthStr, tempAge, hobby[4], i;

    while(j<2)
    {

        User* newUser = NULL;

here it pass on first time but fail on second time. but only when adding the code that map the token to the newUser. without the mapping - do manage to calloc the user again and again as much as needed

error code: Critical error detected c0000374 - TEST.exe has triggered a breakpoint.

        newUser = (User*)calloc(1, sizeof(User));
        if (newUser == NULL)
        {
            printf("error");
            exit(1);
        }

        //start map string to user
        if (j == 0)
        {
            token = strtok(str1, ";");
            printf("%s", str1);
        }
        else {
            token = strtok(str2, ";");
            printf("%s", str2);
        }

        //Input ID
        newUser->id = (char*)calloc(10, sizeof(char));
        if (newUser->id == NULL)
        {
            printf("error");
            exit(1);
        }
        strcpy(newUser->id, token);

        //Input first name
        token = strtok(NULL, ";");
        lengthStr = strlen(token);
        newUser->firstName = (char*)calloc((lengthStr + 1), sizeof(char));
        if (newUser->firstName == NULL)
        {
            printf("error");
            exit(1);
        }
        strcpy(newUser->firstName, token);

        //Input last name
        token = strtok(NULL, ",;");
        lengthStr = strlen(token);
        newUser->lastName = (char*)calloc((lengthStr + 1), sizeof(char));
        if (newUser->lastName == NULL)
        {
            printf("error");
            exit(1);
        }
        strcpy(newUser->lastName, token);

        //Input Age
        token = strtok(NULL, ",;");
        tempAge = atoi(token);
        newUser->age = tempAge;

        //Input gender
        token = strtok(NULL, ",;");
        newUser->gender[0] = token[0];


        //Input User Name
        token = strtok(NULL, ",;");
        lengthStr = strlen(token);
        newUser->userName = (char*)calloc((lengthStr), sizeof(char));
        if (newUser->userName == NULL)
        {
            printf("error");
            exit(1);
        }
        strcpy(newUser->userName, token);

        //Input password
        token = strtok(NULL, ",;");
        lengthStr = strlen(token);
        newUser->password = (char*)calloc((lengthStr), sizeof(char));
        if (newUser->password == NULL)
        {
            printf("error");
            exit(1);
        }
        strcpy(newUser->password, token);

        //Input hobbies
        newUser->hobbies[0] = 0;
        for (i = 0; i < 4; ++i)
        {
            token = strtok(NULL, ",;");
            tempAge = atoi(token);
            arrangingHobbies = 1;
            arrangingHobbies <<= (tempAge - 1);
            newUser->hobbies[0] |= arrangingHobbies;
        }

        //Input description
        token = strtok(NULL, ",;");
        newUser->description = (char*)calloc((lengthStr), sizeof(char));
        if (newUser->description == NULL)
        {
            printf("error");
            exit(1);
        }
        replaceEnterInString(strlen(token), token, 300);
        strcpy(newUser->description, token);

        j++;
    }

}

void replaceEnterInString(int lengthString, char* string, int  maxChars)
{
    if (lengthString < maxChars)
    {
        //remove the /n
        string[lengthString - 1] = '\0';
    }
}

Probably not related to the problem, but you should always use `unsigned` types when doing bit shifting. — Barmar, Jan 02 '19 at 22:49
[don't cast malloc](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) — Barmar, Jan 02 '19 at 22:51
the posted code, when run through the compiler (`gcc`) with the `-Wall -Wextra -Wconversion -pedantic -std-gnu11` options results in 12 warning messages. The very first message is the 'typedef' is not at the beginning of the struct definition. This means the compiler figured it out, but the syntax is not correct. When compiling, always enable the warnings, then fix those warnings. — user3629249, Jan 02 '19 at 23:17
OT: the function: `strlen()` returns a `size_t` so (to avoid risky conversions) the variable receiving the returned value should also be a `size_t`, not an `int` — user3629249, Jan 02 '19 at 23:26
OT: regarding: `printf("error");` 1) error messages should be output to `stderr`, not `stdout` 2) when the error indication is from a C library function, should also output to `stderr` the text reason the system thinks the error occurred. Suggest using: `perror( "calloc failed" );` — user3629249, Jan 02 '19 at 23:29
OT: when calling `strtok()`, always check (!=NULL) the returned value to assure the operation was successful — user3629249, Jan 02 '19 at 23:33
regarding: `arrangingHobbies = 1;` the variable: `arrangingHobbies` is declared as a `char`, not a `int` So the best that can be hoped for is the resulting value in the variable will be 0x01. However, it would be much better to either declare it as a `int` or assign a char (I.E. '1' ) — user3629249, Jan 02 '19 at 23:41
regarding: `tempAge = atoi(token);` the function: `atoi()` does not have any indication when it fails. Suggest using: `strtol()` instead — user3629249, Jan 02 '19 at 23:44
regarding: `newUser->userName = calloc((lengthStr), sizeof(char));` this is an error and the following statement: `strcpy(newUser->userName, token);` will corrupt the heap structures because there is no allowance for the trailing NUL byte. I.E. it should be: `newUser->userName = calloc((lengthStr+1), sizeof(char));` Notice the `+1 on the size. Similar considerations exist for `newUser->password = calloc((lengthStr), sizeof(char));` — user3629249, Jan 02 '19 at 23:49
regarding: `arrangingHobbies = 1; arrangingHobbies <<= (tempAge - 1);` the first statement sets the variable (at best) to 0x01. The second statement shifts that 0x01 to the left by the value (tempAge-1) When the `tempAge` is greater than 7 the result is undefined behavior — user3629249, Jan 02 '19 at 23:56
regarding: `arrangingHobbies <<= (tempAge - 1);` this causes an implicit (risky) conversion from `int` to `char` — user3629249, Jan 02 '19 at 23:58
regarding: ` token = strtok(NULL, ",;"); newUser->description = calloc((lengthStr +1 ), sizeof(char));` this is missing a statement to set the value in `lengthStr`, so will be using what every was left from the prior setting of `lengthStr`. If the description is longer than what was in `lengthStr` then the allocated memory will be overrun by the statement: `strcpy(newUser->description, token);` This results in undefined behavior — user3629249, Jan 03 '19 at 00:02
in function: `replaceEnterInString()` this statement: `string[lengthString - 1] = '\0';` is not assured to be correct. Strongly suggest: `string[ strcspn( string, "\n" ) ] = '\0';` — user3629249, Jan 03 '19 at 00:06
regarding: `replaceEnterInString(strlen(token), token, 300);` the first parameter is passing the unsigned long (`size_t`) value returned from the call to `strlen()` however, the function: `replaceEnterInString()` is expecting the first parameter to be an `int` (signed) While this might work, there are conditions/values were it will not work — user3629249, Jan 03 '19 at 00:09
OT: the posted code contains the 'magic' number 300. 'magic' numbers are numbers with no basis. 'magic' numbers make the code more difficult to understand, debug, etc. suggest using a `#define` statement or a `enum` statement to give that 'magic' number a meaningful name Then use that meaningful name throughout the code — user3629249, Jan 03 '19 at 00:12

Stephan Lechner · Accepted Answer · 2019-01-02T22:58:02.547

5

Maybe there are other issues as well, yet the following code leads to undefined behaviour for sure:

lengthStr = strlen(token);
newUser->userName = (char*)calloc((lengthStr), sizeof(char));
...
strcpy(newUser->userName, token);

In previous similar statements, you correctly wrote ... = (char*)calloc((lengthStr+1), sizeof(char));.

BTW: In C, you usually don't cast the results of malloc, sizeof(char) is always 1 by definition, and there is no need for setting memory to 0 using calloc if you fill the memory with a subsequent strcpy anyway. So you should write...

lengthStr = strlen(token);
newUser->userName = malloc(lengthStr+1);
...
strcpy(newUser->userName, token);

Look through your code for similar issues, please.

edited Jan 02 '19 at 22:58

answered Jan 02 '19 at 22:52

Stephan Lechner

34,891
4
35
58

1

he also made that mistake with `newUser->description`, and forgot to call `strlen()` as well. – Barmar Jan 02 '19 at 22:55
Also with `newUser->password`. – Weather Vane Jan 02 '19 at 22:57
@Barmar thanks for the fix, this was the issue StephanLechner thanks for all the good comment – efrat Jan 03 '19 at 10:44

calloc issue on second time - on C

1 Answers1