C Program works for me but shows runtime error online

Question

The following code worked fine for me (code blocks 10.05) and showed no compile-time/runtime errors for various test cases. But showed runtime error as I submitted it online on a programming website.

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

/*

 Here comes newPos()

*/
int main()

{
int t,i,n,k,j;
scanf("%d",&t);
int* a;

for(i=0;i<t;i++)
{
    scanf("%d",&n);
    free(a);

    a=(int*) malloc(n);

    for(j=0;j<n;j++)
        scanf("%d",&a[j]);
    scanf("%d",&k);

    printf("%d\n",newPos(a,n,k));

}


return 0;
}

And then I changed it into a .cpp file after making a few changes. i.e., instead of free(a) I used the statement, delete a; and instead of a=(int*) malloc(n), I used the statement, a=new int[n]; Then it executed successfully both on my compiler and online.

At the first iteration you are freeing a dangling pointer. Initialize `a` to `NULL`. — Matteo Italia, Nov 14 '13 at 13:52
As you've found out 'works for me' != 'free of bugs'. It's a very important lesson for C or C++ programmers. — john, Nov 14 '13 at 13:53
FYI, in C++ you should use `delete[]` (not `delete`) after `new T[n]`. — interjay, Nov 14 '13 at 14:07
There really is no c++ in this code. Perhaps the c++ tag should be removed. — drescherjm, Nov 14 '13 at 14:26
@drescherjm Below the program i have included how i changed the dynamic allocation to c++ and it worked. — user2983237, Nov 14 '13 at 15:35
@all the C++ part is where I changed the malloc and free to new and delete. its below the program. — user2983237, Nov 14 '13 at 15:37

score 6 · Accepted Answer · edited May 23 '17 at 11:58

6

First error:

You are not allocating enough memory to store n integer values. So you should change:

a=(int*) malloc(n);

to:

a=malloc(n * sizeof(int));

I have also removed the cast since it's useless and could hide a forgotten include.

Second error:

You must not free a before allocating memory. Free the memory only at the end of your loop.

C/C++ mix:

In the comments of this answer, people are talking about the need or not to cast, in particular in C++. In C, you should not cast.

If you are willing to do C++ code, you should use new and delete instead of malloc and free. Honestly, I don't know if a cast is needed in C++ when using malloc, because in C++, I always use new. But please, don't write C code with a C++ compiler. Choose between C and C++ depending on your needs.

edited May 23 '17 at 11:58

Community

1
1

answered Nov 14 '13 at 13:50

Maxime Chéramy

16,409
7
51
73

That is not the problem. – Sadique Nov 14 '13 at 13:53
@Acme I don't understand. – Maxime Chéramy Nov 14 '13 at 13:53
the reason you stated is not the cause of the problem - see here - http://ideone.com/O4xkMj – Sadique Nov 14 '13 at 13:55
2

@Acme: Neither problem is "the" problem, since there are at least two; but this is certainly one of them. – Mike Seymour Nov 14 '13 at 13:55
I see 2 problems: the malloc too small and the free of a before allocating. What's wrong?! – Maxime Chéramy Nov 14 '13 at 13:56
@MikeSeymour - the runtime error is not because of the cast which i want to say. – Sadique Nov 14 '13 at 13:57
2

@Acme The problem is not the cast, it's the argument to `malloc`. – interjay Nov 14 '13 at 13:57
1

@Acme: Sorry, it wasn't clear what you meant by "that". There's no reason not to remove the pointless cast though. – Mike Seymour Nov 14 '13 at 13:59
@MikeSeymour - the answer initially did not contain any information regarding the wrong malloc argument and freeing of `a` before allocating - its been edited way later after other answers were posted. – Sadique Nov 14 '13 at 14:01
@Acme I disagree, I only forgot the free in the initial answer. – Maxime Chéramy Nov 14 '13 at 14:02
@Maxime - Your answer did not contain that info before and that is why my first comment says what i said - you still have not noticed that the question is tagged `C` and `C++` both - your answer is still incomplete. In C++ the cast `is required` – Sadique Nov 14 '13 at 14:03
1

@Acme If you read the question, it says that in C++ OP used `new` instead of `malloc`. So the cast isn't needed with `malloc` because it was only used in C. – interjay Nov 14 '13 at 14:06
@Acme, my initial answer said to change to `a=malloc(n * sizeof(int));`. And that's why I also said I didn't understand your comment. You read too fast. – Maxime Chéramy Nov 14 '13 at 14:06
We are talking language specific and not code specific. Using malloc with a cast is perfectly valid in C++. – Sadique Nov 14 '13 at 14:07
1

If you are using C++, then you should be using `new` and `delete`, not `malloc` and `free` (and probably not use `printf`, but `cout`). – Mats Petersson Nov 14 '13 at 14:07
@MatsPetersson - When did i say what one should do - even the above answer does not mention what you said. – Sadique Nov 14 '13 at 14:10
@Acme thank you for your suggestions for improving my answer. I am not the one who downvoted your answer though (I would not downvote a correct answer). – Maxime Chéramy Nov 14 '13 at 14:18
@Maxime - :) no harsh feelings. – Sadique Nov 14 '13 at 14:18

score 3 · Answer 2 · edited May 23 '17 at 11:58

You are freeing before allocating:

free(a); // This can lead to Undefined Behavior because a is containing some junk value
a=(int*) malloc(n);

Also, no specific need to cast return type of malloc and check your malloc argument you are not specifying size in bytes correctly. But in C++ the case is required (Since you tagged both C and C++).

Do I cast the result of malloc?

 a=(int*) malloc(n*sizeof(int));

score 2 · Answer 3 · answered Nov 14 '13 at 13:52

2

Aside from the mentioned allocation size problem, you can't free(a) unless you have either already allocated something, or have initialized a to have the value NULL.

answered Nov 14 '13 at 13:52

Mats Petersson

123,518
13
127
216

score 0 · Answer 4 · edited May 23 '17 at 11:43

This is because your argument to malloc() is wrong. The function has no idea what "unit" you're going to be using, so the unit for the argument is always "bytes". With C++'s new[] operator, that's operating a higher level in the language so it can take the type's size into account.

Thus, change your allocation to:

a = malloc(n * sizeof *a);

This removes the pointless and annoying cast, and also adds the missing sizeof to scale the argument by the number of bytes in each pointed-to object. This is a good form of malloc() usage to remember.

Also don't free() random pointers.

C Program works for me but shows runtime error online

4 Answers4

First error:

Second error:

C/C++ mix: