ERROR: AddressSanitizer: heap-buffer-overflow on address X at pc Y bp Z sp W

Question

I'm pretty new to dynamic memory management and using Fsanitise flag to find problems with memory management. I cannot use vector to store data - I need to use primitive arrays, as well as "new" and "delete" to manage the heap objects.

I got the following error when I try to run the EuclideanVectorTester compiled program but not sure what the problem is, will someone enlighten me please?

weill % make
g++ -std=c++14 -Wall -Werror -O2 -fsanitize=address -c EuclideanVectorTester.cpp
g++ -std=c++14 -Wall -Werror -O2 -fsanitize=address -c EuclideanVector.cpp
g++ -fsanitize=address EuclideanVectorTester.o EuclideanVector.o -o EuclideanVectorTester
weill % ./EuclideanVectorTester
1
=================================================================
==15341==ERROR: AddressSanitizer: heap-buffer-overflow on address 0xb59007e0 at pc 0x8048ca7 bp 0xbfb47388 sp 0xbfb4737c
WRITE of size 8 at 0xb59007e0 thread T0
    #0 0x8048ca6 in main (/tmp_amd/kamen/export/kamen/3/z3386180/cs6771/evec/EuclideanVectorTester+0x8048ca6)
    #1 0xb6ecae45 in __libc_start_main (/lib/i386-linux-gnu/i686/cmov/libc.so.6+0x16e45)

0xb59007e0 is located 0 bytes to the right of 16-byte region [0xb59007d0,0xb59007e0)
allocated by thread T0 here:
    #0 0xb722a4c4 in operator new[](unsigned int) (/usr/lib/libasan.so.1+0x524c4)
    #1 0x8048b9a in main (/tmp_amd/kamen/export/kamen/3/z3386180/cs6771/evec/EuclideanVectorTester+0x8048b9a)
    #2 0xb6ecae45 in __libc_start_main (/lib/i386-linux-gnu/i686/cmov/libc.so.6+0x16e45)
    #3 0x8048d8c (/tmp_amd/kamen/export/kamen/3/z3386180/cs6771/evec/EuclideanVectorTester+0x8048d8c)

SUMMARY: AddressSanitizer: heap-buffer-overflow ??:0 main
Shadow bytes around the buggy address:
  0x36b200a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x36b200b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x36b200c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x36b200d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x36b200e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x36b200f0: fa fa fa fa fa fa fa fa fa fa 00 00[fa]fa 04 fa
  0x36b20100: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x36b20110: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x36b20120: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x36b20130: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x36b20140: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Contiguous container OOB:fc
  ASan internal:           fe
==15341==ABORTING
weill %

The EuclideanVector.h file is this:

#ifndef _EuclideanVector_h
#define _EuclideanVector_h

#include <iostream>
#include <algorithm>


namespace even {
    class EuclideanVector {

        public:

            /*
             * A constructor that takes the number of dimensions (as an unsigned int) but no magnitudes, 
             * sets the magnitude in each dimension as 0.0. This is the default constructor, with the default value being 1.
             */
            template <typename NUM>
            EuclideanVector(const NUM dimensions = 1): 
                  EuclideanVector(dimensions, 0.0) {}; // delegating constructor; default constructor that takes in dimensions if there is user input, otherwise dimensions = 1 if it is an empty constructor

            // target constructor for delegating constructor
            template <typename NUM1, typename NUM2> // any numeric types of user input for dimensions and magnitude will be static_cast to unsigned int and double respectively
            EuclideanVector(const NUM1 dimensions, const NUM2 magnitude){

                        // static cast to unsigned int and assign dimensions_ to that
                        dimensions_ = new unsigned int (static_cast<unsigned int>(dimensions));

                        // assign pointer "magnitude_" to dynamically-allocated memory of new unnamed array<double> object of size "dimensions_"
                        magnitude_  =  new double [*dimensions_];

                        // fill the array<double> object "magnitude_" a number of "dimensions_" times, with the <double> value of "magnitude_" for each dimension
                        std::fill_n(magnitude_, dimensions_, static_cast<double>(magnitude));

                  }

            /*
             * Destructor: ~EuclideanVector
             * The destructor deallocates any memory acquired by the constructors. 
             */
            ~EuclideanVector();


