0

I have a file which stored a sequence of integers. The number of total integers is unknown, so I keep using malloc() to apply new memory if i read an integer from the file. I don't know if i could keep asking for memory and add them at the end of the array. The Xcode keeps warning me that 'EXC_BAD_EXCESS' in the line of malloc(). How could i do this if i keep reading integers from a file?

int main()
{
    //1.read from file
    int *a = NULL;
    int size=0;
    //char ch;
    FILE *in;

    //open file
    if ( (in=fopen("/Users/NUO/Desktop/in.text","r")) == NULL){
        printf("cannot open input file\n");
        exit(0);    //if file open fail, stop the program
    }

    while( ! feof(in) ){
        a = (int *)malloc(sizeof(int));
        fscanf(in,"%d", &a[size] );;
        printf("a[i]=%d\n",a[size]);
        size++;
    }
fclose(in);
return 0;
}
NUO
  • 247
  • 2
  • 3
  • 14
  • How many integers do you have when you get this error? – Ingrid Morstrad Oct 28 '15 at 21:18
  • By checking the pointer returned by `malloc`? It's an essential basic test. – Weather Vane Oct 28 '15 at 21:19
  • 1
    Bad UB. You always allocate memory for exactly 1 `int`, then you attempt to treat it as an array of `ints` and access the size-th element. On top of that you have a memory leak as you're not freeing `a`. Also, `while(!feof( ))` is always wrong: http://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong – user4520 Oct 28 '15 at 21:20
  • 2
    Aside: your use of `feof` is bad, even though it's been used in answers below: see http://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong – Weather Vane Oct 28 '15 at 21:21
  • 1
    @szczurcio: Is there any good UB? – too honest for this site Oct 28 '15 at 21:40

5 Answers5

5

Calling malloc() repeatedly like that doesn't do what you think it does. Each time malloc(sizeof(int)) is called, it allocates a separate, new block of memory that's only large enough for one integer. Writing to a[size] ends up writing off the end of that array for every value past the first one.

What you want here is the realloc() function, e.g.

a = realloc(a, sizeof(int) * (size + 1));
if (a == NULL) { ... handle error ... }

Reworking your code such that size is actually the size of the array, rather than its last index, would help simplify this code, but that's neither here nor there.

2
  1. Instead of using malloc, use realloc.
  2. Don't use feof(in) in a while loop. See why.
int number;
while( fscanf(in, "%d", &number) == 1 ){
    a = realloc(a, sizeof(int)*(size+1));
    if ( a == NULL )
    {
       // Problem.
       exit(0);
    }
    a[size] = number;
    printf("a[i]=%d\n", a[size]);
    size++;
}
Community
  • 1
  • 1
R Sahu
  • 204,454
  • 14
  • 159
  • 270
2

Your malloc() is overwriting your previous storage with just enough space for a single integer!

a = (int *)malloc(sizeof(int));
 ^^^ assignment overwrites what you have stored!

Instead, realloc() the array:

a = realloc(a, sizeof(int)*(size+1));
Ryan
  • 14,392
  • 8
  • 62
  • 102
chrisaycock
  • 36,470
  • 14
  • 88
  • 125
0

You haven't allocated an array of integers, you've allocated one integer here. So you'll need to allocate a default array size, then resize if you're about to over run. This will resize it by 2 each time it is full. Might not be in your best interest to resize it this way, but you could also reallocate each for each additional field.

size_t size = 0;
size_t current_size = 2;

a = (int *)malloc(sizeof(int) * current_size);
    if(!a)
        handle_error();


while( ! feof(in) ){
    if(size >= current_size) {
        current_size *= 2;
        a = (int *)realloc(a, sizeof(int) * current_size);
        if(!a)
            handle_error();
    }
    fscanf(in,"%d", &a[size] );;
    printf("a[i]=%d\n",a[size]);
    size++;
}
Ryan
  • 14,392
  • 8
  • 62
  • 102
0

The usual approach is to allocate some amount of space at first (large enough to cover most of your cases), then double it as necessary, using the realloc function.

An example:

#define INITIAL_ALLOCATED 32 // or enough to cover most cases
...
size_t allocated = INITIAL_ALLOCATED;
size_t size = 0;
...
int *a = malloc( sizeof *a * allocated );
if ( !a )
  // panic

int val;

while ( fscanf( in, "%d", &val ) == 1 )
{
  if ( size == allocated )
  {
    int *tmp = realloc( a, sizeof *a * allocated * 2 ); // double the size of the buffer
    if ( tmp )
    {
      a = tmp;
      allocated *= 2;
    }
    else
    {
      // realloc failed - you can treat this as a fatal error, or you
      // can give the user a choice to continue with the data that's 
      // been read so far.  
    }
    a[size++] = val;
  }
}

We start by allocating 32 elements to a. Then we read a value from the file. If we're not at the end of the array (size is not equal to allocated), we add that value to the end of the array. If we are at the end of the array, we then double the size of it using realloc. If the realloc call succeeds, we update the allocated variable to keep track of the new size and add the value to the array. We keep going until we reach the end of the input file.

Doubling the size of the array each time we reach the limit reduces the total number of realloc calls, which can save performance if you're loading a lot of values.

Note that I assigned the result of realloc to a different variable tmp. realloc will return NULL if it cannot extend the array for any reason. If we assign that NULL value to a, we lose our reference to the memory that was allocated before, causing a memory leak.

Note also that we check the result of fscanf instead of calling feof, since feof won't return true until after we've already tried to read past the end of the file.

John Bode
  • 119,563
  • 19
  • 122
  • 198
  • Your code has undefined behavior -- `sizeof *a` without *a being declared in the call to `malloc` – Ryan Oct 28 '15 at 21:45
  • @self this usage is correct; `a` is in scope as soon as `int *a =` appears (objects name may be used in their initializer), and since the operand of `sizeof` is unevaluated, `sizeof *a` does not invoke UB. In fact this is a common idiom, we can be sure that `p = malloc(N * sizeof *p);` allocates the right number of bytes for N objects, and there's no reason to stop using this if it happens to be done as initialization instead of assignment. – M.M Oct 28 '15 at 22:20