false leaks in wxZipOutputStream?? Topic is solved

[maxbld]

Dear all,
If I run the code below, VS2010 memory leaks detector built in the CRT Debug Library reports leaks. If I explicitly delete all the wxZipEntry pointers I get, the leaks go away, but I fee like it's wrong: in the wxZipOutputStream::Close() method the entries are deleted and the evidence is that if I delete it before tempZipOutput.Close();
the Close method gives an exception because memory can't be read, if I delete the entries with delete(*entry); after tempZipOutput.Close(); the delete( gives same exception in its turn.

Code: Select all

	wxFileOutputStream tempZipOut(sFileName);
	wxZipOutputStream tempZipOutput(tempZipOut);
	wxString sInFileName;
	wxString CurrDir = wxGetCwd();

	wxFileInputStream zipFileIn(sInFileName);
wxZipInputStream zipIn(zipFileIn);
	wxZipEntry **entry, **pBegin;
	int iTotEntries = zipIn.GetTotalEntries();
	entry = (wxZipEntry**) calloc(iTotEntries, sizeof(wxZipEntry*));
	
	while(*entry=zipIn.GetNextEntry()){
		if(iEntriesCount == 0)
			pBegin = entry;
		iEntriesCount++;
		if(*entry!=NULL){
                        //I do my stuff with the entries
			//if(iEntriesCount < iTotEntries){
				tempZipOutput.CloseEntry();
				//zipIn.CloseEntry();
				//delete(entry);
				//entry = NULL;
			//}
		}
		entry++;
	}
	

	tempZipOutput.Close();
	/*
	  the memory leaks detector built in the CRT Debug Library senses the entries are not deleted in tempZipOutput.Close(); but it's
	  wrong, actually they are in d:\wxWidgets-2.9.5\src\common\zipstrm.cpp ln. 2320 in the wxZipOutputStream::Close() method. 
	  vld exits without claiming
	entry = pBegin;
	for(int i=0; i<iTotEntries; i++){
		delete(*entry);
		entry++;
	}
	*/
	tempZipOut.Close();

Is anybody aware of this behavior, can you please confirm the built in leak detector is wrong?

Thanks a lot, BR,
Max.

[doublemax]

I've never used wxZipOutputStream and without a test case i can't test it myself.

But the docs clearly say that wxZipInputStream::GetNextEntry() gives away ownership, so i would guess that you have to delete the pointer.

Also you have to delete "wxZipEntry **entry" yourself, none of the wxWidgets classes know about it.

I don't know if it's relevant for your testing, but in this out-commented code:

Code: Select all

//zipIn.CloseEntry();
//delete(entry);
//entry = NULL;

i think it should be

Code: Select all

delete(*entry);
*entry = NULL;

when it's commented in.

On a side note: I personally find it more convenient to access zip files using wxFileSystem with wxZipFSHandler.

[maxbld]

But the docs clearly say that wxZipInputStream::GetNextEntry() gives away ownership, so i would guess that you have to delete the pointer.

Yes. true and that's why I say it's deleted in wxZipOutputStream::Close(). If you read the source code of zipstrm.cpp ln. 2320, you'll see that it loops the list of the entries it got from wxZipInputStream::GetNextEntry() and deletes one by one, and I've seen in debug the pointers addresses are the same. So the leaks detector must be mistaken when it says there's a memory leak...

[doublemax]

I think i never had a false memory leak report in Visual Studio.

Can you try with VLD?
https://vld.codeplex.com/

[maxbld]

Can you try with VLD?

Yes. I wanted to write it in my post, but I forgot. Codeplex VLD is clean on same code, only the memory leaks detector built in the CRT Debug Library reports leaks. I assume the latter checks the pointers only against the destructor of the wxZipInputStream that generated it. In this case, since the pointers to wxZipEntry objects are traded to the wxZipOutputStream object that takes it in its wxZipEntryList_ m_entries private member, the built in detector gets fooled. Actually the delete of the entries takes place in tempZipOutput.Close(); when method wxZipOutputStream::Close() is called, in it the following code is executed:

