1

I am iterating through an integer array and trying to find the elements that are non-zero and getting the count. Here is my complete code.

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

int countelem(int *a, int a_length){
     int i, count = 0;
     for (i=0; i<a_length; i++){
         if (a[i] != 0) {
             printf("element number: %d and element is: %d\n", i, a[i]);
             count ++;
         };
     };
     return count;
 }

int main(){
    int count, a_length = 5;
    int *ptr_a, a[a_length];
    ptr_a = calloc(a_length, 4);
    ptr_a = a;
    a[0] = 1;
    count = countelem(ptr_a, a_length);
    printf("number of non zeroes %d\n", count);
    if (ptr_a){
        printf("needs to be freed\n");
        free(ptr_a);
    }
    return 0;
}

I am compiling using the command

cc -Wall -std=c99  con.c -o con

while running ./con I am basically encountering two issues.

  1. In the function countelem in the statement if (a[i] != 0) a[i] is producing irrelevant results for uninitialized elements.
  2. Since I am allocating memory to the pointer ptr_a why the call free(ptr_a) causing error pointer being freed was not allocated.

Here is the stdout

element number: 0 and element is: 1
element number: 1 and element is: 32767
element number: 2 and element is: 185925632
element number: 3 and element is: 1
number of non zeroes 4
needs to be freed
con(535,0x7ffff180a3c0) malloc: *** error for object 0x7fff54aaefe0: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
Abort trap: 6

Your help is greatly appreciated.

NB. One interesting thing that I have noticed that if I am using array of double instead of int, a[i] delivers correct output.

melpomene
  • 84,125
  • 8
  • 85
  • 148
SreeT
  • 21
  • 1
  • `ptr_a = calloc(...); ptr_a = ...` is a memory leak. You've just overwritten the only pointer to the block that `calloc` created. – melpomene Oct 28 '18 at 00:23
  • "*I am iterating through an integer array and trying to find the elements that are non-zero and getting the count.* Why? If allocated via `calloc()` all were `0`, else the weren't initialised so you may not test them as this would result in undefined behaviour. – alk Oct 28 '18 at 12:28

2 Answers2

1

Well... "What were you thinking?"

    int *ptr_a, a[a_length];

Let's look at two lines and see if we can make sense of what you are doing:

    ptr_a = calloc(a_length, 4);
    ptr_a = a;

Line 1 above allocates a block of memory with calloc sizing the block for a_length members of 4-bytes each for a total of 20-bytes given a_length = 5;. It then assigns the starting address for the new block of memory to ptr_a.

Line 2 above, then assigns the address of the array a (with automatic storage type) to ptr_a overwriting the memory address for the block of memory you just allocated (thereby causing a memory leak, because there a no more references to the start of that new block meaning it can never be freed).

However, since ptr_a now points to a memory address that was NOT previously allocated with malloc, calloc or realloc, when you pass ptr_a to free (ptr_a); boom! SegFault.

Using Memory Allocated by calloc for Storage

You do not need the VLA (Variable Length Array) a at all. You allocate a block of memory sufficient to hold five integer values and assign the starting address of that block to ptr_a. You simply use ptr_a as you attempted to use a, e.g.

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

#define NELEMENTS 5 /* if you need a constant, #define one (or more) */

int countelem (int *a, int a_length)
{
    int i, count = 0;

    for (i = 0; i < a_length; i++)
        if (a[i] != 0) {
            printf ("element number: %d and element is: %d\n", i, a[i]);
            count++;
        }  /* no ; following block closure */

    return count;
}

int main (void) {

    int count = 0,              /* initialize all variables */
        a_length = NELEMENTS,
        *ptr_a = NULL;

    ptr_a = calloc(a_length, sizeof *ptr_a);     /* allocate block of mem */
    if (ptr_a == NULL) {    /* validate & handle error before using block */
        perror ("calloc-ptr_a");
        return 1;
    }
    ptr_a[0] = 1;   /* assign value 1 as first value in block of mem */

    count = countelem (ptr_a, a_length);

    printf ("number of non zeroes %d\n", count);
    free(ptr_a);    /* you validated the block above, just free */

    return 0;
}

