malloc creates seg faults?

Hi,

For an assignement that I have, I have to write a function that is used
for duplicating arrays of some objects (called Product). It is supposed
to be something similar to strndup, but taking a Product* as an argument
instead of a char*, and of course the length of the array.

However, at some point in my code, where I have to call malloc to
allocate the required memory, I get a Segmentation Fault. Here's what I
have written:

Product* prodndup(const Product* src, size_t n)
{
int length = n * sizeof(Product); // size of memory to be allocated
Product* retval; // pointer to be returned

retval = (Product*) malloc(length); // <= this is the point

if (retval == NULL) return NULL;

Product *p = retval;

while (n > 0)
{
*p = *src;
p++;
src++;
n--;
}


return retval;
}

When I uncomment the marked line, the program terminates (although of
course, without doing what it is supposed to do).

What am I doing wrong? Isn't this how malloc is supposed to be called?

Thanks,
BB

"Berk Birand" <graffiti@yahoo.com> wrote in message
news:1108357762.05636e8fef4de605858860b6ab18c07c@t eranews...[color=blue]
> Larry Brasfield wrote:[color=green]
>> "Berk Birand" <graffiti@yahoo.com> wrote in message
>> news:1108355721.1538885266d9be3798d4ce31daa7e0a7@t eranews...
>> [snip]
>>
>>
>>
>> The above code uses the Product assignment operator
>> to give an initial value to what malloc() has returned.
>> This is incorrect. Assignment should only be done to
>> already constructed objects. Using it on raw memory
>> is likely to lead to the kinds of problems you mention
>> in the subject line.
>>
>> If you insist on using malloc() rather than array new,
>> then you need to use placement new to get the Product
>> ctor to be executed on each block of raw memory that
>> is to become a Product object. Then you can assign
>> to it. Alternatively, you can use placement new and
>> the Product copy ctor, if it has one.
>>[/color]
>
> Hmm, this is awfully interesting. The professor hadn't mentioned this problem. The problem with new is that I need dynamic
> allocation. The size of the array is not known beforehand, and as far as I know, new doesn't accept variables.[/color]

Array new does.
If 'SomeType' is a type name, (such as float, or 'Product' where
you defined "class Product { ... }" somewhere), then you can
get an array of SomeType objects by writing:
SomeType * gobOfObjects = new SomeType[howMany];
where 'howMany' names an integer with a positive value.
Just remember to dispose of it via
delete [] gobOfObjects;
or an equivalent on the same pointer value.
[color=blue]
> So, in the light of this new idea, what am I supposed to do??[/color]

Having not seen your assignment, I'm not sure. But if your
professor, when shown my post, does not agree with my
criticism of the quoted code, then you should double check
everything he/she says with a more reliable reference.

I would be remiss not to mention that using std::vector<Product>
would be a better solution than rolling your own array memory
management, as your posted code apparently represents. If
you were to look at the code for std::vector, you would see
use of placement new as I mentioned.

--
--Larry Brasfield
email: donotspam_larry_brasfield@hotmail.com
Above views may belong only to me.

> Seems like you have a problem before the call to malloc. Can you post a[color=blue]
> minimal program that demonstrates your problem ? Tools like Rational[/color]
Purify[color=blue]
> or valgrind can also help here.
>
> Sharad
>[/color]

I had some other unrelated code working before. After learning that
those maybe flawed I commented all out. Now here's what I have in my
main program, along with the function definition.

I also installed valgrind and tried running memcheck on it. It ended
with apparently 34 errors in 15 contexts. Here's that output:

==31275== LEAK SUMMARY:
==31275== definitely lost: 0 bytes in 0 blocks.
==31275== possibly lost: 640 bytes in 1 blocks.
==31275== still reachable: 32 bytes in 2 blocks.
==31275== suppressed: 0 bytes in 0 blocks.
==31275== Reachable blocks (those to which a pointer was found) are not
shown.
==31275== To see them, rerun with: --show-reachable=yes
Segmentation fault

And here's the code:

#include <iostream>
#include <string.h>
// string.h covers the C-style string functions.
#include "product.h"

using namespace std;

Product* prodndup(const Product* src, int n);

int main()
{

printf("\n\nManipulating products\n");

Product *prod1 = new Product("a",12,32);
Product *prod2;


printf("Before dup, pointer prod2 = %p, contents =\n", prod2);

prod1->print();
prod1->print();

prod2 = prodndup(prod1 , 1); // <-- Problem here

printf("Pointer prod2 = %p, contents =", prod2);
//prod2->print();

return 0;
}

