Czy w tej funkcji jest błąd?

Witajcie,

Debuguje program metodą jakiegoś jaskiniowca. (na Linux chyba nie ma dobrego debuggera, w Win to MSVC ma dobry). Czy w tej funkcji znajduje się błąd? Nie składniowy tylko działania. Kompiluje sie bez problemu tylko program rzuca mi błędami pamięci, teraz akurat takim *** Error in './bearchhost': double free or corruption (fasttop): 0x00000000009e75d0 *** Przerwane (zrzut pamięci) wcześniej był access violation, ale go znalazłem. Oto funkcja:

 void Node::addChild(Node* child)
{
	/* OBJASNIENIE, PROSZĘ PRZECZYTAĆ...
	 * header->itemChildrens = 0
	 * childrens = new Node[0];
	 * child to utworzona i uzupełniona wcześniej dynamicznie zaalokowana klasa. 
	 * Po wywolaniu tej funkcji to child jest juz usuwane
	 */

	Node* tempChildrens = new Node[header->itemChildrens + 1];
	std::memcpy(tempChildrens, childrens, header->itemChildrens*sizeof(Node));
	delete [] childrens;

	childrens = tempChildrens;
	childrens[header->itemChildrens] = *child;
	++header->itemChildrens;

	sort();
}

Oto klasa Node:

 class Node {
//private:
public:
	FileHeader* header;
	Node* childrens;
	Node* parent;
	static int compareNodes(Node* a, Node* b);
public:
	Node();
	~Node();

	void addChild(Node* child);
	...
	void sort();
	...
};

PS. Dodam może, że nadrzędną funkcją jest to:

Error Archive::mkDir(FilePath const& dirName)
{
	FileChildrenNum result = 0;
	Error err = actualNode->findChild(dirName, result);

	if(err == NOT_FOUND)
	{
		Node* tempNode = new Node();
		FileHeader* tempNodeHeader = 0;
		tempNode->getHeader() = tempNodeHeader = new FileHeader;

		std::memset(tempNodeHeader->itemName, 0, FileMaxName);
		std::strcpy(tempNodeHeader->itemName, dirName.c_str());
		tempNodeHeader->itemChildren = 0;
		tempNodeHeader->itemType = DIRECTORY;
		tempNodeHeader->dataFirst = 0;
		tempNodeHeader->dataLength = 0;

		actualNode->addChild(tempNode);
		delete tempNode;
	}
	else if(err == INVALID_PATH)
		return err;
	else if(err == IS_OK)
		return FILE_ALREADY_EXISTS;
	else
		return UNDEFINED_BEHAVIOR;

	return IS_OK;
}

Problem nie powstaje przy pierwszym wywołaniu mkDir tylko przy podwójnym wywołaniu tej funkcji (niezależnie od parametrów). Error w Linuxie to właśnie ten *** Error in './bearchhost': double free or corruption (fasttop): 0x00000000009e75d0 *** Przerwane (zrzut pamięci) wcześniej był access violation. Sprawdziłem wszystkie delete wywoływane przy funkcji i nie widze żadnych podwójnych.

• Po wywolaniu tej funkcji to child jest juz usuwane

Jest jakiś powód takiego kopiowania obiektu i niszczenia oryginału?
Nie lepiej po prostu "zawłaszczyć" ten obiekt, czyli skopiować wskaźnik i nie niszczyć obiektu?

childrens

liczba mnoga od “child” to “children”. nie ma czegoś takiego jak “childrens”.

Troche wróżenie z fusów.
Masz napisane że gdzieś zwalniasz pamięć 2 razy.
Co do debuggerów to masz przecież na linucha np kolejno:

• Nakładka na GDB w Code::Blocks
• DDD, nieco stary ale da sie używać, też nakładka
• No i samo GDB.

Moimi prehistorycznymi metodami doszedłem, że w funkcji mkDir w pierwszym wywołaniu delete tempNode; wywołuje się, a w drugim już nie i powoduje błąd double free. Tylko to są osobne zmienne lokalne i osobne miejsca w pamięci... Chyba, że optymalizacja kompilatora robi jakieś wtf.

PS. Te moje prehistoryczne metody to cout po ważnych momentach w kodzie i wyświetlanie jaki Error zwraca funkcja. Jak dojde do dokładnego miejsca w którym jest crash to między każdą linijką daje cout z informacją. Haha

przy błędach pamięciowych błąd klasy AV może Ci wyskoczyć w dowolnym miejscu.

Trochę lepsza wersja Twojego kodu (usuwa jeden potencjalny błąd):

 void Node::addChild(Node* child)
{
    size_t newSize = header->itemChildrens + 1;
    Node* tempChildrens = new Node[newSize];

    std::memcpy(tempChildrens, childrens, header->itemChildrens*sizeof(Node));

    std::swap(tempChildrens, childrens);
    delete [] tempChildrens;
 
    childrens[header->itemChildrens] = *child;
    header->itemChildrens = newSize;
 
    sort();
}