            /*
             * Member function: getMagnitude()
             * Returns a double containing the number of dimensions in a particular array.
             */
            const double& getMagnitude () const;


            /*
             * Member function: getNumDimensions()
             * Returns an unsigned int containing the number of dimensions in a particular vector.
             */
            unsigned int getNumDimensions() const;


        private:
        /* Everything from here to the end of the class is private, so
         * not accessible or intended for use for the client */

            unsigned int *dimensions_;
            double *magnitude_;
            //double normal_;
        };
}

#endif

The EuclideanVector.cpp file is this:

#include "EuclideanVector.h"
#include <algorithm>
#include <cmath> // for sqrt
#include <sstream>
#include <iterator>

namespace even {

    unsigned int EuclideanVector::getNumDimensions () const
    {
        return *dimensions_;
    }

    const double& EuclideanVector::getMagnitude () const
    {
        return *magnitude_;
    }

    // destructor
    EuclideanVector::~EuclideanVector() {
        delete dimensions_;
        delete [] magnitude_;
    }
}

and the EuclideanVectorTester.cpp file is this:

#include <iostream>
#include <vector>
#include <list>

#include "EuclideanVector.h"

int main() {

    std::cout << "1" << std::endl;

    evec::EuclideanVector a(2); 


    std::cout << "2" << std::endl;
    std::cout << "3" << std::end;

}

