[fox88]

Some Support, on 12 October 2014 - 11:49 AM, said:

I don't remember that anything changed in the tag extraction code, but I'll put checking it on my todo list.

I don't think you did; but probably M$ helped.
Debug build fails (including your devbuild), release builds do not.

It was interesting to investigate this issue because eMule here crashes somewhat similar to KAD ~CEntry() failures.
eMule gets exception in _orphan_ptr() in sdt::list. Pointed value there is compared to zero, while the value is 0xcdcdcdcd (allocated, yet uninitialized memory). Hence it either loops forever, or gets access violation.
That iterator validation code appeared in VS2005; naturally VS2003 builds were doing fine.
Search in the net came up with the link to an old msdn discussion

Here's the fix.
Add virtual destructors to corresponding classes in id3lib\include\id3\tag.h
virtual ~Iterator() {};
and
virtual ~ConstIterator() {};

Now debug version can read mp3 tags again.

PS. Afterwards I checked development code of the library, and fournd that tag.h file already had those missing lines.

Edit.
After reading Virtuality article I decided to try to downgrade public destuctor ~ID3_FieldImpl(); to protected in file id3lib\src\field_impl.h
Appears to be working so far.

This post has been edited by fox88: 30 November 2014 - 11:55 AM

[Some Support]

Thanks. Mhm, if we change the library we have to include the source into our repository which is a bit annyoing (but it seems cannot be avoided if the fix is needed).

[fox88]

I downloaded development code of id3lib. It compiles, but fails to start (by the way, the same is true for ResizableLib both 1.4a and development).
The id3lib project stays unchanged for quite a while; no bug fixes and also people tell there is no proper support for Unicode and other issues.
Maybe a more promising option would be to try taglib instead. Also it reads a number of file formats in addition to mp3.

This post has been edited by fox88: 05 December 2014 - 08:02 PM

[Tuxman]

Maybe an even more promising option would be to drop internal libs and stick to MediaInfo at all.

[Enig123]

Remind me of the fact that eMule cannot report the width x height of a .wmv file, which might due to eMule only use microsoft sdk for that kind of file info. The whole downloaded .wmv fileinfo can be read correctly, through Mediainfo I guess.

[Riso64Bit]

Enig123, on 05 December 2014 - 10:42 PM, said:

Remind me of the fact that eMule cannot report the width x height of a .wmv file, which might due to eMule only use microsoft sdk for that kind of file info. The whole downloaded .wmv fileinfo can be read correctly, through Mediainfo I guess.

as already written via pm to SS, sometimes emule not detect the bitrate to some video files. Maybe is a similar problem found by Enig123?

This post has been edited by Riso64Bit: 06 December 2014 - 12:00 AM

[fox88]

Enig123, on 06 December 2014 - 12:42 AM, said:

the width x height of a .wmv file, which might due to eMule only use microsoft sdk for that kind of file info.

http://microsoft.pub...ight-attributes
The best thing here is that both .wmv and sdk were invented by MS guys.

[fox88]

While running debug version with overlapped sockets I got an assert which might indicate a possible trouble.

BaseClient.cpp, line: 271
ASSERT( m_nConnectingState == CCS_NONE || !theApp.emuledlg->IsRunning() );

eMule was running, hence the first condition should be false.
Then it means the destructor CUpDownClient::~CUpDownClient was called before complete disconnect.

This post has been edited by fox88: 13 October 2016 - 12:24 PM

[fox88]

fox88, on 29 November 2014 - 08:27 PM, said:

It was interesting to investigate this issue because eMule here crashes somewhat similar to KAD ~CEntry() failures.

Using this idea, I made changes in srchybrid\kademlia\kademlia\Tag.h.
That is, added

virtual ~CKadTagNameString()
{}

and

virtual ~CKadTagValueString()
{}

Running eMule for a month did not give a single crash in ~CEntry() with enabled overlapped sockets.
Two weeks ago I tried to get in touch with Ergol by PM for more testing, but he was offline so far.

PS. Earlier I also replaced
typedef std::list<CKadTag*> TagList;
with
typedef CList<CKadTag*> TagList;
But that did not put an end to crashes in ~CEntry().
My current code is still using CList instead of std::list in TagList, because implementation of CList is slightly better.

This post has been edited by fox88: 31 January 2015 - 07:39 PM

[Enig123]

Thanks fox88 for your work on this issue.

This bugfix will leave me with the debug that with slot focus feature, the standby clients won't survive with the new overlapped sockets. I have reported this before, SS said it's slot focus's sides fault.

Edit: Haha!, I found the culprit, SS is right about this.

