2

In writing a response, I wrote some code that challenged my assumptions on how const pointers work. I had assumed const pointers could not be deleted by the delete function, but as you'll see from the code below, that isn't the case:

#include <new>
#include <string.h>

class TestA
{
    private:
        char *Array;
    public:
        TestA(){Array = NULL; Array = new (std::nothrow) char[20]; if(Array != NULL){ strcpy(Array,"Input data"); } }
        ~TestA(){if(Array != NULL){ delete [] Array;} }

        char * const GetArray(){ return Array; }
};

int main()
{
    TestA Temp;
    printf("%s\n",Temp.GetArray());
    Temp.GetArray()[0] = ' '; //You can still modify the chars in the array, user has access
    Temp.GetArray()[1] = ' '; 
    printf("%s\n",Temp.GetArray());
    //Temp.GetArray() = NULL //This doesn't work

    delete [] Temp.GetArray(); //This works?! How do I prevent this?
}

My question is, how do I pass the user access to a pointer (so they can use it like a char array), whilst making it so that the delete function cannot delete it, by preferably throwing some sort of complaint or exception?

SE Does Not Like Dissent
  • 1,767
  • 3
  • 16
  • 36

5 Answers5

6

If your users are using delete[] on pointers they didn't get from new[], hit them upside the head with a clue bat.

There are so many reasons a pointer can be dereferenced but mustn't be passed to delete:

  • Someone else will delete it.
  • It's not the beginning of the block.
  • It came from malloc or some other non-new allocator.
  • It has static, not dynamic lifetime.
  • If has automatic lifetime.

Some of these will manifest in exceptions at runtime. Others will cause crashes at some later time. All are "undefined behavior" according to the standard.

The assumption should be that a pointer cannot be used as the argument to delete, unless explicitly stated otherwise.

If entry-level programmers are making this mistake, educate them. If "experienced" developers are doing it, fire them for lying on their resume.

If you just want it to hurt when they do that, allocate the array one larger than necessary, and return Array + 1;. Now everything will blow up if they try to use delete[].

The practical use is that it's (more) likely to make the program crash inside the bogus call to delete, with the offending function still on the call stack. Where the original will probably continue running for a while, and finally crash in innocent code. So this helps you catch stupid users.

Ben Voigt
  • 277,958
  • 43
  • 419
  • 720
  • Not inexperienced users, but malicious ones. I like your Array+1 trick however, as this offers a practical solution (IE the program will not continue to run). I won't be implementing it in this version of the CharArray, but the next revision will have this. I'll tack in, one might want two pointers: A pointer to the memory address for deletion that is never returned, and one to the +1 offset. Return the +1 offset and bam. – SE Does Not Like Dissent Oct 10 '11 at 17:33
  • @SSight3: Don't consider that any sort of protection against malicious users. It's not difficult to reach into your class and overwrite your private pointer. Only thing to do with untrusted code is to run it in an OS-enforced (with help from the memory protection unit) sandbox. – Ben Voigt Oct 10 '11 at 17:38
  • 2
    @SSight3: You can still just `delete [] (Test.GetArray()-1);` if the user sees your internal pointer. And he can also just edit your implementation. Don't loose your head over this, just do the easiest and fastest thing. – Xeo Oct 10 '11 at 17:42
  • @Ben: Noted. I suppose in a way there isn't much you can do, but everything helps teach me something new. Still like the trick though. – SE Does Not Like Dissent Oct 10 '11 at 17:42
5
delete [] Temp.GetArray(); //This works?! How do I prevent this?

As long as, it returns char* or some other pointer type, you cannot prevent it; it doesn't matter if the expression in delete statement is const, because all of the following are perfectly valid in C++:

char *pc = f(); 
delete [] pc;  //ok

const char *pc = g(); 
delete [] pc; //ok

char * const pc = h(); 
delete [] pc; //ok

const char * const pc = k(); 
delete [] pc; //ok

However, if you change this :

char *Array;

to this

std::vector<char> Array;

And then you could achieve what you want, by returning it as:

std::vector<char> & GetArray() { return Array; }

The bottomline is :

In C++, the default choice for dynamic array should be std::vector<T> unless you've a very strong reason not to use it.