Edit: nie używaj strcpy, lepsze: strncpy.

Nadal jest ten błąd z usuwaniem tempNode w mkDir. Bardzo to dziwne. Przecież do Node zostaje włożona kopia tempNode, a wskaźnik nie jest zmieniany. Zastosowałem poprawkę @vpiotr, ale jednak błąd nie leży w dodawaniu nowego dziecka do Node. Zauważyłem, że w nowo stworzonym Node children jest NULL, a w destruktorze jest wywoływane delete [] children Już znalazłem winowajce pewnie. Zaraz sprawdzę.

PS. Błąd niby nadal jest.

bajos napisał(a):

PS. Błąd niby nadal jest.

Popraw jeszcze to:

tempNode->getHeader() = tempNodeHeader = new FileHeader;

(getHeader raczej nie jest do zmiany)

Funkcje w node tym który powoduje błąd wykonywane sa w tej kolejnosci. AddChildren jest wykonywane na nadrzędnym Node który z kolei takich błędów nie ma:

Node::Node()
{
	children = new Node[0];
	parent = 0;
	header = new FileHeader;
}

Node::~Node()
{
	/* Ta linijka ma blad */
	/* --->*/ delete [] children;
	/* Nie wiem jakim cudem to powoduje blad?
	 * Przecież nic nie jest usuwane/tworzone kilka razy jak sugeruje error */

	parent = 0;
	delete header;
}

Przecież to tak jak by napisać:

Node* children = new Node[0];
delete [] children;

Node* tempChildren = new Node[1];
delete [tempNode];

childrens[header->itemChildrens] = *child;
Nie masz operatora kopiowania, przez co wszystkie wskaźniki z klasy Node zostają przekopiowane, i jeśli w destruktorze masz delete[] wykonywane na nich, no to jak przekopiujesz tak jeden raz to 2 razy zwolnisz tą samą pamięć. A chyba nie muszę mówić, że zwalnianie tej samej pamięci w większych programach/grach jest koszmarem, bo może się zdarzyć tak, że ten sam obszar pamięci masz w dwóch wskaźnikach, na jednym zostanie wykonane delete[], na drugim zostanie wykonane później, ale w międzyczasie dokładnie ten sam obszar pamięci jest przekazywany innemu wskaźnikowi. I wtedy kompletnie "z d**y" nagle obszar, który dopiero co przydzieliliśmy jakiemuś wskaźnikowi zostaje magicznie zwolniony, i crashuje w losowych miejscach.

Wymyśliłem lepsze rozwiązanie. addChild() nie będzie przyjmowało parametru, a będzie zwracało wskaźnik na to dodane dziecko które będzie można ustawić według potrzeby. Może przeciążę operator= albo zrobię swap(). Przeczytałem na SO, że new[] allokuję sizeof(typ)*liczbaElementów+4 i przesuwa ten wskaźnik do przodu o sizeof(size_t). Ja będę musiał cofnąć ten wskaźnik na dzieci o cztery bajty i zrobić realloc() o rozmiarze sizeof(Node)*nowyRomiar+sizeof(size_t). Tylko wszystko ładnie, ale napisałem dla testu na http://ideone.com/DzAyGM , ale wynik zaalokowanej pamięci to 49. Nie powinno być sizef(int)*10+4 czyli 44? Jeszcze gryzie mnie czy w 64bitowym programie przed tym co zwraca new[] znajduje się liczba 4bajty czy 8bajtow? Tak to można by tylko alokować do 4GiB pamięci jednorazowo.

Co Ty tworzysz za wtf!

cout << "TabSize value: " << *tabSize << endl;

Undefined behavior.

Co to za bzdury z jakimś *sizeof(typ)liczbaElementów+4? T *x = new T[N] alokuje miejsce na N elementów typu T, przeprowadza odpowiednią inicjalizację i zwraca wskaźnik na pierwszy element w tak zaalokowanej pamięci. Nic więcej nie jest powiedziane. Wtf. Odczytywanie pamięci przed tym zaalokowanym obszarem za pomocą wskaźnika z tego obszaru to undefined behavior (5.7.5), dodatkowo po tym rzutowaniu używanie tego wskaźnika to też UB (3.10.10).

Jeszcze poczytałem posty wyżej. To co tam tworzysz to jakiś koszmar. To:

std::memcpy(tempChildrens, childrens, header->itemChildrens*sizeof(Node));

jest undefined behavior (3.8.1 i dalej).

Dodatkowo brak konstruktora kopiującego i operatora przypisania w klasie Node jest źródłem wszystkich problemów zapewne, ale o tym pisał @mwl4.

Jak to naprawić? Zacznij używać kontenerów z biblioteki standardowej.

Więc zrobie tak, że dzieci będe trzymał w char* o rozmiarze sizeof(Node)*liczbaDzieci. http://ideone.com/eL4UPU Wtedy jak bede zmienial rozmiar tej tablicy to nie wywolam destruktora tylko przeniose surowe dane. A jak bede chcial wywolac destruktor to można tak jak zwykłą funkcje.