This post has been edited by Enig123: 31 January 2015 - 09:03 PM

[fox88]

It was not the most obvious idea to search the bug in stable KAD instead of new overlapped socket code.
After the change the persistent crash never happened in my case; the same was reported by Enig123.

There is a good reason to refresh project's state; and maybe even start beta testing.

[Some Support]

fox88, on 11 February 2015 - 05:25 PM, said:

It was not the most obvious idea to search the bug in stable KAD instead of new overlapped socket code.
After the change the persistent crash never happened in my case; the same was reported by Enig123.

There is a good reason to refresh project's state; and maybe even start beta testing.

Sorry that I haven't answered to this yet, but it'll take a bit for me to look into it and figure out if it's related to the overlapped crashes. I'm fairly sure that the bug isn't in the new code itself because it really aren't that many lines and I have checked them all. More likely it is indeed on old existing bug which surfaces now due to the changed behavior, possible thread related. I'll at least put up an update dev version soon if this indeed fixed it for you.

[Ergol]

Some Support, on 12 February 2015 - 02:14 AM, said:

I'll at least put up an update dev version soon if this indeed fixed it for you.

Ready to test.

Hope it helps.

[Enig123]

Noticed some log lines which bother me:

2/17/2015 10:09:33 PM: CEncryptedStreamSocket: Received wrong magic value from clientIP xxx.xxx.xxx.xx on a supposly encrytped stream / Wrong Header
2/17/2015 10:09:33 PM: Client TCP socket: Error: Encryption layer error; IP=211.161.159.14
2/17/2015 10:09:33 PM: Error: Invalid or corrupted packet received - while processing eDonkey packet: opcode=OP_HELLO  size=69; IP=xxx.xxx.xxx.xx

in which the TCP socket was decrypted without success, while still feed to the protocol handling process, and created the corrupted packet error.

Is there a simple way to avoid such unsuccessfully decrypted packets been processed?

[Some Support]

Enig123, on 18 February 2015 - 06:21 AM, said:

Noticed some log lines which bother me:

2/17/2015 10:09:33 PM: CEncryptedStreamSocket: Received wrong magic value from clientIP xxx.xxx.xxx.xx on a supposly encrytped stream / Wrong Header
2/17/2015 10:09:33 PM: Client TCP socket: Error: Encryption layer error; IP=211.161.159.14
2/17/2015 10:09:33 PM: Error: Invalid or corrupted packet received - while processing eDonkey packet: opcode=OP_HELLO  size=69; IP=xxx.xxx.xxx.xx

in which the TCP socket was decrypted without success, while still feed to the protocol handling process, and created the corrupted packet error.

Is there a simple way to avoid such unsuccessfully decrypted packets been processed?

