Dynamic allocation of float matrix causes Access violation writing location error

Question

I've received the source code of an MFC application, but when I start it I get an access violation writing location error.

Here below the VS project General configuration: Project configuration

I cannot share the full source code, but the culprit lines are the followings:

#include <iostream>

using namespace std;

int main()
{
    float** ppfPoints[2];
    int maxIteration = 300000;
    for (int i = 0; i < 2; i++)
    {
        ppfPoints[i] = (float**)new float[maxIteration];
        for (int j = 0; j < maxIteration; j++)
        {
            ppfPoints[i][j] = new float[3];
            cout<<j<<" ";
        }
    }
    
    return 0;
}

If I run it on onlinegdb (uncommenting the print) the code stops while printing the value 151479. The fun fact (at least for me) is that if I change the maxIteration value to 50000 the program stops at the value 26598.

In any case onlinegdb says "...Program finished with exit code 0", while on my MSVC compiled application I have the previously mentioned error.

Could anyone help me pointing out where the error is? Thank you in advance!

`new float[maxIteration]` doesn't return a `float**`. _Do not cast_, there is no reason to. — tkausl, Dec 20 '22 at 16:43
`new float[maxIteration];` gives you wrong type, instead of fixing it you tried to hide it by casting, creating worse problem. Remove cast, fix type and it will work. Though you should use containers or at least smart pointers. — Slava, Dec 20 '22 at 16:46
General rule of thumb: Only cast when you KNOW that the cast is the solution. If you don't know for sure, you're probably replacing a compiler error that you know about with a runtime error that you don't know about and will have to find the hard way. If you see a cast, especially a C-Style cast like `(float**)` or a `reinterpret_cast`, you're probably looking at a bug, so inspect the code carefully. — user4581301, Dec 20 '22 at 17:02
Using new/delete after C++11 is no longer recommended, because you can easily have memory leaks (like your program does). You should reeally use `std::vector>`. [Avoid calling new/delete explicitly](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#r11-avoid-calling-new-and-delete-explicitly). All in all your code still uses some "C" style coding, where are you learning C++ from? (Also : `using namespace std'` is not recommended — Pepijn Kramer, Dec 20 '22 at 17:03
[Here's an even better (most of the time) trick.](https://stackoverflow.com/a/36123944/4581301) This makes a 1D array LOOK like a 2d array. The 1D array is one big memory block, but when you have dynamically allocated pointers to pointers of dynamic allocations or `vector`s of `vector`s the variable's storage is scattered through memory and the poor CPU has to chase the pointers down and may not be able to use cache efficiently. The difference in performance can be staggering. — user4581301, Dec 20 '22 at 17:06
Good [mre], by the way. Distilled the problem down to the bare essentials. — user4581301, Dec 20 '22 at 17:08

score 0 · Accepted Answer · answered Dec 20 '22 at 17:02

Here are the problems:

Casting of a float* to a float**
Didn't declare the matrix correctly (should be new float*[matrix_height])

#include <iostream>

using namespace std;

int main()
{
    float** ppfPoints = new float*[2]; // array (pointers) of array (pointers) of floats = (float**)
    int maxIteration = 300000;
    for (int i = 0; i < 2; i++)
    {
        ppfPoints[i] = new float[maxIteration]; // returns a pointer to an array of float (float*)
        for (int j = 0; j < maxIteration; j++)
        {
            ppfPoints[i][j] = 0.f; // a float (float)
            cout<<j<<" ";
        }
    }
    
    return 0;
}

I've changed the row with the cast like this: `ppfPoints[i] = new float*[maxIteration];` of course now it seems to work correctly. Your answer is not 100% correct, because I wanted to put another float* inside it — Cav, Dec 20 '22 at 17:05

score 0 · Answer 2 · answered Dec 20 '22 at 17:11

C++ style for your code (I had to guess some variable names, I hope you can do better, since you know what your code should be doing).

#include <iostream>
#include <vector>

int main()
{
    // datastructure sizes use std::size_t
    static constexpr std::size_t maxIteration{ 30ul }; // smaller number for demo
    static constexpr std::size_t dimensions{ 2ul };

    // no need to use `hungarian notation` name things after what they are (not how they are implemented).
    // this line alone allocates all data and sets it to 0.0f
    std::vector<std::vector<float>> point_data(dimensions, std::vector<float>(maxIteration, 0.0f));

    // and use range based for loops if you can
    // example of setting all values to 1.0f
    for (auto& dimension : point_data) // dimensions
    {
        for (auto& value : dimension) // maxIteration
        {
            value = 1.0f; // 
            std::cout << value << " ";
        }
        std::cout << "\n";
    }

    return 0;
}

Unluckily all code is written with hungarian notation and I cannot use std::vector :/ — Cav, Dec 20 '22 at 17:15
Another option is to use a std::unique_ptr,2>> and use the address of [0][0] to pass to legacy code. I sometimes write new code in static libaries and give them an "interface" that older code can also use (bonus : you can add unit tests to the static library). That gives you a stepwise way out of legacy code as well. — Pepijn Kramer, Dec 20 '22 at 17:20

Dynamic allocation of float matrix causes Access violation writing location error

2 Answers2