0

I have a class in my project that mainly stores std::vector elements and should be able to store and load a binary representation of an object to/from disc. When an object loads a file, it reads the number of elements to a variable and then the stored elements into an array. From this array, it pushes the elements into the vector member. I let C++ handle the memory as in I don't explicitly call calloc or new. Although it works fine, Valgrind gives me some error messages about invalid reads and writes that I don't understand. They seem to originate from the class destructor but as the class only has std::vector elements that are "dynamical" I should not have to do anything in there, or do I?

This is a working minimal example:

#include <vector>
#include <string>
#include <fstream>
#include <iostream>

class Test {
    public:
        Test() {}
        ~Test() {}

        void add(std::string s) { v.push_back(s); }
        std::vector<std::string>& get() { return v; }

        void store(char const* path) {
            std::ofstream file(path, std::ios_base::out | std::ios_base::binary);
            unsigned int n = v.size();
            file.write((char*) &n, sizeof(unsigned int));
            file.write((char*) v.data(), n*sizeof(std::string));
            file.close();
        }

        void read(char const* path) {
            std::ifstream file(path, std::ios_base::in | std::ios_base::binary);
            unsigned int n;
            file.read((char*) &n, sizeof(unsigned int));
            std::string in_v[n];
            file.read((char*) in_v, n*sizeof(std::string));
            v.clear();
            for (unsigned int i = 0; i < n; i++) {
                v.push_back(in_v[i]);
            }

            if (!file) { throw std::runtime_error("reading failed"); }
        }

    private:
        std::vector<std::string> v;
};

std::ostream& operator<<(std::ostream& os, Test& t) {
    for (unsigned int i = 0; i < t.get().size(); i++) {
        os << t.get()[i] << std::endl;
    }
    return os;
}

int main(int argc, char *argv[]) {
    Test a;
    a.add("foo");
    a.add("bar");

    std::cout << a << std::endl;

    a.store("file");

    //Test b;
    //b.read("file");
    //std::cout << "restored:" << std::endl << b << std::endl;

    return 0;
}

If I compile and run this in valgrind, everything works as expected and no leaks are detected:

==24891== Memcheck, a memory error detector
==24891== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==24891== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==24891== Command: ./dummy
==24891== 
foo
bar

==24891== 
==24891== HEAP SUMMARY:
==24891==     in use at exit: 72,704 bytes in 1 blocks
==24891==   total heap usage: 7 allocs, 6 frees, 81,528 bytes allocated
==24891== 
==24891== LEAK SUMMARY:
==24891==    definitely lost: 0 bytes in 0 blocks
==24891==    indirectly lost: 0 bytes in 0 blocks
==24891==      possibly lost: 0 bytes in 0 blocks
==24891==    still reachable: 72,704 bytes in 1 blocks
==24891==         suppressed: 0 bytes in 0 blocks
==24891== Rerun with --leak-check=full to see details of leaked memory
==24891== 
==24891== For counts of detected and suppressed errors, rerun with: -v
==24891== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

But as soon as I uncomment the last lines in the main() function, everything still works, but now valgrind prints some messages and I do not see why.

foo
bar

restored:
foo
bar

==4004== Invalid read of size 4
==4004==    at 0x4F04610: std::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string() (in /usr/lib64/libstdc++.so.6.0.21)
==4004==    by 0x4026CE: void std::_Destroy<std::string>(std::string*) (stl_construct.h:93)
==4004==    by 0x402534: void std::_Destroy_aux<false>::__destroy<std::string*>(std::string*, std::string*) (stl_construct.h:103)
==4004==    by 0x4021FD: void std::_Destroy<std::string*>(std::string*, std::string*) (stl_construct.h:126)
==4004==    by 0x401D76: void std::_Destroy<std::string*, std::string>(std::string*, std::string*, std::allocator<std::string>&) (stl_construct.h:151)
==4004==    by 0x401B5B: std::vector<std::string, std::allocator<std::string> >::~vector() (stl_vector.h:424)
==4004==    by 0x4016E5: Test::~Test() (dummy.cpp:9)
==4004==    by 0x4015B0: main (dummy.cpp:48)
==4004==  Address 0x5aa8c90 is 16 bytes inside a block of size 28 free'd
==4004==    at 0x4C2A184: operator delete(void*) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==4004==    by 0x4F04603: std::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string() (in /usr/lib64/libstdc++.so.6.0.21)
==4004==    by 0x4026CE: void std::_Destroy<std::string>(std::string*) (stl_construct.h:93)
==4004==    by 0x402534: void std::_Destroy_aux<false>::__destroy<std::string*>(std::string*, std::string*) (stl_construct.h:103)
==4004==    by 0x4021FD: void std::_Destroy<std::string*>(std::string*, std::string*) (stl_construct.h:126)
==4004==    by 0x401D76: void std::_Destroy<std::string*, std::string>(std::string*, std::string*, std::allocator<std::string>&) (stl_construct.h:151)
==4004==    by 0x401B5B: std::vector<std::string, std::allocator<std::string> >::~vector() (stl_vector.h:424)
==4004==    by 0x4016E5: Test::~Test() (dummy.cpp:9)
==4004==    by 0x4015A4: main (dummy.cpp:56)
==4004== 
==4004== Invalid write of size 4
==4004==    at 0x4F04616: std::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string() (in /usr/lib64/libstdc++.so.6.0.21)
==4004==    by 0x4026CE: void std::_Destroy<std::string>(std::string*) (stl_construct.h:93)
==4004==    by 0x402534: void std::_Destroy_aux<false>::__destroy<std::string*>(std::string*, std::string*) (stl_construct.h:103)
==4004==    by 0x4021FD: void std::_Destroy<std::string*>(std::string*, std::string*) (stl_construct.h:126)
==4004==    by 0x401D76: void std::_Destroy<std::string*, std::string>(std::string*, std::string*, std::allocator<std::string>&) (stl_construct.h:151)
==4004==    by 0x401B5B: std::vector<std::string, std::allocator<std::string> >::~vector() (stl_vector.h:424)
==4004==    by 0x4016E5: Test::~Test() (dummy.cpp:9)
==4004==    by 0x4015B0: main (dummy.cpp:48)
==4004==  Address 0x5aa8c90 is 16 bytes inside a block of size 28 free'd
==4004==    at 0x4C2A184: operator delete(void*) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==4004==    by 0x4F04603: std::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string() (in /usr/lib64/libstdc++.so.6.0.21)
==4004==    by 0x4026CE: void std::_Destroy<std::string>(std::string*) (stl_construct.h:93)
==4004==    by 0x402534: void std::_Destroy_aux<false>::__destroy<std::string*>(std::string*, std::string*) (stl_construct.h:103)
==4004==    by 0x4021FD: void std::_Destroy<std::string*>(std::string*, std::string*) (stl_construct.h:126)
==4004==    by 0x401D76: void std::_Destroy<std::string*, std::string>(std::string*, std::string*, std::allocator<std::string>&) (stl_construct.h:151)
==4004==    by 0x401B5B: std::vector<std::string, std::allocator<std::string> >::~vector() (stl_vector.h:424)
==4004==    by 0x4016E5: Test::~Test() (dummy.cpp:9)
==4004==    by 0x4015A4: main (dummy.cpp:56)
==4004== 
==4004== Invalid free() / delete / delete[] / realloc()
==4004==    at 0x4C2A184: operator delete(void*) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==4004==    by 0x4F04603: std::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string() (in /usr/lib64/libstdc++.so.6.0.21)
==4004==    by 0x4026CE: void std::_Destroy<std::string>(std::string*) (stl_construct.h:93)
==4004==    by 0x402534: void std::_Destroy_aux<false>::__destroy<std::string*>(std::string*, std::string*) (stl_construct.h:103)
==4004==    by 0x4021FD: void std::_Destroy<std::string*>(std::string*, std::string*) (stl_construct.h:126)
==4004==    by 0x401D76: void std::_Destroy<std::string*, std::string>(std::string*, std::string*, std::allocator<std::string>&) (stl_construct.h:151)
==4004==    by 0x401B5B: std::vector<std::string, std::allocator<std::string> >::~vector() (stl_vector.h:424)
==4004==    by 0x4016E5: Test::~Test() (dummy.cpp:9)
==4004==    by 0x4015B0: main (dummy.cpp:48)
==4004==  Address 0x5aa8c80 is 0 bytes inside a block of size 28 free'd
==4004==    at 0x4C2A184: operator delete(void*) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==4004==    by 0x4F04603: std::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string() (in /usr/lib64/libstdc++.so.6.0.21)
==4004==    by 0x4026CE: void std::_Destroy<std::string>(std::string*) (stl_construct.h:93)
==4004==    by 0x402534: void std::_Destroy_aux<false>::__destroy<std::string*>(std::string*, std::string*) (stl_construct.h:103)
==4004==    by 0x4021FD: void std::_Destroy<std::string*>(std::string*, std::string*) (stl_construct.h:126)
==4004==    by 0x401D76: void std::_Destroy<std::string*, std::string>(std::string*, std::string*, std::allocator<std::string>&) (stl_construct.h:151)
==4004==    by 0x401B5B: std::vector<std::string, std::allocator<std::string> >::~vector() (stl_vector.h:424)
==4004==    by 0x4016E5: Test::~Test() (dummy.cpp:9)
==4004==    by 0x4015A4: main (dummy.cpp:56)
==4004== 
==4004== 
==4004== HEAP SUMMARY:
==4004==     in use at exit: 72,704 bytes in 1 blocks
==4004==   total heap usage: 11 allocs, 12 frees, 90,296 bytes allocated
==4004== 
==4004== LEAK SUMMARY:
==4004==    definitely lost: 0 bytes in 0 blocks
==4004==    indirectly lost: 0 bytes in 0 blocks
==4004==      possibly lost: 0 bytes in 0 blocks
==4004==    still reachable: 72,704 bytes in 1 blocks
==4004==         suppressed: 0 bytes in 0 blocks
==4004== Rerun with --leak-check=full to see details of leaked memory
==4004== 
==4004== For counts of detected and suppressed errors, rerun with: -v
==4004== ERROR SUMMARY: 6 errors from 3 contexts (suppressed: 0 from 0)

Where do these errors come from? I expect it is something with the read method but then why does valgrind shows errors from the creation of a ("dummy.cpp:48") where nothing were wrong before?

mable
  • 61
  • 1
  • 6
  • 1
    `std::string in_v[n];` is a VLA and might be what triggered it. If the array size is something that is not known you should use a `std::vector` – NathanOliver Feb 09 '17 at 16:57
  • 2
    `file.read((char*) in_v, n*sizeof(std::string));` looks _really_ dodgy – Mike Vine Feb 09 '17 at 17:18
  • @NathanOliver thanks for the advice, I just tried `v.resize(n);` and `file.read((char*) v.data(), n*sizeof(std::string));`. This works as well but I still get the valgrind errors. Same if I explicitly allocate `std::stiring* in_v = new std::string[n]` and `delete[]` it at the end of the method. – mable Feb 09 '17 at 17:23
  • 1
    Oh. Silly me I never even went futher to see that you are trying to read a `std::string` in binary mode. You really can't do that. `std::string` is a dynamic object as as such needs to be serialized. You can't just use `read` and `write` on it as you wont capture the data the string points to. I'll try and find you a good dupe – NathanOliver Feb 09 '17 at 17:27
  • I can't find a good dupe right now but see this: http://stackoverflow.com/questions/234724/is-it-possible-to-serialize-and-deserialize-a-class-in-c – NathanOliver Feb 09 '17 at 17:31
  • @MikeVine honestly I don't really understand why the read and write methods need `char*` to the data but so far this approach I used worked so I just came to terms with it. What do you find dogdy about it? – mable Feb 09 '17 at 17:33
  • See @NathanOliver's comments. std::string is a (non-pod) `class`. You cant just blast bytes into and out of a class. You must serialise/deserialse them properly. It may 'work' sometimes but it'll at least corrupt memory and at works plain break everything. – Mike Vine Feb 09 '17 at 17:47
  • @MikeVine alright, got it. Thank you both! I'm still modifying the methods but not reading/writing with std::string wiped away the valgrind errors (got an other one instead but that's a different story...) – mable Feb 09 '17 at 18:15
  • @NathanOliver if you post your comment as an answer I can accept it as solution :) – mable Feb 09 '17 at 18:15

1 Answers1

1

This line - 

file.read((char*) in_v, n*sizeof(std::string));

Is causing undefined behavior later on the road. You are trying to read some characters into a std::string object which was not designed for it. Doing so, you are probably running over internal data (probably pointers), and accessing them will cause UB

Instead, you should use std::vector<char>, reserve enough bytes to hold the string you ate reading from the file, and read into vec.data()

user1708860
  • 1,683
  • 13
  • 32