Memory leak: CMemFile wrapper leaked on malformed OP_SERVERLIST packet in CServerSocket::ProcessPacket #885

• Criticality: Medium
• Component: Networking / Server protocol handling (CServerSocket::ProcessPacket)
• File: src/ServerSocket.cpp (lines 476–496; catch at 558–564)
• CWE: CWE-401 (Missing Release of Memory after Effective Lifetime)
• Reachability: Remote post-auth (connected server)
• Impact: Memory leak — one small heap object leaked per malformed OP_SERVERLIST packet; bounded resource exhaustion under sustained malicious input.

Summary

In CServerSocket::ProcessPacket, the OP_SERVERLIST handler allocates a CMemFile on the heap with a bare raw pointer (CMemFile* servers = new CMemFile(packet, size)) and only frees it via an unguarded delete servers at the end of the handler. The very first read (servers->ReadUInt8()) on an empty/zero-length payload, and the ReadUInt32/ReadUInt16 reads inside the parsing loop on a truncated payload, throw CEOFException. These exceptions propagate out of the CMemFile* scope to the function-level catch block (lines 558–564), which never frees servers. Because servers is a local raw pointer with no owning destructor, the CMemFile wrapper object is leaked on every malformed packet. The attach overload sets m_delete = false, so the underlying wire buffer is not leaked — only the fixed-size wrapper object — which is why this is rated Medium rather than higher.

Affected code

// src/ServerSocket.cpp
case OP_SERVERLIST: {
    AddDebugLogLineN(logServer, "Server: OP_SERVERLIST");

    CMemFile* servers = new CMemFile(packet,size);   // 476: raw owning pointer
    uint8 count = servers->ReadUInt8();              // 477: throws CEOFException on empty payload
    if (((int32)(count*6 + 1) > size)) {
        count = 0;
    }
    int addcount = 0;
    while(count) {
        uint32 ip	= servers->ReadUInt32();         // 483: throws on truncated entry
        uint16 port = servers->ReadUInt16();         // 484: throws on truncated entry
        CServer* srv = new CServer(
                    port ,				// Port
                    Uint32toStringIP(ip));	// Ip
        srv->SetListName(srv->GetFullIP());
        if (!theApp->AddServer(srv)) {
            delete srv;
        } else {
            addcount++;
        }
        count--;
    }
    delete servers;                                  // 496: skipped on any throw above
    if (addcount) {
        AddLogLineN(CFormat(wxPLURAL("Received %d new server", "Received %d new servers", addcount)) % addcount);
    }
    theApp->serverlist->SaveServerMet();
    AddLogLineN(_("Saving of server-list completed."));
    break;
}

The attaching constructor never throws once the buffer is non-null (packet->GetDataBuffer() is always non-null here), so the new always succeeds and servers is always assigned:

// src/MemFile.cpp:54
CMemFile::CMemFile(const uint8* buffer, size_t bufferSize)
{
    MULE_VALIDATE_PARAMS(buffer, "CMemFile: Attempted to attach invalid buffer.");
    m_buffer		= const_cast<uint8*>(buffer);
    m_BufferSize	= bufferSize;
    m_fileSize		= bufferSize;
    m_growthRate	= 0;
    m_position		= 0;
    m_delete		= false;   // wire buffer is NOT owned/freed by CMemFile
    m_readonly		= true;
}

The throwing read:

// src/SafeFile.cpp:83
if (Eof()) {
    throw CEOFException("Attempt to read past end of file.");
} else {
    throw CIOFailureException("Read error, failed to read from file.");
}

The function-level catch is outside the CMemFile* scope and does not free servers:

// src/ServerSocket.cpp:558
} catch (const CInvalidPacket& e) {
    AddLogLineN(CFormat( _("Bogus packet received from server: %s") ) % e.what());
} catch (const CEOFException& e) {
    AddLogLineN(CFormat( _("Bogus packet received from server: %s") ) % e.what());
} catch (const wxString& error) {
    AddLogLineN(CFormat( _("Unhandled error while processing packet from server: %s") ) % error);
}

Trigger / Attack vector

1. The client connects and authenticates to a (possibly malicious or compromised) eD2k server. ProcessPacket is invoked for each received packet (src/ServerSocket.cpp:138, dispatched from line 639).
2. The server sends an OP_SERVERLIST packet with an empty / zero-length payload. At line 477, servers->ReadUInt8() reads past end of file; CSafeMemFile detects Eof() and throws CEOFException (src/SafeFile.cpp:83).
   • Alternatively, the server sends a payload with a valid count byte but a truncated entry list, so servers->ReadUInt32() (483) or servers->ReadUInt16() (484) inside the loop throws CEOFException.
3. The exception unwinds past delete servers (line 496), which is therefore never executed, and is caught at the function-level handler (line 560). That handler logs the bogus packet but never frees servers.
4. The CMemFile wrapper object leaks. Repeating the malformed packet leaks one wrapper per packet. The underlying wire buffer is not leaked (m_delete == false), so each leak is small and bounded, but a malicious server can repeat this indefinitely.

Suggested fix

Make ownership exception-safe so the wrapper is released on every exit path. Preferred options:

• Use a stack object instead of a heap allocation — there is no reason to allocate CMemFile on the heap here:

  CMemFile servers(packet, size);
  uint8 count = servers.ReadUInt8();
  // ... use servers. ...
  // no delete needed; destroyed automatically, including on throw

• Or, if a pointer is required, wrap it in an RAII smart pointer (e.g. CScopedPtr<CMemFile> / std::unique_ptr<CMemFile>) so destruction occurs during stack unwinding and the explicit delete servers can be removed.

Either approach guarantees the CMemFile is freed when any of the ReadUInt8/ReadUInt32/ReadUInt16 calls throw.