*I cannot use vector to store data* -- I love to hear the reason why you can't use `vector` -- you're using templates, namespaces, even STL algorithms, but can't use `std::vector`?? . And if you can't use `vector`, then create your own simple `vector` class, test it, and then use it. — PaulMcKenzie, Aug 28 '16 at 23:53
According to my professor, "What this means is that this is a vector that should be implemented by yourself. As you must represent your vector using raw memory, your obvious choice is to use primitive arrays. This is often how STL containers are implemented." — iteong, Aug 29 '16 at 00:05
"we are trying to create a context in which every student plays the role of a STL developer. Here, our smallish task is to develop a high-performance Euclidean vector. In principle, this is supposed to be a lowest-level API that must be efficiently implemented so that it can be used by the other higher-level library developers. Therefore, we are very much concerned with the efficiency of our own implementation. As in the STL vector, we can only achieve this by managing our memory usage ourself. It won't be as efficient to use another STL container, e.g., vector." — iteong, Aug 29 '16 at 00:07
"Creating a STL vector involves a lot of overhead in constructing a vector (including the metadata for managing its own memory). This will be pure overhead for our Euclidean vector. Then why do higher-level library developers from, say, software companies make use of the STL library instead of managing their own memory themselves? The shortest answer is "productivity". While the higher-level code thus developed may not be the fastest possible, software companies do end up enhancing their productivity in terms of code reuse, maintainability, and correctness." — iteong, Aug 29 '16 at 00:07
That's a "great" speech by the professor (sarcasm, as it is full of you know what). Too bad he / she didn't give the speech concerning copy constructor, assignment operator, destructor, exception safety, etc. etc. You're led to believe that beginners can implement this themselves -- I have yet to see a beginner implement this correctly by themselves without a ton of help form experienced C++ programmers. — PaulMcKenzie, Aug 29 '16 at 00:08
"The main reason for using raw memory is to give everyone a chance to understand and practise move semantics better due the presence of raw pointers. If you think about it, implementing a vector with an internal array will never be less efficient than if you use a vector. In our case, a single one-dimensional array will be allocated with no extra book-keeping baddage associated with the STL vector." — iteong, Aug 29 '16 at 00:10
@PaulMcKenzie Trust me I'd rather not use pointers lol and dynamic memory management. But what to do, it's the point of this assignment. =X — iteong, Aug 29 '16 at 00:12
Read my previous commnet. Unless one of us gives you the answer and basically fills in all the holes and bugs in your code, I have yet to see a beginner write such a class correctly. Sorry to pour cold water on this whole thing. I mean simple things like this you got wrong: `delete magnitude_;` That is the wrong form of `delete`. It should be `delete[]`. — PaulMcKenzie, Aug 29 '16 at 00:13
Yeah I just changed that it is still not working, that's a minor typo. — iteong, Aug 29 '16 at 00:19
Also, why is `dimesions_` a pointer to a single unsigned int? Why not just make it an unsigned int and just `static_cast` the parameter to assign to it? You gave 3 comments already on what your professor says is efficienct, yet you used the least efficient things to assign a simple integer. Guess what? You do that (make it an unsigned int instead of a pointer), and all your problems may be solved (given the `main` function you're using -- I can still easily break your `EuclideanVector` class with a simple 3 line `main` program). — PaulMcKenzie, Aug 29 '16 at 00:22
Okay I made dimensions_ a non-pointer private member and changed the rest of the code, it did solve the problem ... it did solve the problem. Am I allowed to do that based on what the professor said? I'm just not sure of the requirements... Does he only expect the dynamic memory allocation to happen with the storing of data in magnitude_, but no need that for dimensions_? — iteong, Aug 29 '16 at 00:34
Again, read my comments. Using `new` to allocate a single integer is less efficient (and quite honestly, kind of dumb to do). Your professor makes grandiose claims of efficiency, and then you resorted to use the least efficient thing. Just declare a simple int and assign to it. — PaulMcKenzie, Aug 29 '16 at 00:36
Hahahaha okay thanks anyway, let me double-check with my mates on what the requirement is, but at least now I resolved the issue. Hope he allows me to not use new for dimensions_ =P — iteong, Aug 29 '16 at 00:37
Apparently your professor doesn't want you to [read Knuth](http://c2.com/cgi/wiki?PrematureOptimization) until *after* his course. Not going to sugarcoat it; he grandstands like someone that is paid to talk about code; not *write* it. Best of luck ot you. — WhozCraig, Aug 29 '16 at 00:38
Guys!! I realised what was wrong with my previous code with the pointers declared for dimensions_ and magnitude_. I forgot to deference dimensions_ in std:fill_n. That is why there was a segmentation fault. Cheers! =D — iteong, Aug 29 '16 at 00:53
Consider compiling with '-O0 -g3' for better debug annotation in the output. — kfsone, Aug 29 '16 at 00:54

PaulMcKenzie · Accepted Answer · 2018-08-24T11:36:21.600

Let me just say that what your professor states has a lot of inaccuracies (to say the least).

Having said that, the issue is that you've declared dimensions to be an unsigned int*, but yet you're using it as if it is a plain unsigned int here:

std::fill_n(magnitude_, dimensions_, static_cast<double>(magnitude));

The immediate fix is to do this:

std::fill_n(magnitude_, *dimensions_, static_cast<double>(magnitude));

However, that begs the question of why a simple unsigned int needs to be a pointer, and then allocated using new. There is no reason for this, as using new is less efficient than what you're doing now.

If you had declared dimensions_ as this:

unsigned int dimensions_;

instead of it being a pointer, then the code to assign to dimensons_ becomes this:

// no call to new is done
dimensions_ = static_cast<unsigned int>(dimensions);

// no dereference of a pointer needs to be done on the two lines below 
magnitude_ = new double[dimensions_];
std::fill_n(magnitude_, dimensions_, static_cast<double>(magnitude));

No additional call to the allocator is done, thus the code instantly becomes more efficient.

In addition, the destructor now would look like this:

EuclideanVector::~EuclideanVector() 
{
    delete [] magnitude_;
}

But even with all of this stated, the answer given only fixes the issue if you use that very same main program to test with. If you changed main to this:

even::EuclideanVector a(2);
even::EuclideanVector b = a;

You will now run into copy semantics being incorrect. So again, the fix above is only required to make your main function work. Change main to something very simple as the above example, and you run into more issues.

Just to clarify, how come dereferencing magnitude_ is not needed in std::fill_n? Is it coz the first parameter of std::fill_n is an output parameter which should be a pointer? Whereas the second parameter is the count of elements to modify, so it needs to be dereferenced to get the value of dimensions_ instead of the pointer? — iteong, Aug 29 '16 at 01:06
@iteong -- Please look at the documentation [here](http://en.cppreference.com/w/cpp/algorithm/fill_n). Yes, you need to dereference the pointer, otherwise you gave it a count value that was equal to the pointer value (probably in the millions or even billions if your app was 64-bit). — PaulMcKenzie, Aug 29 '16 at 01:36

ERROR: AddressSanitizer: heap-buffer-overflow on address X at pc Y bp Z sp W

1 Answers1