malloc is failing to allocate memory

Question

I'm writing a C Code to solve Euler equations. My code works perfectly fine on the cluster but not on my pc. Seems to be an issue with malloc(). It is not being able to allocate the requested memory and fails.

How do i make it work? is it something to do with Defragmentation? But the system settings show (0% Defragmented).

Just including a part of malloc() code here.

double **u, **rho_u, **rho,    
int Size = 1000;
u = (double**)malloc(Size*sizeof(double*));
for(i=0;i<=Size;i++)
    u[i] = (double*)malloc(Size*sizeof(double));

rho_u = (double**)malloc(Size*sizeof(double*));
for(i=0;i<=Size;i++)
    rho_u[i] = (double*)malloc(Size*sizeof(double));

You are roughly allocating about 16MB here. Should be no problem on an average pc. What exactly fails? — Ctx, Dec 23 '18 at 16:31
As a general rule, when you begin suspecting things like fragmentation, runtime library functions, or the operating system, immediately discard those suspicions and look for the bug in your code. — Carey Gregory, Dec 23 '18 at 16:38
If the posted code were to be checking (!=NULL) the returned value from a call to `malloc()` and when it fails, then call `perror()` with an appropriate error message, then the OP would know what actually happened. Also, in C, the returned type is `void*` which can be assigned to any pointer, Casting just clutters the code, making it more difficult to understand, debug, etc — user3629249, Dec 23 '18 at 19:02
*part of malloc() code*, Does this mean there is even more calls to `malloc()`? — user3629249, Dec 23 '18 at 19:04

score 5 · Accepted Answer · answered Dec 23 '18 at 16:34

5

You probably corrupt your heap here:

for(i=0;i<=Size;i++)
    u[i] = (double*)malloc(Size*sizeof(double));

You assign 1001 pointers, but only have 1000 allocated. Correct version:

for(i=0;i<Size;i++)
    u[i] = (double*)malloc(Size*sizeof(double));

Same for the second loop.

answered Dec 23 '18 at 16:34

Ctx

18,090
24
36
51

It was such a stupid mistake. It's working fine now on pc. As it was running fine on server, I thought there was nothing wrong in the code. – Black Heart Dec 23 '18 at 16:44
2

@BlackHeart Yes, undefined behaviour is ugly, it may seem to work at one moment and then fail in the other. – Ctx Dec 23 '18 at 16:45

Basile Starynkevitch · Answer 2 · 2018-12-23T16:39:25.173

4

Read carefully the documentation of malloc. It can fail, and when it does fail, malloc returns NULL (and the failure reason is given by errno which you often display using perror).

So you should test against failure of malloc. The typical code is at least:

u = (double**)malloc(Size*sizeof(double*));
if (u==NULL) { perror ("malloc u"); exit(EXIT_FAILURE); };

and likewise for your rho_u and every rho_u[i]

Some operating systems may provide memory overcomitment. It is a feature I dislike.

Consider initializing every memory zone entirely. And using memory outside of a valid memory zone (or a valid address) is undefined behavior (and your program have one, noticed by Ctx's answer). Be scared.

I also recommend using valgrind. It is a very handy tool to hunt memory related bugs, and could have detected yours.

edited Dec 23 '18 at 16:39

answered Dec 23 '18 at 16:33

Basile Starynkevitch

223,805
18
296
547

1

All good advice but it's a simple buffer overrun bug as shown in Ctx's answer. – Carey Gregory Dec 23 '18 at 16:40
2

Sure, @CareyGregory, but by following this advice the OP could have been confident in rejecting their initial premise that `malloc` was failing. They might then have studied their logic a little more carefully and found the real issue for themselves. – John Bollinger Dec 23 '18 at 16:45

Achal · Answer 3 · 2018-12-23T16:50:00.307

Observation :

Avoid typecasting malloc(), Read Do I cast the result of malloc?
Check the return value of malloc() & do proper error handling.
Change loop condition from i<=Size to i<Size as it cause buffer overrun since earlier memory was allocated only for Size rows not size+1 rows.

Try this version :

int Size = 1000;
double **u = malloc(Size * sizeof(*u)); /* typecasting is not needed */
if(u == NULL) {
   /* @TODO error handling */
}
for(i=0;i<Size;i++) { /* loop should rotate only 1000 times not 1001 times */
    u[i] = malloc(Size * sizeof(**u));
    if(u[i] == NULL) {
         /* @TODO error handling */
    }
}

Similarly for rho_u and rho.

The answer from "Ron Burk" in your linked question has many valid points too, it is "primarily opinion based" if you choose the pro's or the con's of casting the result of malloc to have more weight. — Ctx, Dec 23 '18 at 16:51

malloc is failing to allocate memory

3 Answers3