Official eMule-Board: Fix For Two Bugs/flaws In Aich Recovery Hashset Code - Official eMule-Board

Jump to content


Page 1 of 1

Fix For Two Bugs/flaws In Aich Recovery Hashset Code

#1 User is offline   netfinity 

  • Master of WARP
  • PipPipPipPipPipPipPip
  • Group: Members
  • Posts: 1658
  • Joined: 23-April 04

Posted 19 August 2013 - 07:22 PM

First one is rather serious and will cause parsing recovery data from client to fail when we got two or more corrupt parts we asking recovery data for. The problem is that FindExistingHash is not recursing to itself but rather to the FindHash method, which will create the hash node/leaf rather than just trying to locate it. This will cause problem when receiving recovery data from a client as it is for only one part and will not populate all leafs which will cause hash tree validation to fail.
const CAICHHashTree* CAICHHashTree::FindExistingHash(uint64 nStartPos, uint64 nSize, uint8* nLevel) const
{
    (*nLevel)++;
    if (*nLevel > 22){ // sanity
        ASSERT( false );
        return false;
    }
    if (nStartPos + nSize > m_nDataSize){ // sanity
        ASSERT ( false );
        return NULL;
    }
    if (nSize > m_nDataSize){ // sanity
        ASSERT ( false );
        return NULL;
    }

    if (nStartPos == 0 && nSize == m_nDataSize){
        // this is the searched hash
        return this;
    }
    else if (m_nDataSize <= GetBaseSize()){ // sanity
        // this is already the last level, cant go deeper
        ASSERT( false );
        return NULL;
    }
    else{
        uint64 nBlocks = m_nDataSize / GetBaseSize() + ((m_nDataSize % GetBaseSize() != 0 )? 1:0); 
        uint64 nLeft = ( ((m_bIsLeftBranch) ? nBlocks+1:nBlocks) / 2)* GetBaseSize();
        uint64 nRight = m_nDataSize - nLeft;
        if (nStartPos < nLeft){
            if (nStartPos + nSize > nLeft){ // sanity
                ASSERT ( false );
                return NULL;
            }
            if (m_pLeftTree == NULL)
                return NULL;
            else{
                ASSERT( m_pLeftTree->m_nDataSize == nLeft );
#ifdef FIX_FINDEXISTINGHASH
                return m_pLeftTree->FindExistingHash(nStartPos, nSize, nLevel);
#else
                return m_pLeftTree->FindHash(nStartPos, nSize, nLevel);
#endif
            }
        }
        else{
            nStartPos -= nLeft;
            if (nStartPos + nSize > nRight){ // sanity
                ASSERT ( false );
                return NULL;
            }
            if (m_pRightTree == NULL)
                return NULL;
            else{
                ASSERT( m_pRightTree->m_nDataSize == nRight );
#ifdef FIX_FINDEXISTINGHASH
                return m_pRightTree->FindExistingHash(nStartPos, nSize, nLevel);
#else
                return m_pRightTree->FindHash(nStartPos, nSize, nLevel);
#endif
            }
        }
    }
}