Product* prodndup(const Product* src, int n)
{
int length = n * sizeof(Product); // size of memory to be allocated
Product* retval; // pointer to be returned

retval = (Product*) malloc(length);

if (retval == NULL) return NULL;

Product *p = retval;

while (n > 0)
{
*p = *src;
p++;
src++;
n--;
}


return retval;
}


Thanks a lot for both of your helps!

[color=blue]
> What am I doing wrong? Isn't this how malloc is supposed to be called?
>[/color]

I was just wondering, if there wasn't any previous mistakes in the code,
would that function work as expected? Are we positive that the "leak"
is not in that function??

Thanks again,
BB

Larry Brasfield wrote:[color=blue]
> "Berk Birand" <graffiti@yahoo.com> wrote in message
> news:1108357762.05636e8fef4de605858860b6ab18c07c@t eranews...[color=green]
> > Larry Brasfield wrote:[color=darkred]
> >> "Berk Birand" <graffiti@yahoo.com> wrote in message
> >> news:1108355721.1538885266d9be3798d4ce31daa7e0a7@t eranews...
> >> [snip]
> >>
> >>
> >>
> >> The above code uses the Product assignment operator
> >> to give an initial value to what malloc() has returned.
> >> This is incorrect. Assignment should only be done to
> >> already constructed objects. Using it on raw memory
> >> is likely to lead to the kinds of problems you mention
> >> in the subject line.
> >>
> >> If you insist on using malloc() rather than array new,
> >> then you need to use placement new to get the Product
> >> ctor to be executed on each block of raw memory that
> >> is to become a Product object. Then you can assign
> >> to it. Alternatively, you can use placement new and
> >> the Product copy ctor, if it has one.
> >>[/color]
> >
> > Hmm, this is awfully interesting. The professor hadn't mentioned[/color][/color]
this problem. The problem with new is that I need dynamic[color=blue][color=green]
> > allocation. The size of the array is not known beforehand, and as[/color][/color]
far as I know, new doesn't accept variables.[color=blue]
>
> Array new does.
> If 'SomeType' is a type name, (such as float, or 'Product' where
> you defined "class Product { ... }" somewhere), then you can
> get an array of SomeType objects by writing:
> SomeType * gobOfObjects = new SomeType[howMany];
> where 'howMany' names an integer with a positive value.
> Just remember to dispose of it via
> delete [] gobOfObjects;
> or an equivalent on the same pointer value.
>[color=green]
> > So, in the light of this new idea, what am I supposed to do??[/color]
>
> Having not seen your assignment, I'm not sure. But if your
> professor, when shown my post, does not agree with my
> criticism of the quoted code, then you should double check
> everything he/she says with a more reliable reference.
>
> I would be remiss not to mention that using std::vector<Product>
> would be a better solution than rolling your own array memory
> management, as your posted code apparently represents. If
> you were to look at the code for std::vector, you would see
> use of placement new as I mentioned.
>
> --
> --Larry Brasfield
> email: donotspam_larry_brasfield@hotmail.com
> Above views may belong only to me.[/color]

"Berk Birand" <graffiti@yahoo.com> wrote in message
news:1108355721.1538885266d9be3798d4ce31daa7e0a7@t eranews...
[snip][color=blue]
> And here's the code:[/color]

[snip][color=blue]
> Product* prodndup(const Product* src, int n)
> {
> int length = n * sizeof(Product); // size of memory to be allocated
> Product* retval; // pointer to be returned
>
> retval = (Product*) malloc(length);
>
> if (retval == NULL) return NULL;
>
> Product *p = retval;
>
> while (n > 0)
> {
> *p = *src;
> p++;
> src++;
> n--;
> }
>
>
> return retval;
> }[/color]


The above code uses the Product assignment operator
to give an initial value to what malloc() has returned.
This is incorrect. Assignment should only be done to
already constructed objects. Using it on raw memory
is likely to lead to the kinds of problems you mention
in the subject line.

If you insist on using malloc() rather than array new,
then you need to use placement new to get the Product
ctor to be executed on each block of raw memory that
is to become a Product object. Then you can assign
to it. Alternatively, you can use placement new and
the Product copy ctor, if it has one.

--
--Larry Brasfield
email: donotspam_larry_brasfield@hotmail.com
Above views may belong only to me.