You could add an additional error handling for this of course. But I don't think it's necessary. After all there is always a chance that the package is just unencrypted (it shouldn't happen but neither should it ever be corrupted) and if it is junk it's handled just the same way as junk packets which aren't sent on an encrypted stream.

[fox88]

Once upon a time I managed to click top left big Connect button so that ED2K never connected.
The reason in the log was: empty server list, but there was a handful of servers, no static lists in use, and Reconnect on loss was checked.

Stepping through startup sequence I saw that KAD was initilized first somewhere before CemuleDlg::StartupTimer(), then file list was loaded, then servers list.
There were 400+ shared files and not too fast computer, and the conclusion became quite obvious: connection was attempted before servers were loaded.

Of course it is a minor problem, but also it should be rather simple to fix.
For exapmle, by disabling 3 connect buttons until all the necessary things are ready to go. That moment might be expected near the lines

if (thePrefs.DoAutoConnect())
	theApp.emuledlg->OnBnClickedConnect();

in CemuleDlg::StartupTimer().

[Enig123]

Some Support, on 11 February 2015 - 04:14 PM, said:

fox88, on 11 February 2015 - 05:25 PM, said:

It was not the most obvious idea to search the bug in stable KAD instead of new overlapped socket code.
After the change the persistent crash never happened in my case; the same was reported by Enig123.

There is a good reason to refresh project's state; and maybe even start beta testing.

Sorry that I haven't answered to this yet, but it'll take a bit for me to look into it and figure out if it's related to the overlapped crashes. I'm fairly sure that the bug isn't in the new code itself because it really aren't that many lines and I have checked them all. More likely it is indeed on old existing bug which surfaces now due to the changed behavior, possible thread related. I'll at least put up an update dev version soon if this indeed fixed it for you.

SS, it seems you're right about the bug new socket codes triggers. I have encountered a weird crash of eMule, when the socket close caused a client instance been disconnected and then deleted, while when perform RemoveSource of downloadqueue, it crashed at that point. Restart eMule indicates that the crash happened when a file has been completed and moved to the incoming directory, eMule has to re-hash it after restarting.

Edit1: As I double-checked the code, since RemoveAllSources of a completed CPartFile is performed in the main thread, that casual crash appears more likely been triggered by the socket codes, which I believe belongs to another thread.

Edit2: The crash was at line 704 of DownloadQueue.cpp speaking of the latest svn codes, in the stack the invoke can be traced to the sockets which is been disconnected for some reason which then triggers the deletion of that instance of CUpDownClient.

Edit3: There's multiple files been downloading while the crash happened, hence AFAF sources positive.

This post has been edited by Enig123: 21 February 2015 - 09:29 PM

[fox88]

Enig123, on 22 February 2015 - 12:07 AM, said:

I have encountered a weird crash of eMule, when the socket close caused a client instance been disconnected and then deleted, while when perform RemoveSource of downloadqueue, it crashed at that point. Restart eMule indicates that the crash happened when a file has been completed and moved to the incoming directory, eMule has to re-hash it after restarting.

Edit1: As I double-checked the code, since RemoveAllSources of a completed CPartFile is performed in the main thread, that casual crash appears more likely been triggered by the socket codes, which I believe belongs to another thread.

Edit2: The crash was at line 704 of DownloadQueue.cpp speaking of the latest svn codes, in the stack the invoke can be traced to the sockets which is been disconnected for some reason which then triggers the deletion of that instance of CUpDownClient.

See message #308.
Should you be running debug build, you probably would get that assert too.

[Enig123]

fox88, on 21 February 2015 - 02:09 PM, said:

See message #308.
Should you be running debug build, you probably would get that assert too.

No, not the same kind of crash. I am pretty sure the m_nConnectingState be CCS_NONE when the crash happened.

Edit: I have managed to solve this problem by putting the invoke of CPartFile::RemoveAllSources to the main thread in order to avoid the rare thread conflict.

This post has been edited by Enig123: 23 February 2015 - 05:39 AM

[Enig123]

Finally I think the cause of my heap corruption has been figured out. When a downloading file is completing, if we received OP_FILESTATUS, which obviously will trigger SwapToAnotherFile() action, which may have thread conflict with another SwapToAnotherFile() when hashing the file is done and CPartFile::RemoveAllSources() and ended up with heap corruption.

I have two different crashes been caught after download file completed, and DoSwap() leave with 'Unsync between parfile->srclist and client otherfiles list' logs before both crashes.

The fix from my side is as following:

				case OP_FILESTATUS:
				{
					if (thePrefs.GetDebugClientTCPLevel() > 0)
						DebugRecv("OP_FileStatus", client, (size >= 16) ? packet : NULL);
					theStats.AddDownDataOverheadFileRequest(size);

					CSafeMemFile data(packet, size);
					uchar cfilehash[16];
					data.ReadHash16(cfilehash);
					CPartFile* file = theApp.downloadqueue->GetFileByID(cfilehash);
					if (file == NULL)
						client->CheckFailedFileIdReqs(cfilehash);
					//Enig123::fixed a rare crash bug around AFAF handling, after a file is completed while still received OP_FILESTATUS
					else if (file->GetStatus() == PS_COMPLETE || file->GetStatus() == PS_COMPLETING)
					{
						AddDebugLogLine(false, _T("client send FileStatus during the file is being completed, don't process: client '%s' file '%s'"), client->DbgGetClientInfo(), file->GetFileName());
						break;
					}
					//Enig123::end
					client->ProcessFileStatus(false, &data, file);
					break;
				}

After applying this patch, the crash seems never happened again. I dig a little deeper, and found the extra OP_FILESTATUS was asked by introducing NetFinity's Delayed NNP, which tried to ask for the latest partstatus from a source became NNP.

Since there's buggy clients out there sending OP_FILESTATUS in no time, the patch seems to do no harm, and prevented potential heap corruption.

Need developer's opinions on this.

Edit: I thought it was caused by thread conflict, later after I tried to put the RemoveAllSources invokes to the main thread, eMule still crashes, which made me think that maybe two consecutive invoke of SwapToAnotherFile() actions is enough to cause the heap corruption I spotted in this case.

Edit2: The trying to ask for the latest part status before put the source to real NNP would merely made the heap corruption emerge earlier. Even if I commented that that part of code, the "client send FileStatus during the file is being completed" still in the log sometimes.

This post has been edited by Enig123: 02 March 2015 - 03:41 AM