Copying a string - how do I have to deal with memory leaks and error cases?

Question

I am trying to implement the function int *cpy_array(int v[], int size), which copies the array in another and returns the new array as pointer. I also have to watch out for error cases and use dynamic memory.

Ok i know that malloc returns 0 when there is nout enough memory available. I was wondering if there might be any other possible errors as well which I missed out. Then I have to implement free() in the case of succsess as well as in error case. I tried to implement something like:

if (!w[i]) { 
  for (k = 0; k < i; ++k)
     free(w[k]); 
  return 0;
}

But there was always an error with this.

In file included from hot.c:2:
C:/mingw-w64/i686-8.1.0-posix-dwarf-rt_v6-rev0/mingw32/i686-w64-mingw32/include/stdlib.h:502:27: note: expected 'void *' but argument is of type 'int'"
   void __cdecl free(void *_Memory);

And I am not sure why to free() the new array or should the old array be freed? I tried to free it with pointer in my function, but didnt work either and dont think it should be in the main?

Here is the original code:

int *cpy_array(int v[], int size);

int main(void)
{
int size; 
size = 4;
int myArray[4] = {1234};
if (*cpy_array(myArray, size) == 0)
{
    printf("No memory available.");
}else{
printf("The new Array: %i", *cpy_array(myArray, size)); 
}

return 0;
}

 int *cpy_array(int v[], int size)
{
    int i;
    int *a  = malloc(size * sizeof(int));
    if(*a == 0)
    return 0;
    for (i = 0; i < size; i++)
    { 
        a[i] = v[i];
    }
return a;
}

`int myArray[4] = {1234};` -->`int myArray[4] = {1,2,3,4};` and `if(*a == 0)` -->`if(a == NULL)` — kiran Biradar, Jan 12 '19 at 15:54

Uros Krstic · Accepted Answer · 2019-01-12T18:28:51.047

In your first code snippet, you incorrectly deallocated the array of integers w. You can't free single integers in that array, but what you need to do is simply type in:

free(w);

That will free the entire array. You can also see from the text of the error - note: expected 'void *' but argument is of type 'int'" void __cdecl free(void *_Memory), that the program expected a pointer to the array and not an integer.

You can't free the old array, because it's statically created and the memory for it allocated at the start of the program and it will be freed at the end of the function in which it was defined by the program itself, so you don't need to worry about that. Whereas it's your job to free the dynamically created arrays such as the one you created through the cpy_array(int v[], int size) function.

More on the difference between static and dynamic allocation, you can look up here:

Difference between static memory allocation and dynamic memory allocation

This part of code, wouldn't proparly print the array (you will just print the first number of the array), and also you are calling the function twice, which is excessive and should be done only once for the same array.

if (*cpy_array(myArray, size) == 0)
{
    printf("No memory available.");
}else{
    printf("The new Array: %i", *cpy_array(myArray, size)); 
}

You could easify fix these problems by defining a pointer which could store the return value of the function, so you don't have to call it twice and then to correctly print the array use a for loop:

    int * copiedArray = cpy_array(myArray, size);
    if (copiedArray == NULL)
    {
        printf("No memory available.");
    }else{
        printf("The new Array: ");
        for (int i = 0; i < size; i++)
            printf("%i ", copiedArray[i]); 
    }

I noticed that you are checking whether a pointer is pointing to something or not incorrectly. Once in main:

if (*cpy_array(myArray, size) == 0)

And once in the cpy_array(int v[], int size) function:

if(*a == 0)

This will not work because you are dereferencing the pointer and checking whether the value to which it is pointing is zero. What you want to do is check the value of the pointer itself. If that is NULL then the allocation didn't work:

if (cpy_array(myArray, size) == NULL)

and

 if(a == NULL)

You should use NULL instead of zero because you are explicitly stating that you are checking a value of a pointer, and NULL may not be equal to zero on every machine.

More on that topic here:

What is the difference between NULL, '\0' and 0

