Dealing with accessing NULL pointer

Question

In my class, I've got - inter alia - a pointer:

Class GSM
{
//...
private:
    char *Pin;
//...
}

My constructor initialize it as:

GSM::GSM()
{
//...
    Pin = NULL; 
//...
}

Now, I want to set default value ("1234") to my PIN. I tried very simple way:

bool GSM::setDefaultValue()
{
    lock();
    Pin = "0";
    for (uint8 i =0; i < 4; ++i)
    {
        Pin[i] = i+1;
    }
    unlock();
    return true;
}

But it didn't work. When I run my program (I use Visual Studio 2010) there is an error:

Access violation writing location 0x005011d8

I tried to remove line

Pin = "0";

But it didn't help. I have to initialize it as NULL in constructor. It's part of a larger project, but I think, the code above is what makes me trouble. I'm still pretty new in C++/OOP and sometimes I still get confused by pointers.

What should I do to improve my code and the way I think?
EDIT: As requested, I have to add that I can't use std::string. I'm trainee at company, project is pretty big (like thousands of files) and I did not see any std here and I'm not allowed to use it.

You need to make `Pin` point to a `char` array you can write to. BTW this has absolutely nothing to do with OOP. — juanchopanza, Sep 17 '14 at 07:17
Why are you ignoring your compiler warnings and coming straight to Stack Overflow? There's tons of automated diagnostics that could help you without requiring human attention. — Kerrek SB, Sep 17 '14 at 07:19
Why aren't you using a `std::string`, or a `boost::optional` if you need to differentiate having no string from an empty string? Anyway... if you want to use a pointer you need to point it at some `new`-ed - or perhaps a class-member character array - before you try to overwrite it. — Tony Delroy, Sep 17 '14 at 07:19
I'm not using std::string because I'm not allowed to. The whole program is constructed without it, and it's much bigger (like ~700 Mb) and superiors (I'm trainee student ATM) said not to use std::string. If someone is disgusted about not so pro question, may vote it down. I like to speak with human when I can't understand compiler, because I'm human too. — Photon Light, Sep 17 '14 at 07:30
That's OK for learning. There are also systems where one can't really use the standard library anyway. But you could add that information to the question to make it clear. — juanchopanza, Sep 17 '14 at 07:32
@Kappa: Then ask what string type they **do** use. For projects that have been around for 20 years (not unusual with C++), it's possible that they may be using their own `String` class. But if they're not using any string class, then you should learn from this assignment what _not_ to do. — MSalters, Sep 17 '14 at 08:23

Mats Petersson · Answer 1 · 2014-09-17T23:18:07.893

You need to give the Pin some memory. Something like this:

Pin = new char[5];  // To make space for terminating `\0`;
for(...)
{
   Pin[i] = '0' + i + 1;
}
Pin[4] = '\0';      // End of the string so we can use it as a string.

...

You should then use delete [] Pin; somewhere too (Typically in the destructor of the class, but depending on how it's used, it may be needed elsewhere, such as assignment operator, and you need to also write a copy-constructor, see Rule Of Three).

In proper C++, you should use std::string instead, and you could then do:

Class GSM
{
//...
private:
    std::string Pin;
....

Pin = "0000";
for (uint8 i =0; i < 4; ++i)
{
    Pin[i] += i+1;
}

Using std::string avoids most of the problems of allocating/deallocating memory, and "just works" when you copy, assign or destroy the class - because the std::string implementation and the compiler does the work for you.

The `delete` should appear in the destructor, should it not? I suppose it might be unset at other times too; as long as the pointer (`Pin`) is set to null after the memory is released, that won't cause any trouble. — Jonathan Leffler, Sep 17 '14 at 07:26

GHugo · Answer 2 · 2014-09-17T07:49:13.610

1

You need to allocate a block of memory to store "1234". This memory block will be pointed by your Pin pointer. You can try:

bool GSM::setDefaultValue()
{
    lock();
    Pin = new char[4];
    for (uint8 i =0; i < 4; ++i)
    {
        Pin[i] = '0' + (i + 1);
    }
    unlock();
    return true;
}

As you have allocated dynamicaly a memory block, you should always release it when you don't need it anymore. To do so, you should add a destructor to your class:

GSM::~GSM()
{
    delete [] Pin;
}

edited Sep 17 '14 at 07:49

answered Sep 17 '14 at 07:19

GHugo

2,584
13
14

Because he wants "1234", not "0123". You can also modify your for loop to achieve the same thing. – GHugo Sep 17 '14 at 07:22
1

You should add a note that the class now requires a destructor. It might also be better to allocate an extra character to allow the string to be null terminated — and then null terminate it. – Jonathan Leffler Sep 17 '14 at 07:24
For the destructor I agree, for the string, if he only need to store 4 char and will check PIN correctness checking for these 4 chars, then I think the null terminated string is not necessary (He did not say he need to store a string, so I assume an array of char is enough). – GHugo Sep 17 '14 at 07:27
No need to add `if (Pin != NULL)` - delete is guaranteed to handle NULL pointers. It may not matter much in this instance, but if you have 1000 classes, that all do that, it's another 2-3000 instructions that aren't needed (because inside `delete` itself, the same check is made) – Mats Petersson Sep 17 '14 at 07:43
You don't need to test for non-null-ness of `Pin`; you can safely delete a null pointer (see [Is it safe to delete a NULL pointer?](http://stackoverflow.com/questions/4190703/is-it-safe-to-delete-a-null-pointer/)). While I will grant you that the question didn't say it needed a null-terminated string, I think it would be more sensible to assume that one was needed. However, it is your answer. – Jonathan Leffler Sep 17 '14 at 07:43
Did not know for delete checking NULL pointer. I have edited my answer. – GHugo Sep 17 '14 at 07:48
While you're at it, your class now has a default copy constructor which does the wrong thing. You might want to ` = delete` it, as well as deleting the assignment operator for the same reason (or implement it, of course). `std::string` just works. – MSalters Sep 17 '14 at 08:25

score 0 · Answer 3 · edited Jun 20 '20 at 09:12

Simple answer:

Instead of using the heap (new delete) just allocate space in your class for the four character pin:

Class GSM
{
//...
private:
    char Pin[5];
//...
}

The length is fixed at 5 (to allow space for 4 characters and terminating null ('\0'), but as long as you only need to store a maximum of 4 characters, you are fine.

Of course, if you want to make it easy to change in the future:

Class GSM
{
//...
private:
    const int pin_length = 4;
    char Pin[pin_length+1];
//...
}

Your function to set the value will then look like:

bool GSM::setDefaultValue()
{
    lock();
    for (char i = 0; i < pin_length; ++i)
    {
        Pin[i] = i+1;
    }
    Pin[pin_length]=0;
    unlock();
    return true;
}

or even:

bool GSM::setDefaultValue()
{
    lock();
    strcpy(Pin,"1234");  //though you would have to change this if the pin-length changes.
    unlock();
    return true;
}

A `const int` would be better than a preprocessor definition, given the lack of scoping with the latter. You've left room for a NUL (which is `\0` rather than `/0`) but not added one in `setDefaultValue()`. — Tony Delroy, Sep 17 '14 at 17:26

Dealing with accessing NULL pointer

3 Answers3

Simple answer: