Vector po destrukcji powoduje memory corruption

0

Witam,
Napisałem sobie kodzik do wczytywania danych z plików obj.
Użyłem wektorów. Niestety używanie mojej klasy która tworzyła wiele kilka wektorów powoduje w dalszej części programu memory corruption.

Zdaję sobie sprawe z możliwości użycia funkcji do kilku fragmentów, tak było - zaniechałem tej idei celem sprawdzenia czy zmiana kontekstu ma jakieś znaczenie.
Zmienne file oraz filename to pola klasy (zainicjalizowane, dane wczytywane są poprawnie do this->vertices, this->normals.
Błąd nadal występuje - program przejdzie przez ten fragment kodu, lecz w dalszym etapie wykonywania zupełnie innego kodu nastąpi memory corruption(w zupełnie innym juz kontekście).

Bez względu na to czy na końcu metody process wywołam delete dla każdego wskaźnika dla wektora czy nie - problem występuje.

void ObjReader::process()
{
    verticesAmount = 0;
    texturesAmount = 0;
    normalsAmount = 0;

    vector<float> *vertices = new vector<float>();
    vector<float> *textures = new vector<float>();
    vector<float> *normals = new vector<float>();

    vector<int> *verticesIndexes = new vector<int>();
    vector<int> *texturesIndexes = new vector<int>();
    vector<int> *normalsIndexes = new vector<int>();

    file.open(filename);

    string tempString;

    while (!file.eof())
    {
        file >> tempString;

        if(tempString == "v")
        {
            ObjReader::readThreeAndPushEm(vertices);
        }
        else if(tempString == "vt")
        {
            ObjReader::readThreeAndPushEm(textures);
        }
        else if(tempString == "vn")
        {
            ObjReader::readThreeAndPushEm(normals);
        }
        else if(tempString == "f")
        {
            for (int i = 0; i < 3; i++)
            {
                file>>tempString;
                replaceEmptyTextureIndexToZero(tempString);

                char *temp;
                char *tempString2 = const_cast<char*>(tempString.c_str());

                for (int j = 0; j < 3; ++j)
                {
                    j==0 ? temp = strtok(tempString2, "/")
                         : temp = strtok(nullptr, "/");

                    int index = static_cast<int>(strtol(temp, nullptr, 10) - 1);

                    if(index != -1)
                    {
                        switch (j)
                        {
                            case 0:
                                verticesIndexes->push_back(index);
                                break;
                            case 1:
                                texturesIndexes->push_back(index);
                                break;
                            case 2:
                                normalsIndexes->push_back(index);
                                break;
                            default:
                                break;
                        }
                    }
                }
            }
        }
        else
        {
            std :: cout << "Ignored obj data from file " <<  filename << " :" << tempString << "\n";
        }
    }

    this->vertices = new float[verticesIndexes->size()*4];
    this->normals = new float[normalsIndexes->size()*4];

    int iterator = 0;

    for (int k = 0; k < verticesIndexes->size(); ++k)
    {
        for (int i = 0; i < 4; ++i)
        {
            this->vertices[iterator++] = (*vertices)[(*verticesIndexes)[k] + i];
        }

        this->vertices[iterator++] = 1.0f;
    }

    iterator = 0;

    for (int k = 0; k < normalsIndexes->size(); ++k)
    {
        for (int i = 0; i < 4; ++i)
        {
            this->normals[iterator++] = (*normals)[(*normalsIndexes)[k] + i];
        }

        this->normals[iterator++] = 0.0f;
    }
}

void ObjReader::readThreeAndPushEm(vector<float> *vectorToPushTo)
{
    float tempFloat;
    file >> tempFloat;
    vectorToPushTo->push_back(tempFloat);
    file >> tempFloat;
    vectorToPushTo->push_back(tempFloat);
    file >> tempFloat;
    vectorToPushTo->push_back(tempFloat);
}

void ObjReader::replaceEmptyTextureIndexToZero(string &basic_string)
{
    for (int i = 1; i < basic_string.size(); ++i)
    {
        if(basic_string[i-1] == '/'  &&  basic_string[i] == '/')
        {
            basic_string.insert(static_cast<unsigned long>(i), "0");
            break;
        }
    }
}
1

Nie widzę bezpośredniego powodu, jesteś pewien że jest we wskazanym fragmencie? Masz dziwną fascynację z wciskaniem new wszędzie gdzie się da, nawet tam gdzie znacząco utrudnia zrozumienie kodu. Poza nagminnym new (polecam lekturę: https://dsp.krzaq.cc/post/176/ucze-sie-cxx-kiedy-uzywac-new-i-delete/ ), po co przesłaniasz nazwy zmiennych klasy? To jakieś zajęcia z zaciemniania kodu?

Strzelam, że nadpisujesz coś poza zakresem zaalokowanych tablic, albo łamiesz zasadę 3/5/0 i masz double delete, bez MCVE ciężko powiedzieć: https://dsp.krzaq.cc/post/445/jak-zadawac-pytania-na-forum/

W każdym razie wywaliłbym wszystkie new z kodu i użył std::vector wszędzie. Dopiero jak uzyskałbym działający program to brałbym się za ewentualne optymalizacje.

    verticesAmount = 0;
    texturesAmount = 0;
    normalsAmount = 0;

Nigdzie dalej nie ruszasz tych zmiennych. I amount jest niepoliczalny, użyj count.

0

"amount jest niepoliczalny" to implikuje, że nie można go inkrementować?

Od końca:
Fakt, nigdzie nie używam w tym co podesłałem tych 3 zmiennych - były w oryginale.
Wywaliłem te wskaźniki i jest standardowy vector.
Zajęcia z zaciemniania - racja.

MVCE w załączniku, tak jak i screen gdzie się wywala u mnie.

0

using namespace std; w nagłówku. Jesteśmy zgubieni!

0
Pijak napisał(a):

using namespace std; w nagłówku. Jesteśmy zgubieni!

Otóż nie.

0

Amount to ilość, a Ciebie interesuje liczba. https://english.stackexchange.com/questions/9439/amount-vs-number-vs-quantity

Podrzuć jeszcze plik block.obj

0

Jest w katalogu cmake-build-debug.
A co do amount... chyba regex na dysku puszcze xD.

2
[pts/0:krzaq@ArchVM:~/downloads/4pro]% g++ main.cpp ObjReader.cpp -fsanitize=address -g && ./a.out
Ignored obj data from file block.obj :#
Ignored obj data from file block.obj :Blender
Ignored obj data from file block.obj :v2.68
Ignored obj data from file block.obj :(sub
Ignored obj data from file block.obj :0)
Ignored obj data from file block.obj :OBJ
Ignored obj data from file block.obj :File:
Ignored obj data from file block.obj :'block.blend'
Ignored obj data from file block.obj :#
Ignored obj data from file block.obj :www.blender.org
Ignored obj data from file block.obj :s
Ignored obj data from file block.obj :off
=================================================================
==25635==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6160000008c0 at pc 0x55cc1f8d7da6 bp 0x7fff02557110 sp 0x7fff02557100
WRITE of size 4 at 0x6160000008c0 thread T0
    #0 0x55cc1f8d7da5 in ObjReader::process() /home/krzaq/downloads/4pro/ObjReader.cpp:89
    #1 0x55cc1f8d744f in main /home/krzaq/downloads/4pro/main.cpp:11
    #2 0x7fe720276222 in __libc_start_main (/usr/lib/libc.so.6+0x24222)
    #3 0x55cc1f8d734d in _start (/home/krzaq/remote/c/Users/KrzaQ/Downloads/4pro/a.out+0x234d)

0x6160000008c0 is located 0 bytes to the right of 576-byte region [0x616000000680,0x6160000008c0)
allocated by thread T0 here:
    #0 0x7fe720837f19 in operator new[](unsigned long) /build/gcc/src/gcc/libsanitizer/asan/asan_new_delete.cc:93
    #1 0x55cc1f8d7ac6 in ObjReader::process() /home/krzaq/downloads/4pro/ObjReader.cpp:77
    #2 0x55cc1f8d744f in main /home/krzaq/downloads/4pro/main.cpp:11
    #3 0x7fe720276222 in __libc_start_main (/usr/lib/libc.so.6+0x24222)

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/krzaq/downloads/4pro/ObjReader.cpp:89 in ObjReader::process()
Shadow bytes around the buggy address:
  0x0c2c7fff80c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2c7fff80d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2c7fff80e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2c7fff80f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2c7fff8100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c2c7fff8110: 00 00 00 00 00 00 00 00[fa]fa fa fa fa fa fa fa
  0x0c2c7fff8120: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2c7fff8130: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2c7fff8140: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2c7fff8150: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2c7fff8160: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
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
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==25635==ABORTING

czyli gdzieś tu:

    for (int k = 0; k < verticesIndexes.size(); ++k)
    {
        for (int i = 0; i < 4; ++i)
        {
            this->vertices[iterator++] = vertices[verticesIndexes[k] + i];
        }

        this->vertices[iterator++] = 1.0f; // <--
    }

Używaj wszędzie wektorów. Nigdzie nie używaj new. Zamiast [] używaj .at(). Optymalizacje później.

Np, po kiego grzyba:

    auto *objReader = new ObjReader("block.obj");
    objReader->process();

zamiast

    ObjReader objReader("block.obj");
    objReader.process();
0

¯_(ツ)_/¯ , bo przekazywałem dalej wskaźnik - czemu wskaźnik a nie referencje spytasz? Może sobie lubie utrudniać czy coś, ale tak się już przywyczaiłem.
Znalazłem babola. Te informacje które podesłałeś były kluczowe aby mi wskazać, że przy redukowaniu funkcji do jednego kontekstu zapomniałem zmienić 4 na 3 w tych wewnętrznych forach.
Funkcja miała zupełnie inną postać.

Czy jesteś mi może w stanie podpowiedzieć dlaczego segmentation fault na etapie pisania do niezaalokowanej pamięci tablicy nie poleciał - tylko jakieś dzikie błędy podczas czyszczenia vectorów?

0

Taka natura undefined behavior, może zdarzyć się wszystko. Wskaźnik można dalej przekazać używając operatora &, btw. Choć jak nie przewidujesz nullptr, to użyj systemu typów, aby to wymusić.

I jeszcze jedno: polub się z sanitizerami (szczególnie address i undefined), bardzo ułatwiają życie, jak widać.

1

Przedmówców przepraszam jeśli coś powtórzyłem, TL;DR

Rzeczy do poprawki:

  1. zamień:
vector<float> *vertices = new vector<float>();

na

vector<float> vertices;

(i tak dla 5 pozostałych wektorów)

  1. w liniii:
ObjReader::readThreeAndPushEm(vertices);

powinieneś przekazać referencję:

ObjReader::readThreeAndPushEm(*vertices)

albo po zmianie (1) zostawić tak jak jest

  1. w linii:

this->vertices = new float[verticesIndexes->size()*4];

coś tu jest bardzo nie tak.
a) brak sprytnych wskaźników
b) nie wiadomo po co *4
c) używasz tej samej nazwy co zmienna lokalna w funkcji

  1. tej konstrukcji nie znam. Może jakiś spec od standardów potwierdzi czy to ma prawo dzałać - w końcu wynik ?: nie jest nigdzie przypisywany.
                    j==0 ? temp = strtok(tempString2, "/")
                         : temp = strtok(nullptr, "/");

  1. tam gdzie jest file.open, tam też powinno być file.close

  2. ta linia może zawierać błąd pamięciowy:

j==0 ? temp = strtok(tempString2, "/")

Dlaczego:

  • c_str jest typu const char *
  • strtok modyfikuje argument

Prosta poprawka: http://www.cplusplus.com/reference/string/string/c_str/

0
  1. Przypisuje w wyrażeniach, to jest identyczne do
if(j==0) {
    temp = strtok(tempString2, "/")
} else {
    temp = strtok(nullptr, "/");
}

Tylko bardziej cool. W tym przypadku czytelność imo podobna, ale patrząc po Tobie, jednak mniejsza ;​)

  1. albo lokalny obiekt dla pliku i RAII to obsłuży
0

Może to tylko kwestia smaku: https://www.quora.com/Should-I-utilise-the-ternary-operator-over-a-simple-if-statement

Ja bym to raczej zapisał zamiast tak:

j==0 ? temp = strtok(tempString2, "/")
                         : temp = strtok(nullptr, "/");

np. tak:

temp = (j == 0 ? strtok(tempString2, "/") : 
  strtok(nullptr, "/"));

Ale nie testowałem czy to będzie działać tak samo

1 użytkowników online, w tym zalogowanych: 0, gości: 1