The second one is not really a big problem, unless you decide to put more data behind a chunk of recovery data. For example if you write a sequence of recovery data chunks in the part.met file. The problem is that when 16bit identifiers are used the CreateRecoveryData method adds a counter of value 0 after the recovery data set to signify that there are no 32bit identifiers, but the ReadRecoveryData method never reads this last counter causing the file pointer offset to be wrong when exiting the method.
bool CAICHRecoveryHashSet::ReadRecoveryData(uint64 nPartStartPos, CSafeMemFile* fileDataIn){
    if (/*TODO !m_pOwner->IsPartFile() ||*/ !(m_eStatus == AICH_VERIFIED || m_eStatus == AICH_TRUSTED) ){
        ASSERT( false );
        return false;
    }
    /* V2 AICH Hash Packet:
        <count1 uint16>                                            16bit-hashs-to-read
        (<identifier uint16><hash HASHSIZE>)[count1]            AICH hashs
        <count2 uint16>                                            32bit-hashs-to-read
        (<identifier uint32><hash HASHSIZE>)[count2]            AICH hashs
    */

    // at this time we check the recoverydata for the correct ammounts of hashs only
    // all hash are then taken into the tree, depending on there hashidentifier (except the masterhash)

    uint8 nLevel = 0;
    uint32 nPartSize = (uint32)min(PARTSIZE, (uint64)m_pOwner->GetFileSize()-nPartStartPos);
    m_pHashTree.FindHash(nPartStartPos, nPartSize,&nLevel);
    uint16 nHashsToRead = (uint16)((nLevel-1) + nPartSize/EMBLOCKSIZE + ((nPartSize % EMBLOCKSIZE != 0 )? 1:0));
    
    // read hashs with 16 bit identifier
    uint16 nHashsAvailable = fileDataIn->ReadUInt16();
    if (fileDataIn->GetLength()-fileDataIn->GetPosition() < nHashsToRead*(HASHSIZE+2) || (nHashsToRead != nHashsAvailable && nHashsAvailable != 0)){
        // this check is redunant, CSafememfile would catch such an error too
        theApp.QueueDebugLogLine(/*DLP_VERYHIGH,*/ false, _T("Failed to read RecoveryData for %s - Received datasize/amounts of hashs was invalid (1)"), m_pOwner->GetFileName() );
        return false;
    }
    DEBUG_ONLY( theApp.QueueDebugLogLine(/*DLP_VERYHIGH,*/ false, _T("read RecoveryData for %s - Received packet with  %u 16bit hash identifiers)"), m_pOwner->GetFileName(), nHashsAvailable ) );
    for (uint32 i = 0; i != nHashsAvailable; i++){
        uint16 wHashIdent = fileDataIn->ReadUInt16();
        if (wHashIdent == 1 /*never allow masterhash to be overwritten*/
            || !m_pHashTree.SetHash(fileDataIn, wHashIdent,(-1), false))
        {
            theApp.QueueDebugLogLine(/*DLP_VERYHIGH,*/ false, _T("Failed to read RecoveryData for %s - Error when trying to read hash into tree (1)"), m_pOwner->GetFileName() );
            VerifyHashTree(true); // remove invalid hashs which we have already written
            return false;
        }
    }

    // read hashs with 32bit identifier
    if (nHashsAvailable == 0 && fileDataIn->GetLength() - fileDataIn->GetPosition() >= 2){
        nHashsAvailable = fileDataIn->ReadUInt16();
        if (fileDataIn->GetLength()-fileDataIn->GetPosition() < nHashsToRead*(HASHSIZE+4) || (nHashsToRead != nHashsAvailable && nHashsAvailable != 0)){
            // this check is redunant, CSafememfile would catch such an error too
            theApp.QueueDebugLogLine(/*DLP_VERYHIGH,*/ false, _T("Failed to read RecoveryData for %s - Received datasize/amounts of hashs was invalid (2)"), m_pOwner->GetFileName() );
            return false;
        }
        DEBUG_ONLY( theApp.QueueDebugLogLine(/*DLP_VERYHIGH,*/ false, _T("read RecoveryData for %s - Received packet with  %u 32bit hash identifiers)"), m_pOwner->GetFileName(), nHashsAvailable ) );
        for (uint32 i = 0; i != nHashsToRead; i++){
            uint32 wHashIdent = fileDataIn->ReadUInt32();
            if (wHashIdent == 1 /*never allow masterhash to be overwritten*/
                || wHashIdent > 0x400000
                || !m_pHashTree.SetHash(fileDataIn, wHashIdent,(-1), false))
            {
                theApp.QueueDebugLogLine(/*DLP_VERYHIGH,*/ false, _T("Failed to read RecoveryData for %s - Error when trying to read hash into tree (2)"), m_pOwner->GetFileName() );
                VerifyHashTree(true); // remove invalid hashs which we have already written
                return false;
            }
        }
    }
#ifdef FIX_READRECOVERYDATA
    // netfinity: New versions of CreateRecoveryData always write a 16bit integer here, so we have to read it or skip it
    else if (fileDataIn->GetLength() - fileDataIn->GetPosition() >= 2)
    {
        fileDataIn->ReadUInt16();
    }
#endif
    
    if (nHashsAvailable == 0){
        theApp.QueueDebugLogLine(/*DLP_VERYHIGH,*/ false, _T("Failed to read RecoveryData for %s - Packet didn't contained any hashs"), m_pOwner->GetFileName() );
        return false;
    }


    if (VerifyHashTree(true)){
        // some final check if all hashs we wanted are there
        for (uint32 nPartPos = 0; nPartPos < nPartSize; nPartPos += EMBLOCKSIZE){
            CAICHHashTree* phtToCheck = m_pHashTree.FindHash(nPartStartPos+nPartPos, min<uint64>(EMBLOCKSIZE, nPartSize-nPartPos));
            if (phtToCheck == NULL || !phtToCheck->m_bHashValid){
                theApp.QueueDebugLogLine(/*DLP_VERYHIGH,*/ false, _T("Failed to read RecoveryData for %s - Error while verifying presence of all lowest level hashs"), m_pOwner->GetFileName() );
                return false;
            }
        }
        // all done
        return true;
    }
    else{
        theApp.QueueDebugLogLine(/*DLP_VERYHIGH,*/ false, _T("Failed to read RecoveryData for %s - Verifying received hashtree failed"), m_pOwner->GetFileName() );
        return false;
    }
}



Regards,
netfinity
eMule v0.50a [NetF WARP v0.3a]
- Compiled for 32 and 64 bit Windows versions
- Optimized for fast (100Mbit/s) Internet connections
- Faster file completion via Dynamic Block Requests and dropping of stalling sources
- Faster searching via KAD with equal or reduced overhead
- Less GUI lockups through multi-threaded disk IO operations
- VIP "Payback" queue
- Fakealyzer (helps you chosing the right files)
- Quality Of Service to keep eMule from disturbing VoIP and other important applications (Vista/7/8 only!)
4

#2 User is offline   Some Support 

  • Last eMule
  • PipPipPipPipPipPipPip
  • Group: Yes
  • Posts: 3667
  • Joined: 27-June 03

Posted 20 August 2013 - 07:50 AM

Indeed, esp. the first one is really some old bug from the beginning of aich hashes. Thanks :)

  • Member Options

Page 1 of 1

1 User(s) are reading this topic
0 members, 1 guests, 0 anonymous users