c++ - How to properly use new and delete (to avoid buffer overflow problems)

Question

This is related to my previous question(now deleted) where I tried to implement a std::vector like data structure. The following is the code.

#include <iostream>
#define print(m) std::cout << m << std::endl

template<typename T>
class vector
{
private:
    unsigned int size = 0;
    unsigned int mem = 5;

    bool full()
    {
        return size == mem;
    }

    void allocate()
    {
        mem += 5;
        T* newData = new T[mem];
        for (unsigned int i = 0; i < size; i++)
        {
            newData[i] = data[i];
        }
        delete[] data; //previous buffer deleted
        data = newData; //the newbuffer is assigned to the previous buffer
        //
    }
    
public:
    T* data = new T[mem];
    unsigned int Size() 
    { 
        return size;
    }

    void push_back(T data_)
    {
        if (!full())
        {
            data[size] = data_;
        }
        else
        { 
            allocate();
            data[size] = data_;
        }
        size++;
    }

public:
    T& operator [] (unsigned int index)
    {
        return data[index];
    }   
};

int main()
{
    vector<int> list;
    list.push_back(1);
    list.push_back(2);
    list.push_back(1);
    list.push_back(2);
    list.push_back(1);
    list.push_back(2);
    list.push_back(1);
    list.push_back(2);
    print(list.Size() << '
');
    
    for (unsigned int i = 0; i < list.Size(); i++)
        print(list[i]);
}

I was (and still am) getting the following warning.

Warning C6386   Buffer overrun while writing to 'newData':  the writable size is 'mem*4' 
bytes, but '8' bytes might be written.

I was told that this was happening because I didn't delete[] the previous buffer which caused memory leaks and warnings. But this time I am deleting the buffer in allocate method.

Also something else that I don't understand is if I add delete[] newData after the line data = newData, again in allocate method.

void allocate()
{
    //other ommited code
    data = newData;
    //I thought I was supposed to do "delete[] newdata;" at first because it deletes the 
    //temporary buffer used to allocate more memory but it caused problems 
    //so i removed it.
    delete[] newdata;
}

The result that is printed is gibberish. But I don't understand why since the data is already copied to data from newData. So why does deleting newData cause problems? What exactly is causing the warning and how do I fix it?

question from:https://stackoverflow.com/questions/65545815/how-to-properly-use-new-and-delete-to-avoid-buffer-overflow-problems

Answer 1

It is not a problem with your code but a false positive from the analyser.

If you open the + next to the message in the Error List (assuming you include Intellisense errors in it), the analyser will show the step it used to make the deduction.

You could tell the analyser that mem is greater than size using _Analysis_assume_(mem >= size);. There are other options to help the analyser make correct guess.

However, if you work on a large project and you get that kind of error after an update of Visual Studio that add a new analysis and have a lot of false positive, you might prefer to disable some analysis rules if you don't have known memory problems with your application.

See https://docs.microsoft.com/en-us/cpp/code-quality/how-to-specify-additional-code-information-by-using-analysis-assume?view=msvc-160 for more information.

Alternatively, just after increasing mem you can add the following condition for the rest of the function:

if (mem > size) { ... }

Or even add an assertion:

assert(mem > size);

It look like the analyser is not real time so adjusting the code might not immediatly remove the warning. Analysing the file using the Analyser menu of Visual Studio should works immediatly (if the analyser is enabled as it seems that old analysis result can survive compilation and even rebuilding etc...).

An untested alternative would be to have a allocate_if_full function instead. As all required information would be in the same function, analyser might not be fooled.

Using standard algorithm

Also, if you use std::copy instead of hand-coded loop, that the analyser won't report a potential problem.