C - Memncpy/Strncpy (tried strncat too) copies 1 character less than it should, help is appreciated

Question

Like I said, memncpy() (in the middle of main) copies 1 character less than it should, not sure why. I added comments and an image to make it more understandable.

    #define BIT_AMOUNT 4

char * randomBinaryGenerator(char * random){

    int randomNum, i;
    char temp;
    char * tempPtr = (char *)malloc(1 * sizeof(char));

    for(i = 0; i <= BIT_AMOUNT - 1; i++){

        randomNum = rand() % 2;
        temp = randomNum + '0';

        tempPtr = NULL;
        tempPtr = &temp;
        strcat(random, tempPtr);
    }

    return random;
}

int main(){

    srand(time(0));
    char str_bin_bitKey[BIT_AMOUNT] = "";

    char * random = (char *)malloc(BIT_AMOUNT * sizeof(char));
    char * bin_bitKey = (char *)malloc(BIT_AMOUNT * sizeof(char));

    printf("\nSize of str_bin_bitKey: %ld, Size of bin_bitKey: %ld\n", sizeof(str_bin_bitKey), sizeof(bin_bitKey));

    bin_bitKey = randomBinaryGenerator(random);    //generates 4 bit long binary number
    memcpy(str_bin_bitKey, bin_bitKey, BIT_AMOUNT);//copies 1 character less

    printf("\nbin_bitKey:     %s\n", bin_bitKey);    //4 bits
    printf("\nstr_bin_bitKey: %s\n", str_bin_bitKey);//3 bits???

    long long dec_bitKey = 0;//unimportant for now .... convertBinaryToDecimal(bin_bitKey); 

    printf("\ndec_bitKey: %lld\n\n", dec_bitKey);

    free(random);
    return 0;
}

This is what the output looks like, As you can see str_bin_bitKey is 3 characters instead of 4:

I appreciate all the help.

Note that `tempPtr = NULL; tempPtr = &temp;` are overwriting the pointer (to 1 byte!) returned by `malloc`. — Weather Vane, Jun 08 '20 at 15:34
Aside: instead of `i <= BIT_AMOUNT - 1` please use the more idiomatic `i < BIT_AMOUNT`. The first is harder to read, and can be **faulty**. — Weather Vane, Jun 08 '20 at 15:36
In the call `strcat(random, tempPtr);` both parameters need to point to null terminated strings. But `tempPtr` is pointing to a single `char` variable that does not contain `'\0'`, so it is not a null terminated string. — Ian Abbott, Jun 08 '20 at 15:36
Making images of plain text does not make anything more understandable. It only prevents selecting and copying any content. — Gerhardh, Jun 08 '20 at 15:57
Also in the call `strcat(random, tempPtr);` `random` is pointing to a block of memory allocated with `malloc()` by the caller (`main()`), but is is not null terminated. — Ian Abbott, Jun 08 '20 at 16:02
Update: I have remeoved tempPtr = NULL; and made sure the strings were null terminated. tempPtr = &temp; strcat(tempPtr, "\0"); strcat(random, tempPtr); strcat(random, "\0"); I also didn't allocate memory to tempPtr since the pointer was being overwritten. Still doesn't seem to work, it works perfectly once I delete the function call to randomBinaryGenerator and replace it with something like "1010". Off topic: how do I indent code inside comments? — HamzaKerem, Jun 08 '20 at 17:14
OT: regarding: `char * tempPtr = (char *)malloc(1 * sizeof(char));` 1) in C, the returned type is `void*` which can be assigned to any pointer. Casting just clutters the code and is error prone. Suggest removing that cast. 2) the expression: `sizeof( char )` is defined in the C standard as 1. multiplying anything by 1 has no effect and just clutters the code. Suggest removing that expression. 3) always check (!=NULL) the returned value to assure the operation was successful. — user3629249, Jun 08 '20 at 21:59
regarding: `tempPtr = NULL; tempPtr = &temp;` each of these statements overlay the pointer supplied by the call to `malloc()`. Once overlayed, it cannot be recovered. The result is a unrecoverable memory leak. Suggest removing the call to `malloc()` and the statement: `tempPtr = NULL` — user3629249, Jun 08 '20 at 22:02

score 1 · Accepted Answer · answered Jun 08 '20 at 18:43

A few issues ...

In main, the size of the arrays/pointers need to allow for the nul terminator, so they need to be BIT_AMOUNT + 1

In main, your memcpy does not copy the nul terminator. Use strcpy instead.

Adding a starting nul is most easily done with (e.g.):

*random = 0;

Don't cast malloc: Do I cast the result of malloc?

sizeof(char) is always 1 (by definition), regardless of the actual, architecture dependent size (e.g. char is actually 16 bits). So, don't use sizeof(char)

In randomBinaryGenerator, tempPtr leaks memory. No need for malloc [or even tempPtr at all]. Use char temp[2]; instead.

sizeof(bit_bitKey) is always constant, because it's the size of the pointer and not what it points to (i.e. it is not BIT_AMOUNT).

randomBinaryGenerator needs almost a complete rework.

Here's an annotated and fixed version of your code. I've added:

#if 0
// old/original code
#else
// new/fixed code
#endif

to help show the changes.

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

#define BIT_AMOUNT 4

char *
randomBinaryGenerator(char *random)
{

    int randomNum, i;
#if 0
    char temp;
    char *tempPtr = malloc(1);
#else
    char temp[2];
#endif

#if 1
    // add nul terminator
    *random = 0;
    temp[1] = 0;
#endif

#if 0
    for (i = 0; i <= BIT_AMOUNT - 1; i++) {
#else
    for (i = 0; i < BIT_AMOUNT; i++) {
#endif
        randomNum = rand() % 2;
#if 0
        temp = randomNum + '0';
        tempPtr = NULL;
        tempPtr = &temp;
        strcat(random, tempPtr);
#else
        temp[0] = randomNum + '0';
        strcat(random, temp);
#endif
    }

    return random;
}

int
main(void)
{

    srand(time(0));

// NOTE/BUG: need space for EOS terminator
#if 0
    char str_bin_bitKey[BIT_AMOUNT] = "";
    char *random = malloc(BIT_AMOUNT);
    char *bin_bitKey = malloc(BIT_AMOUNT);
#else
    char str_bin_bitKey[BIT_AMOUNT + 1] = "";
    char *random = malloc(BIT_AMOUNT + 1);
    char *bin_bitKey = malloc(BIT_AMOUNT + 1);
#endif

// NOTE/BUG: sizeof(bit_bitKey) is the size of the _pointer_ and _not_ what
// it points to (i.e. it is _not_ BIT_AMOUNT)
#if 0
    printf("\nSize of str_bin_bitKey: %ld, Size of bin_bitKey: %ld\n",
        sizeof(str_bin_bitKey), sizeof(bin_bitKey));
#endif

    // generates 4 bit long binary number
    bin_bitKey = randomBinaryGenerator(random);

    // copies 1 character less
#if 0
    memcpy(str_bin_bitKey, bin_bitKey, BIT_AMOUNT);
#else
    strcpy(str_bin_bitKey, bin_bitKey);
#endif

    // 4 bits
    printf("\nbin_bitKey:     %s\n", bin_bitKey);
    // 3 bits???
    printf("\nstr_bin_bitKey: %s\n", str_bin_bitKey);

    // unimportant for now .... convertBinaryToDecimal(bin_bitKey);
    long long dec_bitKey = 0;

    printf("\ndec_bitKey: %lld\n\n", dec_bitKey);

    free(random);

    return 0;
}

Here's a cleaned up and improved version. Note that randomBinaryGenerator is faster/better without using strcat at all.

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

#define BIT_AMOUNT 4

char *
randomBinaryGenerator(char *random)
{

    int randomNum, i;

    for (i = 0; i < BIT_AMOUNT; i++) {
        randomNum = rand() % 2;
        random[i] = randomNum + '0';
    }

    // add nul terminator
    random[i] = 0;

    return random;
}

int
main(void)
{

    srand(time(0));

    char str_bin_bitKey[BIT_AMOUNT + 1];
    char *random = malloc(BIT_AMOUNT + 1);
    char *bin_bitKey = malloc(BIT_AMOUNT + 1);

    // generates 4 bit long binary number
    bin_bitKey = randomBinaryGenerator(random);

    strcpy(str_bin_bitKey, bin_bitKey);

    // 4 bits
    printf("\nbin_bitKey:     '%s'\n", bin_bitKey);

    // 3 bits???
    printf("\nstr_bin_bitKey: '%s'\n", str_bin_bitKey);

    // unimportant for now .... convertBinaryToDecimal(bin_bitKey);
    long long dec_bitKey = 0;

    printf("\ndec_bitKey: %lld\n\n", dec_bitKey);

    free(random);

    return 0;
}

Wow man thank you so much :) I honestly find memory allocation a bit too confusing thus which results in me writing messy code. I really appreciate your help. If you don't mind could you point me in the right direction for the concepts I should read about as well as resources I should checkout and study, I love C so far, but it definitely can be a bit too complicated for me at times. God bless :) — HamzaKerem, Jun 08 '20 at 18:58
this answer has a memory leak because the memory allocated by `char *bin_bitKey = malloc(BIT_AMOUNT + 1);` is never returned via a call to `free(bin_bitKey);` — user3629249, Jun 09 '20 at 14:01

C - Memncpy/Strncpy (tried strncat too) copies 1 character less than it should, help is appreciated

1 Answers1