Possible misuse of "new"

[Anomanderis]

Hi All,

I'm relatively new to C++, I'm trying to create and utilize a class object, but while my code compiles seamlessly, it appears to hang while creating the object

barhost.h

#ifndef GUARD_barsc
#define GUARD_barsc

#include <string>
#include <vector>
#include <iostream>
#include <fstream>


class BarHost {
  std::string name;
  char logFile[150];
  ino_t inode;
  public:
  std::string get_name() {return name;}
  ino_t get_inode() {return inode;}
  ino_t check_inode();
  std::string set_name();
  BarHost();
  BarHost(std::string);
//  std::ifstream in;
};

#endif

barhost.cpp:

#include "barhost.h"
#include <string>
#include <iostream>
#include <fstream>
#include <vector>
#include <ios>
#include <ctime>
#include <sys/types.h>
#include <sys/stat.h>
#include <dirent.h>

using std::string;
using std::ifstream;
using std::ios;
using std::ofstream;
using std::cout;
using std::endl;
using std::getline;
using std::vector;



BarHost::BarHost(string host_name) {
  name = host_name;
  cout << "Creating Bar host " << host_name << endl;
  string myname = "/var/tmp/" + host_name + ".log";
  strcpy(logFile, myname.c_str());
  cout << "Populate buffer " << endl;
  struct stat* buffer;
  int status(0);
  const char* fileholder = logFile;
  cout << string(fileholder) << endl;
  status = stat(fileholder, buffer);
  cout << status << endl;
  inode = buffer->st_ino;
  cout << "Exit constructor " << endl;

newbar.cpp

#include "barhost.h"
#include <stdlib.h>
#include <string>
#include <iostream>
#include <fstream>
#include <vector>
#include <ios>
#include <unistd.h>
#include <new>

using std::string;
using std::ifstream;
using std::ios;
using std::ofstream;
using std::cout;
using std::endl;
using std::getline;
using std::vector;

int main()
{
  BarHost*  myhost = new BarHost("bar1");
//  BarHost myhost("bar1");
  cout << myhost->get_name() << endl;
  cout << myhost->get_inode() << endl;
  return(0);
}

When I run the binary, I get:

./inbar
Creating Bar host bar1
Populate buffer
/var/tmp/bar1.log
0
Exit constructor <<- It hangs here....

If someone could tell me what I'm doing wrong, I'll really appreciate that.

[cire]

 struct stat* buffer;
  int status(0);
  const char* fileholder = logFile;
  cout << string(fileholder) << endl;
  status = stat(fileholder, buffer);

buffer points to some random place in memory.
stat writes to that random place.

[ne555]

> Possible misuse of "new"
Yes, BarHost* myhost = new BarHost("bar1"); is a misuse.
It should be simply BarHost myhost("bar1");

[Anomanderis]

Are you sure about this? I was under the impression that I needed to use "new" in order to create a "permanent" object.

If I don't use new, then if for example I create several BarHost objects inside a for loop, storing them say in a vector, won't they simply overwrite each other?

[DEUX92yv]

If you use vector.push_back(/* BarHost object*/), each BarHost will have its own index in the vector. If you intend to use new, it would have to be a vector of BarHost*s. Either way, each element in the vector will be a valid BarHost (or a pointer to a valid one).

Keep in mind (because its easy to forget) that if you do this, myhost will no longer be a pointer; to access its members, use the dot operator instead of the arrow (i.e., myhost.get_name() instead of myhost->get_name().

[Anomanderis]

Actually, I resolved this, thanks cire.

I modified the Barhost constructor so that buffer was pointing to the memory address of an rvalue and it worked.

BarHost::BarHost(string host_name) {
  name = host_name;
  cout << "Creating Bar host " << host_name << endl;
  string myname ="/var/tmp/" + host_name + ".log";
  strcpy(logFile, myname.c_str());
  cout << "Populate buffer " << endl;
  struct stat buff;
  struct stat* buffer = &buff;

  int status(0);
  const char* fileholder = logFile;
  cout << string(fileholder) << endl;
  status = stat(fileholder, buffer);
  cout << status << endl;
  inode = buffer->st_ino;
  cout << "Exit constructor " << endl;
}

Thanks all.

[ne555]

> in order to create a "permanent" object.
that's called `memory leak'.
delete new T; is clearly a misuse, you just forgot to delete in your code.

[LB]

Anomanderis wrote:

I modified the Barhost constructor so that buffer was pointing to the memory address of an rvalue and it worked.

I'm pretty sure you don't want anything pointing to the address of a temporary...

[cire]

The pointer variables (which you seem to be overly fond of) are superfluous.

BarHost::BarHost(string host_name) {
  name = host_name;
  cout << "Creating Bar host " << host_name << endl;
  string myname ="/var/tmp/" + host_name + ".log";
  strcpy(logFile, myname.c_str());
  cout << "Populate buffer " << endl;
  struct stat buff;
  
  int status(0);
  cout << logFile << endl;
  cout << stat(logFile, &buff) <<  endl;
  inode = buff.st_ino;
  cout << "Exit constructor " << endl;
}

[MikeyBoy]

I was under the impression that I needed to use "new" in order to create a "permanent" object.

If you use new to create an object, then you do have control over the lifetime of that object, true. If you simply create local object on the stack, then it will be destroyed when it falls out of scope.

In the code you posted, however, this is an irrelevance. You're creating your BarHost object in the main function, so it will remain in scope until the program exits. You don't need to worry about dynamically allocating it in this case.

As a general rule, you shouldn't dynamically allocate stuff unless you need to, because you want to minimize the amount of memory management you have to do in your code.

Under other, more complicated circumstances, you might very well need to dynamically allocate, but not in the code you've shown us.

If I don't use new, then if for example I create several BarHost objects inside a for loop, storing them say in a vector, won't they simply overwrite each other?

If you're storing the actual objects in the vector, like so:

std::vector<BarHost> myVector;
for (int i = 0; i < NUM_OBJECTS; ++i)
{
  BarHost newObj;
  myVector.push_back(newObj);
}

then there's no problem, because when you store something in a vector, you actually store a copy of it. The vector owns that copy and manages the memory for it, so it's safe regardless of what you do in your code.

In detail, what happens is the following:

1) You enter the first iteration of the loop.
2) You create a BarHost object on the stack.
3) When you add that BarHost object to the vector, the vector creates its own copy of it, with its own memory allocated for that copy.
4) You exit the first iteration of the loop. newObj drops out of scope, and the memory for it is no longer reserved, but it doesn't matter, because myVector has already stored its own copy.
5) You enter the 2nd iteration of the loop.
6) You create another BarHost object on the stack. This may re-use the same memory as the first one, or may use different memory - that's up to the operating system to decide. Either way, it won't damage myVector, because it's storing its own copy in other memory.
7) This second BarHost object gets copied into the vector, with the vector storing this copy alongside the first.
8) And so on...