Code: Select all

    for (it = m_entries.begin(); it != m_entries.end(); ++it) {
        size += (*it)->WriteCentral(*m_parent_o_stream, GetConv());
        delete *it;
    }
    m_entries.clear();

That means to me all wxZipEntry objects are dealt with, I saw it happening in debug and the pointers' addresses where same as those given by *entry=zipIn.GetNextEntry(). Furthermore when I delete it on my own, tempZipOutput.Close() throws an exception because it can't read the memory.

I take these things as an evidence that the memory leaks detector built in the CRT Debug Library is in error and Codeplex VLD is correct, do you agree?

Thank you, BR,
Max.

[doublemax]

Could you post the real code you're currently using for the test. I'd like to try it myself.

Code: Select all

while(*entry=zipIn.GetNextEntry()){

BTW: I think this line will write over the end of the allocated array after the last element. I noticed it when i tried your code and added a free(pBegin) at the end. It reported a heap corruption.

[maxbld]

Thank you doublemax for your interest in my issue. Actual code is the following. There's a bit of logic to take the input and output files, then the contents of the input file (.docx) are used as a M$ word template to write a table from a recordset into a M$ word table. As you most likely know, M$ docs are written as zip files, so I look for word/document.xml from the template file, I parse it and I write the data to the output .docx file. That means entries from wxZipInputStream zipIn are either copied if they're not word/document.xml or put if they're word/document.xml in wxFileOutputStream tempZipOut, hence at the end of the while, all wxZipEntry* entry are in tempZipOut and tempZipOut.Close() is supposed to delete it and I really think it does as I've written in my previous posts, but the memory leaks detector built in the CRT Debug Library still whines.

Code: Select all

 ***Edit: Code removed. Somehow business sensitive...

[doublemax]

Hmm, i was hoping for a simple test case that i could just paste into the "minimal" sample and run...

Anway, if we suspect the CopyEntry() call to be the culpit, the following code should leak, too. But it doesn't.

Code: Select all

wxString sFileName( wxT("D:\\_zip_test_out.zip") );
wxFileOutputStream tempZipOut(sFileName);
wxZipOutputStream tempZipOutput(tempZipOut);

wxString sInFileName( wxT("D:\\_zip_test_in.zip") );
wxString CurrDir = wxGetCwd();

wxFileInputStream zipFileIn(sInFileName);
wxZipInputStream zipIn(zipFileIn);

wxZipEntry *entry;
while( (entry=zipIn.GetNextEntry())!=NULL )
{
  tempZipOutput.CopyEntry( entry, zipIn );
}

tempZipOutput.Close();
tempZipOut.Close();

There must still be something else going on. (Tested with VS2003, 2005 and 2010).

[maxbld]

Right, your code doesn't leak. but look at this one:

Code: Select all

	wxZipInputStream zipIn(zipFileIn);
	wxZipEntry* entry;

	while( (entry=zipIn.GetNextEntry())!=NULL )
	{
	  //tempZipOutput.CopyEntry( entry, zipIn );
	  tempZipOutput.PutNextEntry(entry->GetName());
	  part = wxT("Test");
	  tempZipOutput.Write(part.mb_str(wxConvUTF8), part.mb_str(wxConvUTF8).length());
	}

	tempZipOutput.Close();
	tempZipOut.Close();

It leaks... Something must be wrong in PutNextEntry or in Write...

[doublemax]

Code: Select all

tempZipOutput.PutNextEntry(entry->GetName());

The PutNextEntry() call that only takes a wxString as parameter can't take ownership of "entry", it's doesn't know about it.

[maxbld]

Thank you. You rock a lot. Everybody knows on this forum, but it's good to remind it every now and then!

Anyway this means that VLD is malfunctioning for this case... I've read somewhere that VLD was more reliable than its counterpart in the CRT Debug Library, but this doesn't look the case, so I'd say it's important to always double check the results from VLD with the VS builtin LD.

BR,
Max.

[doublemax]

Thanks

wxWidgets temporarily uses smart pointers for the zip-entries. Maybe that confuses VLD somehow (although it shouldn't)