PS. A przez addChild() bede zwracal wskaznik na nowy element, a nie przyjmowal nowy element. Wtedy nie mam szans na podwojne zwolnienie tej samej pamieci bo nie bedzie tymczasowego obiektu.

Musiałem zrobić jedną poprawkę w funkcji wyszukiwania binarnego bo trzeba było typ unsigned long zamienić na signed long long bo przy odejmowaniu od zera robiła się duża liczba i był błąd Naruszenie ochrony pamięci. Teraz program działa (zapewne do czasu dodania nowych ficzerów).

Haha Edit:
Jednak ta funkcja wyszukiwania binarnego zabugowana. Jeśli nie ma dzieci

Error Node::findChild(FilePath const& name, FileChildrenNum& result)
{
	if(name.length() > FileMaxName)
		return INVALID_PATH;

	if(header.itemChildren < 1)/* Valgrind wskazuje to jako Invalid read of size 4. Header nie jest już pointerem tylko jako zwykłe pole klasy (ani nie wskaźnik ani nie referencja). Faktycznie po wyswietleniu .itemChildren wynosi zero, a jednak coś nie tak*/
		return NOT_FOUND;

	long long int indexFirst = 0, indexEnd = header.itemChildren - 1 , indexCenter;
	const char* cname = name.c_str();

	while(indexFirst <= indexEnd)
	{
		indexCenter = (indexFirst+indexEnd) >> 1;

		if(std::strcmp(cname, children[indexCenter].header.itemName) == 0)
		{
			result = indexCenter;
			return IS_OK;
		}
		else if(std::strcmp(cname, children[indexCenter].header.itemName) < 0)
			indexEnd = indexCenter - 1;
		else
			indexFirst = indexCenter + 1;
	}

	return NOT_FOUND;
}

Edit 2:

/* To jest tylko funkcja do testowania nie jakiś program :D */
int main() {
	Bearch::Archive ar;
	Bearch::FilePath path = "archive.bearch";
	cout << ar.openArchive(path) << endl;

	path = "models";
	cout << ar.mkDir(path) << endl;


	path = "audio";
	cout << ar.mkDir(path) << endl;

	cout << ar.cd(path) << endl;
	cout << "Nazwa actual node: " << ar.actualNode->header.itemName << endl; /* Wysypuje sie po wypisaniu tego pierwszego  cudzysłowu*/
	/* Nie wyświetla tego po cudzysłowie czyli ze cos jest w tym wyswietlaniu nazwy.
	actualNode po wykonaniu cd() powinno wskazywać na to Node co nazywa sie "audio". ".cd(path)" zwraca zero więc teoretycznie tak
	powinno być.*/

	cout << "DONE!" << endl;
	return 0;
}

/* Funkcja splitPath działa na pewno przynajmniej przy takiej prostej ścieżce
czyli "audio". */
Error Archive::cd(FilePath const& dirName)
{
	FilePathStruct pathStruct = splitPath(dirName);
	Node* target = 0;
	Error err = getNodeByPath(pathStruct, target);

	if(err == IS_OK)
		actualNode = target;
	else
		return err;

	return IS_OK;
}

/* Przy path = splitPath(std::string("audio"))
 * path.count = 1;
 * path.word[0] = "audio";
 * path.local = true;
 */
Error Archive::getNodeByPath(FilePathStruct const& path, Node* resultNode)
{
	Node* tempNode = 0;

	if(path.local == true)
		tempNode = actualNode;
	else
		tempNode = rootNode;

	if(path.word[0] == "/")
	{
		resultNode = rootNode;	// TODO
		return IS_OK;
	}

	for(FileChildrenNum it=0; it<path.count;++it)
	{
		FileChildrenNum result = 0;
		Error searchErr = tempNode->findChild(path.word[it], result);

		if(searchErr == IS_OK)
		{
			tempNode = tempNode->getChild(result);
		}
		else
		{
			resultNode = reinterpret_cast<Node*>(it);
			return searchErr;
		}
	}

	resultNode = tempNode;
	return IS_OK;
}

Error Archive::mkDir(FilePath const& dirName)
{
	FileChildrenNum result = 0;
	Error err = actualNode->findChild(dirName, result);

	if(err == NOT_FOUND)
	{
		Node* tempNode = 0;
		tempNode = new Node;
		FileHeader* tempNodeHeader = tempNode->getHeader();

		std::strcpy(tempNodeHeader->itemName, dirName.c_str());
		tempNodeHeader->itemType = DIRECTORY;
		tempNode->setParent(actualNode);

		actualNode->addChild(tempNode);

		delete tempNode;
	}
	else if(err == INVALID_PATH)
		return err;
	else if(err == IS_OK)
		return FILE_ALREADY_EXISTS;
	else
		return UNDEFINED_BEHAVIOR;

	return IS_OK;
}