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 wrote:
> 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

Getting a segfault on a call to malloc() is indicative of a trashed free 
store; at some point in your program *previous* to this call you most 
likely have either called free() on already deallocated memory or 
overrun a malloc()-ed buffer. Bugs like these can be devilishly 
difficult to track down -- at least without some memory checking tool 
that would be platform specific (for example, valgrind no x86 Linux).

HTH,
--ag

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

"Berk Birand" <graffiti@yahoo.com> wrote in message
news:1108352961.3e5d5b911d946538199f103e3960f28c@teranews...
> 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;
> }

Seems like you have a problem before the call to malloc. Can you post a
minimal program that demonstrates your problem ?  Tools like Rational Purify
or valgrind can also help here.

Sharad

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

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!

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

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

"Berk Birand" <graffiti@yahoo.com> wrote in message
 news:1108355721.1538885266d9be3798d4ce31daa7e0a7@teranews...
[snip]
> And here's the code:

[snip]
> 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;
> }


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. 

Larry Brasfield wrote:
> "Berk Birand" <graffiti@yahoo.com> wrote in message
>  news:1108355721.1538885266d9be3798d4ce31daa7e0a7@teranews...
> [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.
> 

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.

So, in the light of this new idea, what am I supposed to do??

Thanks,
BB

"Berk Birand" <graffiti@yahoo.com> wrote in message

> 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.

You need to re-read about new operator.
The following is legal -
int i;
....
Product *p = new Product[i]; // assuming a default constructor is available
...
delete [] p;

Sharad

"Berk Birand" <graffiti@yahoo.com> wrote in message
 news:1108357762.05636e8fef4de605858860b6ab18c07c@teranews...
> Larry Brasfield wrote:
>> "Berk Birand" <graffiti@yahoo.com> wrote in message
>>  news:1108355721.1538885266d9be3798d4ce31daa7e0a7@teranews...
>> [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.
>>
>
> 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.

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.

> So, in the light of this new idea, what am I supposed to do??

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. 

Larry Brasfield wrote:
> "Berk Birand" <graffiti@yahoo.com> wrote in message
>  news:1108357762.05636e8fef4de605858860b6ab18c07c@teranews...
> > Larry Brasfield wrote:
> >> "Berk Birand" <graffiti@yahoo.com> wrote in message
> >>  news:1108355721.1538885266d9be3798d4ce31daa7e0a7@teranews...
> >> [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.
> >>
> >
> > 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.
>
> 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.
>
> > So, in the light of this new idea, what am I supposed to do??
>
> 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.

"Berk Birand" <graffiti@yahoo.com> skrev i en meddelelse 
news:1108352961.3e5d5b911d946538199f103e3960f28c@teranews...
> 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.
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.

>
> 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

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

As *p is not a Product this assignment is meaningless. Assignment assumes a 
valid object.

>       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?
Nope. You are not supposed to call malloc at all in C++ code that YOU 
create. malloc is there for backward compatibility.

>
> Thanks,
> BB

/Peter 

Berk Birand wrote:
>  > Seems like you have a problem before the call to malloc. Can you post a
>  > minimal program that demonstrates your problem ?  Tools like Rational 
> Purify
>  > or valgrind can also help here.
>  >
>  > Sharad
>  >
> 
> 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!

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