int pointer as argument in function valgrind error Invalid read of size 4

Question

What is wrong in this simple code? It works, but valgrind shows errors. How should it look like?

What should be definition of int func(int*)?

main.cpp:

#include <iostream>
#include <stdio.h>
#include <cstdlib>

using namespace std;

int func(int* terminal);

int main()
{
    int* x = (int*)malloc(3);
    x[0]=1;
    x[1]=2;
    x[2]=3;

    func(x);

}

int func(int *terminal)
{
    cout<<(*terminal)<<endl;
    cout<<(*terminal+1)<<endl;
    cout<<(*terminal+2)<<endl;

    return 1;
}

valgrind log:

==2806== Invalid write of size 4
==2806==    at 0x80486A6: main (main.cpp:12)
==2806==  Address 0x434f028 is 0 bytes inside a block of size 3 alloc'd
==2806==    at 0x402A17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==2806==    by 0x804869D: main (main.cpp:11)
==2806== 
==2806== Invalid write of size 4
==2806==    at 0x80486B3: main (main.cpp:13)
==2806==  Address 0x434f02c is 1 bytes after a block of size 3 alloc'd
==2806==    at 0x402A17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==2806==    by 0x804869D: main (main.cpp:11)
==2806== 
==2806== Invalid write of size 4
==2806==    at 0x80486C0: main (main.cpp:14)
==2806==  Address 0x434f030 is 5 bytes after a block of size 3 alloc'd
==2806==    at 0x402A17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==2806==    by 0x804869D: main (main.cpp:11)
==2806== 
==2806== Invalid read of size 4
==2806==    at 0x80486F1: func(int*) (main.cpp:22)
==2806==    by 0x80486D1: main (main.cpp:16)
==2806==  Address 0x434f028 is 0 bytes inside a block of size 3 alloc'd
==2806==    at 0x402A17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==2806==    by 0x804869D: main (main.cpp:11)
==2806== 
1
==2806== Invalid read of size 4
==2806==    at 0x804871A: func(int*) (main.cpp:23)
==2806==    by 0x80486D1: main (main.cpp:16)
==2806==  Address 0x434f028 is 0 bytes inside a block of size 3 alloc'd
==2806==    at 0x402A17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==2806==    by 0x804869D: main (main.cpp:11)
==2806== 
2
==2806== Invalid read of size 4
==2806==    at 0x8048746: func(int*) (main.cpp:24)
==2806==    by 0x80486D1: main (main.cpp:16)
==2806==  Address 0x434f028 is 0 bytes inside a block of size 3 alloc'd
==2806==    at 0x402A17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==2806==    by 0x804869D: main (main.cpp:11)
==2806==

Maybe I should go fishing instead of studying c++.

How about allocating for `malloc(3*sizeof(int))`? You should always read errors from the top down. It first says `invalid write`. Doesn't running the program produce a segmentation fault? — Andras Deak -- Слава Україні, Aug 13 '15 at 23:50
Thanks it works. Sorry for silly question. There was no segmentation fault. — , Aug 13 '15 at 23:53
@aschepler thanks, I was just going to, but twentylemon beat me to it, with a better answer. — Andras Deak -- Слава Україні, Aug 13 '15 at 23:53
yes ! forget about C's `malloc()` and use C++'s `new` instead. Not only takes it the right size and avoids an unnecessary casting, but for classes it will return objects constructed with the appropriate constructor instead of just an unitialized memory region. — Christophe, Aug 13 '15 at 23:56
@Christophe then you should abandon `new` altogether and use smart pointers or containers in order to avoid memory leaks. — Andras Deak -- Слава Україні, Aug 13 '15 at 23:58
@AndrasDeak that's an interesting remark, but another discussion (By the way, I'd rather opt for a vector here instead of a smart pointer). The problem with malloc() is not a question of style of improved design, but a problem of correctness: putting a non trivial object into a malloced region requires a placement new, whereas most people just use an ordinary assignment. — Christophe, Aug 14 '15 at 00:10
@Christophe I perfectly agree. But once we're at "using a completely different approach that you thought was needed", it could be instructive to teach the asker an approach that is (more) bulletproof (especially that OP was missing the `free`s in the first place, as @CoffeandCode also pointed out). — Andras Deak -- Слава Україні, Aug 14 '15 at 00:12

score 3 · Accepted Answer · edited Aug 13 '15 at 23:55

3

malloc takes the number of bytes to allocate; it doesn't know anything about the types. You'd have to do malloc(3 * sizeof(int)) or better, use

int* x = new int[3];

edited Aug 13 '15 at 23:55

Jonathan Leffler

730,956
141
904
1,278

answered Aug 13 '15 at 23:52

twentylemon

1,248
9
11

2

Or even better, `std::vector x = {1,2,3};` – aschepler Aug 13 '15 at 23:54
True, either vector or `std::array` would be preferable for arrays. For generic pointers though, `new` beats `malloc`. – twentylemon Aug 13 '15 at 23:55

score 0 · Answer 2 · edited Aug 14 '15 at 00:02

0

int* x = (int*)malloc(3);

allocates 3 bytes, not enough for 3 ints.

Since you are using C++, you can use the new operator.

int* x = new int[3];

If you must use malloc, use:

int* x = (int*)malloc(3*sizeof(int));

edited Aug 14 '15 at 00:02

Lightness Races in Orbit

378,754
76
643
1,055

answered Aug 13 '15 at 23:54

R Sahu

204,454
14
159
270

Daniel Jour · Answer 3 · 2015-08-14T00:15:07.950

malloc(3)

You allocate memory of the size of 3 bytes. A single int may be 2, 4 or even 8 bytes in size.

So if you were using malloc (please don't do that - when writing C++ code - ever again) the correct call would be

malloc(3 * sizeof(int));

But this is C++, so the straight forward way would be to use

int * array = new int[3]{};

or, when you've learned once how manual memory management works:

std::vector<int> array{3};

Or, since you know at compile time that you want 3 elements:

std::array<int,3> array;
// int array[3]; is the raw array, only
// use it to learn what it is and why you should avoid it

And, finally, talking about manual memory management: Don't forget to free your resources:

// if you where using malloc
free(pointer);

// otherwise, if you were using new []
delete [] pointer;

std::vector manages the memory for you, and std::array lives on the stack (and has thus automatic storage duration).

No. You allocate an array, so you need to delete an array. http://stackoverflow.com/q/703691/1116364 — Daniel Jour, Aug 14 '15 at 00:12

int pointer as argument in function valgrind error Invalid read of size 4

3 Answers3