Nice thank you! something which still puzzles me a little bit here is that I return the pointer and have to free them before returning? that makes no sense to me because then I would be returning nothing. I think I am mixing something here in the wrong way... — Francis Drake, Jan 12 '19 at 17:22
Well if you free before returning from the function, you will never return the copied dynamic array and you are right, that doesn't make sense. The new copied array should be freed after it's job is done and is of no more use. That makes more sense to me. — Uros Krstic, Jan 12 '19 at 17:39
So I should free it in the main after its use and not in the seperate function before return? — Francis Drake, Jan 12 '19 at 17:45
Yeah, you should. Freeing it before using it makes no sense. And also check the main answer, I added one more thing. If you have anymore questions, feel free to ask. — Uros Krstic, Jan 12 '19 at 17:59
thank you I forgot the for-printf the whole thing and was just wondering why its not working :D thank you! :) — Francis Drake, Jan 12 '19 at 18:23

bruno · Answer 2 · 2019-01-12T16:14:41.673

To detect you problems concerning the memory use valgrind, If I do that gives :

==10947== Memcheck, a memory error detector
==10947== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==10947== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==10947== Command: ./a.out
==10947== 
==10947== Conditional jump or move depends on uninitialised value(s)
==10947==    at 0x10548: cpy_array (c.c:25)
==10947==    by 0x104B3: main (c.c:11)
==10947== 
==10947== Invalid read of size 4
==10947==    at 0x104B8: main (c.c:11)
==10947==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==10947== 
==10947== 
==10947== Process terminating with default action of signal 11 (SIGSEGV)
==10947==  Access not within mapped region at address 0x0
==10947==    at 0x104B8: main (c.c:11)
==10947==  If you believe this happened as a result of a stack
==10947==  overflow in your program's main thread (unlikely but
==10947==  possible), you can try to increase the size of the
==10947==  main thread stack using the --main-stacksize= flag.
==10947==  The main thread stack size used in this run was 8388608.
==10947== 
==10947== HEAP SUMMARY:
==10947==     in use at exit: 16 bytes in 1 blocks
==10947==   total heap usage: 1 allocs, 0 frees, 16 bytes allocated
==10947== 
==10947== 16 bytes in 1 blocks are definitely lost in loss record 1 of 1
==10947==    at 0x4847568: malloc (vg_replace_malloc.c:299)
==10947==    by 0x10533: cpy_array (c.c:24)
==10947==    by 0x104B3: main (c.c:11)
==10947== 
==10947== LEAK SUMMARY:
==10947==    definitely lost: 16 bytes in 1 blocks
==10947==    indirectly lost: 0 bytes in 0 blocks
==10947==      possibly lost: 0 bytes in 0 blocks
==10947==    still reachable: 0 bytes in 0 blocks
==10947==         suppressed: 0 bytes in 0 blocks
==10947== 
==10947== For counts of detected and suppressed errors, rerun with: -v
==10947== Use --track-origins=yes to see where uninitialised values come from
==10947== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 6 from 3)

The "Conditional jump or move depends on uninitialised value(s)" comes from the *a in if(*a == 0) and "Invalid read of size 4 ..." because you dereference 0 because of the return 0;

after changing if(*a == 0) to if(a == 0) to solve the two previous problems that condition is (a priori) false and _valgrind says :

==11116== Memcheck, a memory error detector
==11116== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==11116== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==11116== Command: ./a.out
==11116== 
Mein neuer Array enthaelt folgende Zeichen: 1==11116== 
==11116== HEAP SUMMARY:
==11116==     in use at exit: 32 bytes in 2 blocks
==11116==   total heap usage: 3 allocs, 1 frees, 1,056 bytes allocated
==11116== 
==11116== 16 bytes in 1 blocks are definitely lost in loss record 1 of 2
==11116==    at 0x4847568: malloc (vg_replace_malloc.c:299)
==11116==    by 0x10523: cpy_array (c.c:24)
==11116==    by 0x104A3: main (c.c:11)
==11116== 
==11116== 16 bytes in 1 blocks are definitely lost in loss record 2 of 2
==11116==    at 0x4847568: malloc (vg_replace_malloc.c:299)
==11116==    by 0x10523: cpy_array (c.c:24)
==11116==    by 0x104CF: main (c.c:15)
==11116== 
==11116== LEAK SUMMARY:
==11116==    definitely lost: 32 bytes in 2 blocks
==11116==    indirectly lost: 0 bytes in 0 blocks
==11116==      possibly lost: 0 bytes in 0 blocks
==11116==    still reachable: 0 bytes in 0 blocks
==11116==         suppressed: 0 bytes in 0 blocks
==11116== 
==11116== For counts of detected and suppressed errors, rerun with: -v
==11116== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 6 from 3)

so yes you have memory leaks because you lost 2 times the allocation return by cpy_array