Nawaz
  • 353,942
  • 115
  • 666
  • 851
  • Not helpful, they can resize it, causing reallocation. Or erase all the contents. – Ben Voigt Oct 10 '11 at 17:10
  • @BenVoigt: That can be stopped easily. But the OP wants to modify it at the callsite (I saw his code), hence I returned it by reference. Now if the user erases the contents, then it is not the problem with `std::vector`, rather the real problem is : why would he design such a class in the place, but then that is another question. – Nawaz Oct 10 '11 at 17:11
  • Efficiency. I might also point out this merely pushes the problem back a level. Consider: `std::vector Test; Test.assign(11,'\"'); delete [] Test.data();` – SE Does Not Like Dissent Oct 10 '11 at 17:15
  • 1
    @BenVoigt But now he can return a const reference that is not resizable that easily. – Christian Rau Oct 10 '11 at 17:16
  • If it's always `char[20]`, then just make it a static array. – Kerrek SB Oct 10 '11 at 17:17
  • @SSight3: In that case, write your own class, and don't return pointer at all. – Nawaz Oct 10 '11 at 17:17
  • @ChristianRau: A const reference also does not allow modifying elements. – Ben Voigt Oct 10 '11 at 17:18
  • 2
    @SSight3: `delete [] Test.data();` - well, you can be as paranoid as you want, but when a user does this, it's his own fault, nothing you can do. An even better choice would be not to return the array (or vector) and simply wrap the access with an appropriate `operator[]`, but even then the user can do stupid stuff like `delete [] &Temp[0];`. You could return a proxy from the `operator[]` that overloads the `operator&` or something like that, but the bottom line is, that you can't stop your user from shooting themselves in the foot. The only thing you can do is make it harder for them. – Xeo Oct 10 '11 at 17:20
  • 1
    @SSight3 So then don't write `delete[] Test.data()`. Whoever writes this (which is now clearly intentional) is responsible for the problems himself (and I'm sure is invoking UB by standard, anyway). – Christian Rau Oct 10 '11 at 17:20
  • @Xeo: Obviously they can shoot themselves in the foot. I just want to make it as difficult as possible. – SE Does Not Like Dissent Oct 10 '11 at 17:26
  • @Christian Rau: More to prevent malicious behaviour than accidental. I'm the kind of person who assumes (given the existence of trojans, viruses, worms etc) there will always be a malicious user down the road. – SE Does Not Like Dissent Oct 10 '11 at 17:26
  • @KerrekSB: Most minimalistic example possible. TestA/TestB/etc is a running 'example class' that usually contains the most minimalistic features (IE no resize etc) of the actual classes. Adding features not related tends to have people hopping off the wrong track or typo-hunting than answering. – SE Does Not Like Dissent Oct 10 '11 at 17:28
  • 1
    @SSight3: Exactly. `std::vector` makes it one step harder. Also, while programming in C++, one should know that `std::vector` implements RAII which means it takes care of memory management itself, and the user should NOT worry about it. Now if the user writes `delete []Test.data()` or `delete []&Test[0]`, then either he has gone insane, OR he should read a good C++ book. – Nawaz Oct 10 '11 at 17:29
  • 3
    @SSight3 Such a user could also just write `int i = 7; delete &i;` and applause himself for invoking UB, whatever else that may gain him. – Christian Rau Oct 10 '11 at 17:29
  • @Nawaz: This class is already RAII. I just want to prevent external interference, which will occur if you use this class or vector. So I don't see the point (it isn't made harder). – SE Does Not Like Dissent Oct 10 '11 at 17:37
  • @Rau: Obviously you've not heard of the [double free attack](http://www.cprogramming.com/tutorial/secure.html). IE, I don't want the user abusing the memory, and as with ben's example, I want it to blow up in their face if they do. – SE Does Not Like Dissent Oct 10 '11 at 17:39
1

Your const is doing no effect, you are returning a const pointer, not a pointer to const char, which is what you want. Also see Nawaz recommendation on using vectors instead.

piotr
  • 5,657
  • 1
  • 35
  • 60
  • Not in the style of downvoting things, I'll explain why you are incorrect: `Temp.GetArray() = NULL;` is prevented (as would be new etc etc) via const pointer (IE address cannot be reassigned external of class). You don't want a const char, as, given the example, the user is supposed to be able to modify the contents of the array, not the array itself. – SE Does Not Like Dissent Oct 10 '11 at 17:30
  • 1
    @SSIght3: `Temp.GetArray() = NULL;` would only work if you returned a reference to the pointer. When returning a pointer by value, the original can't be overwritten, regardless of `const`. – Ben Voigt Oct 10 '11 at 17:34
  • @Ben Hence why I said "Temp.GetArray() = NULL; is prevented" - as in, I want it prevented "not [permitting modification of] the array itself". So const pointer does indeed do something useful. – SE Does Not Like Dissent Oct 10 '11 at 17:35
  • 1
    @SSight3: No, `const` pointer isn't doing anything useful. Returning by value, not reference, is doing something useful. – Ben Voigt Oct 10 '11 at 17:40
1

The most you can really do is to return something that is not an acceptable operand for delete. This can be an RAII object (like std::vector or std::array) or it can be a reference (depending on your situation, either could be appropriate).

[Un]fortunately, C++ lets the programmer do all sorts of sneaky things.

Michael Price
  • 8,088
  • 1
  • 17
  • 24
0

You cannot block stupidity entirely, but you can stifle it a tad by using an access mechanism instead of returning the actual array pointer.

char& operator[](size_t index) { return Array[index]; }

This does not address the ability to treat it like a char array, but as has been pointed out, if you reveal that pointer, (bad) programmers a free to run delete on it all they want.

TheBuzzSaw
  • 8,648
  • 5
  • 39
  • 58