[DavidXanatos]

in void CUpDownClient::AddReqBlock(Requested_Block_Struct* reqblock)

there is a check:

	for (POSITION pos = m_DoneBlocks_list.GetHeadPosition(); pos != 0; ){
 	const Requested_Block_Struct* cur_reqblock = m_DoneBlocks_list.GetNext(pos);
 	if (reqblock->StartOffset == cur_reqblock->StartOffset && reqblock->EndOffset == cur_reqblock->EndOffset){
 	delete reqblock;
 	return;
 	}
	}
	for (POSITION pos = m_BlockRequests_queue.GetHeadPosition(); pos != 0; ){
 	const Requested_Block_Struct* cur_reqblock = m_BlockRequests_queue.GetNext(pos);
 	if (reqblock->StartOffset == cur_reqblock->StartOffset && reqblock->EndOffset == cur_reqblock->EndOffset){
 	delete reqblock;
 	return;
 	}
	}

it should be howeever:

	for (POSITION pos = m_DoneBlocks_list.GetHeadPosition(); pos != 0; ){
 	const Requested_Block_Struct* cur_reqblock = m_DoneBlocks_list.GetNext(pos);
 	if (reqblock->StartOffset == cur_reqblock->StartOffset && reqblock->EndOffset == cur_reqblock->EndOffset
			&& memcmp(reqblock->FileID, cur_reqblock->FileID, 16) == 0){
 	delete reqblock;
 	return;
 	}
	}
	for (POSITION pos = m_BlockRequests_queue.GetHeadPosition(); pos != 0; ){
 	const Requested_Block_Struct* cur_reqblock = m_BlockRequests_queue.GetNext(pos);
 	if (reqblock->StartOffset == cur_reqblock->StartOffset && reqblock->EndOffset == cur_reqblock->EndOffset
			&& memcmp(reqblock->FileID, cur_reqblock->FileID, 16) == 0){
 	delete reqblock;
 	return;
 	}
	}

without checking the FileID the upload wil fail if a client after completing one file switches to an other and requests the same range

[fox88]

It seems to be a valid point, but as usual there was no respose from devs or code changes, as if this message was ignored.

Additionally, it is good practice to use sizeof() instead of constant wherever applicable. In this case:
memcmp(reqblock->FileID, cur_reqblock->FileID, sizeof(reqblock->FileID))

[Some Support]

Yes, might be a (in practice probably really rare) bug. I'm not actually sure if it is allowed to happen, you have to set the requested file first after all and can't just switch it in the reqblocks.

[fox88]

Some Support, on 01 March 2015 - 12:05 AM, said:

I'm not actually sure if it is allowed to happen, you have to set the requested file first after all and can't just switch it in the reqblocks.

Мisbehaving client are allowed to send any request or data any time.

[DavidXanatos]

Well its possible in the protocol so I would argue that was intended by the authors,
or else why waist 16 bytes in each request packet, to send the already set file ID.

So that should be fixed in the next release.

[tHeWiZaRdOfDoS]

This code has been in eMule+ for ages - just for the reference.

[fox88]

tHeWiZaRdOfDoS, on 01 March 2015 - 07:35 PM, said:

This code has been in eMule+ for ages - just for the reference.

Here the method AddReqBlock() starts at line 880, right?
I do not see hash comparison there, though it is used in AddRequestCount() with md4cmp() (should be preferable to memcmp()).
Maybe some other mod?