you need to have something like :

int * v = cpy_array(myArray, size);

if (*v == 0)
{
  printf("Speicher kann nicht freigegeben werden.");
}else{
  printf("Mein neuer Array enthaelt folgende Zeichen: %i", 
   *v);    
}

free(v);

Doing that correction valgrind signals nothing :

valgrind --leak-check=full  ./a.out
==11224== Memcheck, a memory error detector
==11224== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==11224== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==11224== Command: ./a.out
==11224== 
Mein neuer Array enthaelt folgende Zeichen: 1==11224== 
==11224== HEAP SUMMARY:
==11224==     in use at exit: 0 bytes in 0 blocks
==11224==   total heap usage: 2 allocs, 2 frees, 1,040 bytes allocated
==11224== 
==11224== All heap blocks were freed -- no leaks are possible
==11224== 
==11224== For counts of detected and suppressed errors, rerun with: -v
==11224== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 6 from 3)

I encourage you to

compile with all warning detection, like gcc -g -Wall -pedantic ...
when you have problem use valgrind and/or debugger
even you do not see a problem run anyway under valgrind, so problems can be hidden

I ve never worked with something like this... How do I use this in a sufficient way? — Francis Drake, Jan 12 '19 at 16:07
supposing the program is `a.out` rather than to start it doing in a shell `./a.out` just do `valgrind --leak-check=full ./a.out`. Of course nistall _valgrind_ before :-) — bruno, Jan 12 '19 at 16:09
@GermanNerd what do you mean ? it is useless to see the errors ? if the O.P. need help that means he doesn't see what is append. To show him the advantage of _valgrind_ is much more than to just signale where the errors are in is code, because he will be able to reuse _valgrind_ in the future, no ? — bruno, Jan 12 '19 at 16:28
@FrancisDrake I forgot to say I corrected the initialization to be `int myArray[4] = { 1,2,3,4 };` — bruno, Jan 12 '19 at 16:32
@bruno It appears to me that the OP is almost clueless concerning the C language, and maybe programming in general, judging by the code he posted. And I don't think that pages of problems reported by valgrind can help him/her in the slightest to acquire the fundamental knowledge to understand the C language, not to speak of the concepts it represents. Sure, at some point it is useful to learn how to use valgrind - after one has enough knowledge to make sense of what valgrind reports. But that is just my opinion. — GermanNerd, Jan 12 '19 at 17:25
I have to admit that its somewhat hard for me to understand every line you posted. It seems to me that he is showing me what is actually going on in the memory with every command, but I dont get every line though your corrections make more sense for me now after reading some answers and little more on the internet. — Francis Drake, Jan 12 '19 at 17:52
in your version _cpy_array_ allocates memory in the heap (_malloc_) and returns it, in _main_ with that value you just access to the first element (`*cpy_array(myArray, size)`) an nothing more, so the allocated memory is not anymore available and you do not free it, that memory is definitively lost, this is a _memory leak_. As you can see in my version I save the pointer, access to the first element as you did then I free it, there is no memory leak. — bruno, Jan 12 '19 at 18:22

score 0 · Answer 3 · answered Jan 12 '19 at 16:29

0

this is not the correct way to initialize the array

int myArray[4] = {1234};

write

int myArray[4] = { 1,2,3,4 };

or simply

int myArray[] = { 1,2,3,4 };

Calling the function cpy_array .. when you write

if (*cpy_array(myArray, size) == 0)

Is not correct, why? because what if the function returns NULL, then you are dereferencing NULL

In your function cpy_array you are dereferencing a, that is not correct, instead compare the pointer

if ( a == NULL)

and use the standard constant NULL for a null pointer instead of 0 since it may not be 0 on all platforms.

answered Jan 12 '19 at 16:29

AndersK

35,813
6
60
86

Ok nice thank you! I dont get how you exactly mean that I would be dereferencing the pointer with == because I thought it only checks for the condition if the return equals NULL – Francis Drake Jan 12 '19 at 17:58
if you have a pointer `p` and set it to point to NULL (or 0 as you wrote) then it is not pointing anywhere, if you dereference it i.e. `*p` you are trying to access memory that does not exist which is undefined behavior. your function cpy_array could return NULL (0) if not enough memory could be allocated. – AndersK Jan 12 '19 at 20:48

Copying a string - how do I have to deal with memory leaks and error cases?

3 Answers3