"Berk Birand" <graffiti@yahoo.com> skrev i en meddelelse
news:1108352961.3e5d5b911d946538199f103e3960f28c@t eranews...[color=blue]
> Hi,
>
> For an assignement that I have, I have to write a function that is used
> for duplicating arrays of some objects (called Product). It is supposed to
> be something similar to strndup, but taking a Product* as an argument
> instead of a char*, and of course the length of the array.[/color]
This is an assignment from some class? Go tell your teacher that this is not
C++.

Here, you would copy two arrays of products like this:

std::vector<Product> p1; // first array - using the "built-in" vector
std::vector<Product> p2(p1); // create a vector and make it the same as
your previous array.

// you could also use p2 = p1 if that would be more convenient.
[color=blue]
>
> However, at some point in my code, where I have to call malloc to allocate
> the required memory, I get a Segmentation Fault. Here's what I have
> written:
>
> Product* prodndup(const Product* src, size_t n)
> {
> int length = n * sizeof(Product); // size of memory to be allocated
> Product* retval; // pointer to be returned
>
> retval = (Product*) malloc(length); // <= this is the point[/color]

retval does not point to Product here as there malloc returns raw memory.
As an alternative you could have use retval = new Product[n];[color=blue]
>
> if (retval == NULL) return NULL;
>
> Product *p = retval;
>
> while (n > 0)
> {
> *p = *src;[/color]

As *p is not a Product this assignment is meaningless. Assignment assumes a
valid object.
[color=blue]
> p++;
> src++;
> n--;
> }
>
>
> return retval;
> }
>
> When I uncomment the marked line, the program terminates (although of
> course, without doing what it is supposed to do).
>
> What am I doing wrong? Isn't this how malloc is supposed to be called?[/color]
Nope. You are not supposed to call malloc at all in C++ code that YOU
create. malloc is there for backward compatibility.
[color=blue]
>
> Thanks,
> BB[/color]

/Peter

Berk Birand wrote:[color=blue][color=green]
> > Seems like you have a problem before the call to malloc. Can you post a
> > minimal program that demonstrates your problem ? Tools like Rational[/color]
> Purify[color=green]
> > or valgrind can also help here.
> >
> > Sharad
> >[/color]
>
> I had some other unrelated code working before. After learning that
> those maybe flawed I commented all out. Now here's what I have in my
> main program, along with the function definition.
>
> I also installed valgrind and tried running memcheck on it. It ended
> with apparently 34 errors in 15 contexts. Here's that output:
>
> ==31275== LEAK SUMMARY:
> ==31275== definitely lost: 0 bytes in 0 blocks.
> ==31275== possibly lost: 640 bytes in 1 blocks.
> ==31275== still reachable: 32 bytes in 2 blocks.
> ==31275== suppressed: 0 bytes in 0 blocks.
> ==31275== Reachable blocks (those to which a pointer was found) are not
> shown.
> ==31275== To see them, rerun with: --show-reachable=yes
> Segmentation fault
>
> And here's the code:
>
> #include <iostream>
> #include <string.h>
> // string.h covers the C-style string functions.
> #include "product.h"
>
> using namespace std;
>
> Product* prodndup(const Product* src, int n);
>
> int main()
> {
>
> printf("\n\nManipulating products\n");
>
> Product *prod1 = new Product("a",12,32);
> Product *prod2;
>
>
> printf("Before dup, pointer prod2 = %p, contents =\n", prod2);
>
> prod1->print();
> prod1->print();
>
> prod2 = prodndup(prod1 , 1); // <-- Problem here
>
> printf("Pointer prod2 = %p, contents =", prod2);
> //prod2->print();
>
> return 0;
> }
>
> Product* prodndup(const Product* src, int n)
> {
> int length = n * sizeof(Product); // size of memory to be allocated
> Product* retval; // pointer to be returned
>
> retval = (Product*) malloc(length);
>
> if (retval == NULL) return NULL;
>
> Product *p = retval;
>
> while (n > 0)
> {
> *p = *src;
> p++;
> src++;
> n--;
> }
>
>
> return retval;
> }
>
>
> Thanks a lot for both of your helps![/color]

Just a stab in the dark (with no dispute about everything mentioned
elsethread): What does the constructor for `Product' look like? I
suspect *that's* where the immediate difficulty lies (but use operator
new for objects -- or, better, use a std::vector instead of an array,
etc. etc.). ;-)

HTH,
--ag
--
Artie Gold -- Austin, Texas
http://it-matters.blogspot.com (new post 12/5)
http://www.cafepress.com/goldsays