C++ Difficulty adding an object to an array

Question

My goal is to write a function that adds an object of the AccountInfo class to a 200-element array of AccountInfo objects. The array starts with no objects in it. The AccountInfo class contains several fields - mostly char*, with a few ints.

After hours of attempts, I cannot figure out what is going wrong. My code all complies, but I get an exception

First-chance exception at 0x00A164B0 in Project1.exe: 0xC0000005: Access violation writing location 0x00000000.

on the following line:

getAccounts()[size()] = AccountInfo(*newUser);

I've simplified my code as much as I can while retaining the essential information. If supplying the code for the AccountInfo class would be helpful, I can do that too.

#include <iostream>
using namespace std;

class AccountInfo
{
private:
    char* _userLoginName;   
    char* _password;        
    unsigned int _uid;      
    unsigned int _gid;      
    char* _gecos;           
    char* _home;            
    char* _shell;           

public:
    //Constructor and Destructor
    AccountInfo(char* username);
    ~AccountInfo();

    //Also contains getters and setters.
};

//Method Definitions

AccountInfo::AccountInfo(char* username)
{
    //Initialize the username and other mandatory fields.
    _userLoginName = username;
    _home = "/", "h", "o", "m", "e", "/", username;
    _shell = "/bin/bash";
}

AccountInfo::~AccountInfo()
{
    //Delete dynamically created fields.
    delete _userLoginName;
    delete _password;               
    delete _gecos;          
    delete _home;           
    delete _shell;
}

class UserDB
{
private:
    AccountInfo* _accounts[200];    
    unsigned int _size;             
    unsigned int _nextUid;          
    unsigned int _defaultGid;       

    AccountInfo* getAccounts();

public:
    UserDB();
    ~UserDB();

    void adduser(AccountInfo* newUser); 

    int size(); // return the number of accounts stored (_size)
};

AccountInfo* UserDB::getAccounts()
{
    return _accounts[200];
}

UserDB::UserDB()
{
    _size = 0;
    _nextUid = 1001;
    _defaultGid = 1001;
}

int UserDB::size()
{
return _size;
}

void UserDB::adduser(AccountInfo* newUser)
{
    getAccounts()[size()] = AccountInfo(*newUser);
}

int main() //main method
{
    UserDB users;
    AccountInfo *x = new AccountInfo("Joe"); 
    //This creates an AccountInfo object with one of its
    //char* fields initialized to "Joe".
    users.adduser(x);

    return 0;
}

What am I doing wrong? How can I fix it?

Answer 1

You're creating a new temporary object to add a pointer to:

getAccounts()[size()] = AccountInfo(*newUser);

Why dereference newUser, only to copy-construct a temporary?

You probably wanted:

getAccounts()[size()] = newUser;

Also, size isn't a great name for that function - it makes it sound as though you're indexing the N+1 position every time. numAccounts or similar is probably more appropriate.

Don't forget to increment this counter, and to check you haven't hit the 200 limit!

Further, with your added code I see that you try to set variables of type char* to "something". You can only do this on initialisation; your constructor needs to set _shell etc. in this manner in an initialisation list, not in the constructor body:

AccountInfo::AccountInfo(char* newUser) : _shell("/bin/bash") {/**/}

Answer 2

It is not possible to answer your question authoritatively, because you did not show the contents of the size() and getAccounts() methods.

But a good guess can be made:

void UserDB::adduser(AccountInfo* newUser)
{
    getAccounts()[size()] = AccountInfo(*newUser);
}

For this code to work, the getAccounts() method must return a pointer to an existing array of initialized AccountInfo objects, and this code will replace the existing instance of an AccountInfo object, in the array, with a newly-constructed object.

This doesn't really make a lot of sense.

Additionally:

AccountInfo* _accounts[200];

This declares a 200-element array of pointers to AccountInfo objects. This does not declare an array of 200 AccountInfo objects.

I suspect that your getAccounts() method somehow returns the _accounts class member. If so, this cannot possibly ever work this way. Not, at least, unless your constructor dynamically allocates an actual array on the heap, initializes all these pointers to point to their corresponding elements in the dynamically-allocated array, and then always returns the address of the zero-th class instance.

Is that what it does?

Answer 3

AccountInfo* _accounts[200]; 

Because _accounts[index of element] not create using NEW, so _accounts[index of element] is NULL. Please check NULL getAccounts()[size()] before getAccounts()[size()] = AccountInfo(*newUser);

Answer 4

AccountInfo* UserDB::getAccounts()
{
    // This returns a pointer to the element as index 200,
    // which is past the end of the array and 'undefined behavior'
    return _accounts[200];
}

Since you've declared an array of 200 AccountInfo pointers (i.e., not an array of 200 AccountInfo objects) you should return a pointer to the member.

AccountInfo** UserDB::getAccounts()
{
    return _accounts;
}

Furthermore, assuming the addUser function is intended to take ownership of the given pointer it should be updated as follows:

void UserDB::adduser(AccountInfo* newUser)
{
    // This function also needs to guard against size() returning
    // values larger than 199 and increment the size

    // This takes ownership of the pointer and the destructor needs
    // to delete the memory (see 'rule of three' below)
    getAccounts()[size()] = newUser;
}

Also since this is C++ you should be using standard containers or smart pointers to handle this memory management. For example:

• std::vector
• std::array C++11
• std::unique_ptr C++11

Furthermore, if the UserDB class is in fact meant to own the AccountInfo pointers then you should know about the rule of three.

If C++11 is available then you could also implement move semantics.

Answer 5

Even though this question is two years old I would like to mention an answer that nobody had suggested yet. In my opinion you should use boost::static_vector (http://www.boost.org/doc/libs/1_64_0/doc/html/container/non_standard_containers.html#container.non_standard_containers.static_vector) if it is possible. It has the same behavior that your code tries to have. It constructs an static array with a fixed number of elements and offers the same interface as std::vector. If you try to push more elements than possible it throws an out_of_memory exception just as std::vector.