Example Use/Output

$ ./bin/alloccount
element number: 0 and element is: 1
number of non zeroes 1

Memory Use/Error Check

In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed.

It is imperative that you use a memory error checking program to insure you do not attempt to access memory or write beyond/outside the bounds of your allocated block, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.

For Linux valgrind is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.

$ valgrind ./bin/alloccount
==7530== Memcheck, a memory error detector
==7530== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==7530== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==7530== Command: ./bin/alloccount
==7530==
element number: 0 and element is: 1
number of non zeroes 1
==7530==
==7530== HEAP SUMMARY:
==7530==     in use at exit: 0 bytes in 0 blocks
==7530==   total heap usage: 1 allocs, 1 frees, 20 bytes allocated
==7530==
==7530== All heap blocks were freed -- no leaks are possible
==7530==
==7530== For counts of detected and suppressed errors, rerun with: -v
==7530== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Always confirm that you have freed all memory you have allocated and that there are no memory errors.

Using the VLA for Storage

Conversely, if you did want to make use of your VLA, then there is no need to allocate memory with calloc. It's a one or the other, but not both, proposition.

Regardless of whether storage is provided by the VLA or calloc, you can still use ptr_a to point to that storage. However, if storage is provided by the VLA, then don't allocate with calloc, simply:

int a[a_length], *ptr_a = a;

That's all that is required to declare a VLA and then a pointer to the VLA. (note: the VLA values are indeterminate, so before use, you may want to include string.h and then memset (a, 0, sizeof a);)

If you use your variable-length array as storage instead of allocating with calloc, your code simplifies to:

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

#define NELEMENTS 5 /* if you need a constant, #define one (or more) */

int countelem (int *a, int a_length)
{
    int i, count = 0;

    for (i = 0; i < a_length; i++)
        if (a[i] != 0) {
            printf ("element number: %d and element is: %d\n", i, a[i]);
            count++;
        }  /* no ; following block closure */

    return count;
}

int main (void) {

    int count = 0,              /* initialize all variables */
        a_length = NELEMENTS,
        a[a_length], *ptr_a = a;

    memset (a, 0, sizeof a);    /* initialize a all zero */
    ptr_a[0] = 1;               /* assign value 1 as first element */

    count = countelem (ptr_a, a_length);

    printf ("number of non zeroes %d\n", count);

    return 0;
}

(note: the addition of memset as discussed above. Without it, any attempt to access the indeterminate values in a[1]- a[4] would result in Undefined Behavior (and funky values output as you found))

Simply removing a_length and using NELEMENTS instead removes the VLA and instead provides storage with a normal array which you can initialize when it is declared, e.g.

    int count = 0,              /* initialize all variables */
        a[NELEMENTS] = {0},
        *ptr_a = a;
    ...
    count = countelem (ptr_a, NELEMENTS);

(the output is the same, but no need for string.h or memset or any need to run it through a memory/error check)

Enable Additional Warnings (-Wextra at minimum)

At minimum, add -Wextra for gcc/clang (though you should additionally add -pedantic -Wshadow as well) for VS use /W3. Do not accept code until it compiles cleanly -- without a single warning. Listen to what your compiler is telling you, read and understand the warnings. It will give you the precise line on which each problematic bit of code is found. Go fix it before going further. Simply resolving all warnings can and will eliminate a majority of the problems you will find yourself spending a lot of time resolving otherwise.

Look things over and let me know if you have any questions.

David C. Rankin
  • 81,885
  • 6
  • 58
  • 85
0

You have assigned ptr_a to point to a, an uninitialised array on the stack rhat could contain any random values.

Luke McCarthy
  • 879
  • 2
  • 9
  • 21
  • ok, I assume that a `static` classifier to the array declaration would be sufficient. or there is anything else. – SreeT Oct 28 '18 at 01:09