EDIT:

Now, if you were just storing pointers to BarHost objects in the vector, things would be different:

std::vector<BarHost*> myVector;
for (int i = 0; i < NUM_OBJECTS; ++i)
{
  BarHost newObj;
  myVector.push_back(&newObj);
  // TROUBLE - the memory that the pointer in the vector points to is about to be trashed.
}

The only thing you're copying into the vector now is the address of newObj. Now, when you reach the end of a loop iteration, newObj drops out of scope, which means that the memory that was storing it is no longer reserved, and can be overwritten with other things. But your vector is still storing an address to that memory, despite the fact that it no longer contains what it did.

You see the problem?

In this case, you'd need to dynamically allocate:

std::vector<BarHost*> myVector;
for (int i = 0; i < NUM_OBJECTS; ++i)
{
  BarHost* pNewObj = new BarHost();
  myVector.push_back(pNewObj);
}

// BUT WHAT FREES UP THE MEMORY YOU'VE ALLOCATED? 

This is fine, but because you've dynamically allocated that memory, you (or rather, the code you write, is going to have to take responsibility for freeing it. Where should this be done? When should this be done? Which classes and/or functions are responsible for doing it? How can you be sure it's not going to be freed twice? How can you be sure that nothing's going to try and use that pointer after the memory's been freed?

These are not always easy questions to answer. There are ways to design your code to make dynamic allocation safer and easier, but that's beyond the scope of this post.