[C++] std::list.erase geeft soms een assert error

[Wortelpudding]

Ik heb 2 classes: CReferenceBuffer en CSyncFIFO.
CReferenceBuffer is een buffer class die bedoeld is om vanuit meerdere threads gebruikt te worden. Hiermee kan ik (net zoals in COM) mbv AddRef en Release de refcount verhogen en verlagen. Als de RefCount 0 is wordt hij vrijgegeven.
Deze class gebruik ik in de class CSyncFIFO. Dit is een implementatie van een synchronized FIFO. Deze class wordt ook vanuit meerdere threads gebruikt (1 thread om buffers toe te voegen en 1 om ze weer uit de FIFO te halen).
Voor CSyncFIFO heb ik ook nog een typedef:

C++:

typedef std::list<CReferenceBuffer*> TBufferList;

Dit is de class-declaratie van CSyncFIFO:

C++:

class CSyncFIFO
{
protected:
    HANDLE      hListMutex;
    TBufferList blFifo;
public:
    CSyncFIFO();
    virtual ~CSyncFIFO();

    CReferenceBuffer* Pop(void);
    bool Push(CReferenceBuffer* prbBuffer);
};

Vanuit de ene thread (producer) wordt dus een CReferenceBuffer instance gemaakt, deze wordt mbv Push in de FIFO gezet. De andere thread (consumer) haalt mbv Pop weer de eerste buffer in de FIFO op.
In deze Pop functie zit het probleem. Ik zal eerst even de code van Pop posten om het wat duidelijker te maken:

C++:

CReferenceBuffer* CSyncFIFO::Pop()
{
    TBufferList::iterator  iBuffers;
    CReferenceBuffer*     prbResult = NULL;

    WaitForSingleObject(hListMutex, INFINITE);

    iBuffers = blFifo.begin();
    if (iBuffers != blFifo.end())
    {
        prbResult = *iBuffers;
        blFifo.erase(iBuffers);
    }

    ReleaseMutex(hListMutex);

    return prbResult;
}

Het probleem dat ik heb zit hem bij blFifo.erase(iBuffers). Af en toe krijg ik hier de volgende assert melding:
_BLOCK_TYPE_IS_VALID(pHead->nBlockUse) in DBGHEAP.C regel 1050.

Teruggaand op de callstack kom ik bij regel 12 in de voorgaande code uit.
De code in regel 11 gaat gewoon goed.

Iemand een idee?

[.oisyn]

Laat je push code ook eens zien? Misschien gaat er daar iets fout (bijvoorbeeld dat ie niet goed wacht op de mutex)

En welke compiler gebruik je?


Trouwens, waarom gebruik je geen zo'n constructie:

C++:

if (!blFifo.empty ())
{
    prbResult = blFifo.front ();
    blFifo.pop_front ();
}

Dat scheelt weer een aanmaak van een iterator, en voorkomt ook dat die iterator op een een of andere manier invalidated is

[ Voor 50% gewijzigd door .oisyn op 05-05-2003 12:54 ]

[Wortelpudding]

Push code:

C++:

bool CSyncFIFO::Push(CReferenceBuffer *prbBuffer)
{
    WaitForSingleObject(hListMutex, INFINITE);

    blFifo.insert(blFifo.end(), prbBuffer);
    prbBuffer->AddRef();

    ReleaseMutex(hListMutex);

    return true;
}

@.oisyn: Ik probeer je suggestie gelijk uit . ==> Helaas

[ Voor 15% gewijzigd door Wortelpudding op 05-05-2003 13:35 ]

[Wortelpudding]

(Ik gebruik overigens Microsoft Visual C++ 6 service pack 5).

Hmmm, als ik met behulp van _CrtSetDbgFlag de flag _CRTDBG_CHECK_ALWAYS_DF aanzet, (waardoor voor elke new en delete de functie _CrtCheckMemory wordt aangeroepen) dan krijg ik al een ASSERT bij regel 5 van de Push functie uit mijn vorige post. Het lijkt er dus op dat de heap op de een of andere manier corrupt raakt
Is het mogelijk dat er een bug zit in de STL van Microsoft die dit veroorzaakt?

[farlane]

Kunnen insert en erase misschien exceptions geven ? ( Ik zie geen exception handling in je code namelijk )

[Wortelpudding]

farlane schreef op 05 May 2003 @ 15:25:
Kunnen insert en erase misschien exceptions geven ? ( Ik zie geen exception handling in je code namelijk )

Die ASSERT lijkt me al rigoreus genoeg
Nee, de ASSERT komt in beide gevallen in de functies (dus in insert/erase/pop_front), dus ze krijgen niet eens de gelegenheid om een exception te geven.

Call stack:

code:

_heap_alloc_dbg(unsigned int 0x0000000c, int 0x00000001, const char * 0x00000000, int 0x00000000) line 332 + 41 bytes
_nh_malloc_dbg(unsigned int 0x0000000c, int 0x00000000, int 0x00000001, const char * 0x00000000, int 0x00000000) line 248 + 21 bytes
_malloc_dbg(unsigned int 0x0000000c, int 0x00000001, const char * 0x00000000, int 0x00000000) line 165 + 27 bytes
operator new(unsigned int 0x0000000c) line 325 + 15 bytes
std::_Allocate(int 0x0000000c, char * 0x00000000) line 30 + 9 bytes
std::allocator<CReferenceBuffer *>::_Charalloc(unsigned int 0x0000000c) line 62 + 11 bytes
std::list<CReferenceBuffer *,std::allocator<CReferenceBuffer *> >::_Buynode(std::list<CReferenceBuffer *,std::allocator<CReferenceBuffer *> >::_Node * 0x01269cd0, std::list<CReferenceBuffer *,std::allocator<CReferenceBuffer *> >::_Node * 0x03546fc8) line 387 + 10 bytes
std::list<CReferenceBuffer *,std::allocator<CReferenceBuffer *> >::insert(std::list<CReferenceBuffer *,std::allocator<CReferenceBuffer *> >::iterator {...}, CReferenceBuffer * const & 0x01815d70) line 219 + 27 bytes
CSyncFIFO::Push(CReferenceBuffer * 0x01815d70) line 288
AudioDispatcher(void * 0x00d449d8) line 194
KERNEL32! 77e7d33b()

[.oisyn]

Dit soort vage errors wijzen meestal op of het gebruik van al opgeruimd geheugen (dus bijvoorbeeld een pointer dereferencen nadat je 'm hebt gedelete), of het meerdere keren vrijgeven van hetzelfde stukje geheugen.

Waarschijnlijk zit de fout dus ergens in je reference-counting mechanisme, of je hebt ergens anders in je programma dergelijke fouten zitten

[MSalters]

Strict genomen zou de oorzaak kunnen zijn dat prbBuffer naar een invalide adres wijst (bv eerder gedelete CReferenceBuffer) maar VC6 maakt daar geen gebruik van.
De call naar std::_Allocate is volstrekt normaal (12 bytes, geen hint) dus op dat moment moet de heap al corrupt zijn.

Is het een optie om een andere allocator op te scharrelen? Dan kun je (in theorie) zelfs per TBufferList, en in elk geval voor alle TBufferLists samen een apart geheugengebied gebruiken. Het resultaat is dan dat je kunt vaststellen of TBufferList de geheugencorruptie veroorzaakt.

[farlane]

Kan het nog zijn dat je WaitForSingleObject niet met WAIT_OBJECT_0 terugkomt maar met een WAIT_FAILED en dat je daarna je list trashed ?

( Of heb je in je originele code wel meer retval checking staan? Je doet er in deze code weining aan, beetje bad practice )

Anders denk ik dat je idd iets niet lekker hebt zitten in je refcounting mechanisme, of je trashed je list met een andere bufferoverflow oid )

[ Voor 8% gewijzigd door farlane op 05-05-2003 20:11 ]

[.oisyn]

Die WAIT_FAILED error zou ook nog kunnen idd... Ben je niet gewoon vergeten je mutex aan te maken in de constructor van CSyncFIFO?

[curry684]

.oisyn schreef op 05 May 2003 @ 20:21:
Die WAIT_FAILED error zou ook nog kunnen idd... Ben je niet gewoon vergeten je mutex aan te maken in de constructor van CSyncFIFO?

Gezien het gedeelte 'af en toe' in de originele post en dat ik me nog kan herinneren waarom MrHuge deze queue maakt (de 240 audiomessages per seconde) lijkt dit me idd de meest waarschijnlijke oorzaak. Verder lijkt het me onmogelijk dat de fout in te vaak dereferencen zit daar hij wel een AddRef doet bij de Push en geen Release bij de Pop, dus het object wordt waarschijnlijker nooit gedelete dan te vaak

[.oisyn]

curry: het verkeerd Release'n van de pointer kan natuurlijk ook elders in het programma voorkomen

Ik weet niet hoe z'n Release () method in elkaar zit, maar dit gaat natuurlijk gegarandeerd fout op de manier zoals boven is beschreven (vage malloc () fouten)

C++:

class Object
{
private:
    int numRef;

public:
    Object () : numRef (1) { }

    void AddRef () { numRef++; }
    void Release ()
    {
        if (--numRef <= 0)
            delete this;
    }
};

int main ()
{
    Object * o = new Object ();
    o->Release ();
    o->Release ();   // bliep! heap corruption
}

Een assert () in de AddRef en Release functies die controleert of numRef > 0 lijkt me dan het beste

[curry684]

Nog een stoere mogelijke source van bugs:

C++:

int AddRef()
{
return ++m_ReferenceCount;
}

int Release()
{
if(!--m_ReferenceCount)
  delete this;
return m_ReferenceCount;
}

Spot the bug

[ Voor 19% gewijzigd door curry684 op 05-05-2003 22:32 . Reden: Whoops ]

[.oisyn]

Daar heb je meestal nog de mazzel dat m_ReferenceCount niet opnieuw uitgelezen wordt (of je moet 'm als volatile gedefinieerd hebben ), en dus m_ReferenceCount inleest in een register, die vervolgens decrement, dat opslaat en test (en evt. een delete this), en vervolgens die waarde retourneren

Hoewel de delete this natuurlijk weer een function call wordt, waardoor de waarde in ebx, esi of edi moet komen te staan om behouden te blijven, waardoor je de vorige waarde van dat register weer moet pushen en pop'en op de stack, waardoor het niet efficienter wordt 'm in een register te laten staan, en dus opnieuw wordt uitgelezen uit het al vrijgegeven geheugen (snap je het nog? )

[Wortelpudding]

.oisyn schreef op 05 May 2003 @ 16:47:
Waarschijnlijk zit de fout dus ergens in je reference-counting mechanisme, of je hebt ergens anders in je programma dergelijke fouten zitten

C++:

ULONG STDMETHODCALLTYPE CReferenceBuffer::AddRef(void)
{
    ULONG lResult = InterlockedIncrement(&lRefCount);

    return lResult;
}

ULONG STDMETHODCALLTYPE CReferenceBuffer::Release(void)
{
    ULONG lResult = InterlockedDecrement(&lRefCount);

    if ((iBufferIndex != -1) && (lResult == 0))
    {
        LDLog.AddLineToLog("WARNING: lRefCount of pooled buffer is now zero.");
    }

    if ((iBufferIndex != -1) && (lResult == 1))
    {
        WaitForSingleObject(hPoolMutex, INFINITE);
        pbInUse[iBufferIndex] = false;
        ReleaseMutex(hPoolMutex);
    } else if (lResult == 0)
        delete this;

    return lResult;
}

Als iBufferIndex != -1 betekent het dat deze CReferenceBuffer instance onderdeel is van m'n memory pool.
pbInUse is een array van bools die aangeeft of een gepoolde buffer wel of niet beschikbaar is.

farlane schreef op 05 May 2003 @ 20:10:
Kan het nog zijn dat je WaitForSingleObject niet met WAIT_OBJECT_0 terugkomt maar met een WAIT_FAILED en dat je daarna je list trashed ?

WaitForSingleObject komt in alle gevallen met WAIT_OBJECT_0 terug.

curry684 schreef op 05 May 2003 @ 21:25:
Gezien het gedeelte 'af en toe' in de originele post en dat ik me nog kan herinneren waarom MrHuge deze queue maakt (de 240 audiomessages per seconde) lijkt dit me idd de meest waarschijnlijke oorzaak. Verder lijkt het me onmogelijk dat de fout in te vaak dereferencen zit daar hij wel een AddRef doet bij de Push en geen Release bij de Pop, dus het object wordt waarschijnlijker nooit gedelete dan te vaak

Het object wordt wel degelijk netjes gereleased, maar dat gebeurt in de 'consumer' thread(s).
Goed geheugen trouwens, curry . Jouw suggestie voor een SyncFIFO in combinatie met een memory pool levert me bij een configuratie met 24 simultane encoding sessies (WMA 32kbit, 32KHz, mono) een performance winst van 40% op!!!

Terug ontopic

Ondertussen ben ik aan de gang gegaan met TurboPower CodeWatch en Rational PurifyPlus (memory leak detectie proggies). Met PurifyPlus heb ik 1 memory bugje eruit gehaald ergens anders in m'n code. Dit was helaas niet de oorzaak van m'n heap corruptie. Hebben jullie toevallig nog wat tips voor memory leak detectie programmatuur?

[.oisyn]

Hebben jullie toevallig nog wat tips voor memory leak detectie programmatuur?

niet echt, ik gebruik zelf ook altijd rational purify, fantastisch tooltje! Alleen af en toe wil het niet helemaal werken zoals het hoort (meestal door nogal exotische dll's die in je project geladen zijn)

[curry684]

Wortelpudding schreef op 06 mei 2003 @ 09:37:
Goed geheugen trouwens, curry . Jouw suggestie voor een SyncFIFO in combinatie met een memory pool levert me bij een configuratie met 24 simultane encoding sessies (WMA 32kbit, 32KHz, mono) een performance winst van 40% op!!!

En da's altijd leuk om terug te horen

[farlane]

Had je al gevonden waar het aan ligt ?

[Wortelpudding]

farlane schreef op 07 mei 2003 @ 17:26:
Had je al gevonden waar het aan ligt ?

Nou, ik heb het probleem gelokaliseerd, nu nog oplossen...

Het volgende is het geval:

C++:

class CReferenceBuffer : public INSSBuffer
{
protected:
    int        iBufferIndex;
    LONG       lRefCount;
    SYSTEMTIME stTimeStamp;
    WAVEHDR    whBuffer;

    static bool*              pbInUse;
    static BYTE*              pbBuffer;
    static CReferenceBuffer** prbPool;
    static HANDLE             hPoolMutex;
    static int                iBufferCount,
                              iTotalSize,
                              iIterator;
    static int*               piBufferSizes;

    ~CReferenceBuffer();
public:
    CReferenceBuffer();

    HRESULT STDMETHODCALLTYPE GetBuffer(BYTE **ppdwBuffer);
    HRESULT STDMETHODCALLTYPE GetBufferAndLength(BYTE **ppdwBuffer, DWORD *pdwLength);
    HRESULT STDMETHODCALLTYPE GetLength(DWORD *pdwLength);
    HRESULT STDMETHODCALLTYPE GetMaxLength(DWORD *pdwLength);
    HRESULT STDMETHODCALLTYPE SetLength(DWORD dwLength);

    virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID riid, void __RPC_FAR *__RPC_FAR *ppvObject);
    virtual ULONG STDMETHODCALLTYPE AddRef(void);
    virtual ULONG STDMETHODCALLTYPE Release(void);

    static void Init(void);
    static void Cleanup(void);
    static int GetCurrentSize(void);
    static void PreAlloc(int iSize, int iCount);
    static CReferenceBuffer* GetBuffer(int iSize);

    WAVEHDR* GetWaveHdr(void)
    {
        return &whBuffer;
    }
    SYSTEMTIME* GetTimeStamp(void)
    {
        return &stTimeStamp;
    }
};

De method GetTimeStamp is hier relevant, maar ik post de rest van de classdeclaratie er maar gewoon ff bij...
Ik heb een class afgeleid van CWinThread, die MM_WIM_DATA events ontvangt van de MS Multimedia SDK.
In de event-handler van dit event doe ik onder andere het volgende:

C++:

GetLocalTime(&stNow);
pstTemp = prbStereo->GetTimeStamp();
memcpy(pstTemp, &stNow, sizeof(SYSTEMTIME));

pstTemp is hier een SYSTEMTIME*, stNow is een SYSTEMTIME en prbStereo is een CReferenceBuffer.
Deze prbStereo wordt in een synchronized fifo gepushed en vervolgens weer door een andere thread gepopt. Na deze pop wordt mbv GetTimeStamp de SYSTEMTIME* weer opgehaald.

C++:

pstTime = prbBuffer->GetTimeStamp();

Dit werkt allemaal correct, maar toch wordt de heap corrupt. Nu heb ik ontdekt dat de heap niet meer corrupt raakt als ik het gedeelte weglaat waarin ik de SYSTEMTIME structure mempcy.
Ik heb al geprobeerd om van de stTimeStamp member een SYSTEMTIME* te maken en die te new-en. Ook heb ik geprobeerd om in plaats van new VirtualAlloc te gebruiken, maar het probleem blijft bestaan.

Iemand een idee?

[.oisyn]

Wijst die SYSTEMTIME struct die je terugkrijgt met GetTimeStamp () wel naar een geldig stuk geheugen?

[Wortelpudding]

.oisyn schreef op 08 May 2003 @ 11:36:
Wijst die SYSTEMTIME struct die je terugkrijgt met GetTimeStamp () wel naar een geldig stuk geheugen?

Yep, zeker weten

[farlane]

Wortelpudding schreef op 08 May 2003 @ 11:32:
[...]
Iemand een idee?

Hmmm...

code:

memcpy(pstTemp, &stNow, sizeof(SYSTEMTIME));

Hier kun je alleen iets mee trashen als sizeof( SYSTEMTIME ) meer is dan sizeof( * pstTemp ).

Lijkt me vrij onwaarschijnlijk.
Of er kan er iets vreemds aan de hand zijn met de structure packing ?

[Wortelpudding]

Nou, uiteindelijk ben ik er dan toch uitgekomen
Het gekloot, als je last hebt van heap-corruptie, is dat de werkelijke bug echt OVERAL in je prog kan zitten. Dat bleek ook weer nu.
Ik kwam er vandaag achter dat ik in een totaaaaal ander gedeelte van m'n programma het probleem veroorzaakt heb. Ik heb daar een array van classes aangemaakt. Door deze array itereer ik en voor elk item roep ik de functie Reset() aan. Dit is op zich nooit een probleem zou je zo zeggen , maar nu wel. Deze array was (in deze configuratie) 8 items groot. Laat ik nu zo slim zijn om een verkeerde member-variabele als maximum index te gebruiken waardoor hij ineens dacht dattie 16 items groot was .

* Wortelpudding kruipt in een hoekje en zit zich diep, diep te schamen voor z'n beginnersfout ondanks 6 jaar programmeer-ervaring

[curry684]

Hehe ik zat het al stil te volgen omdat ik ook niet echt meer ideeen had maar het was natuurlijk te verwachten dat het niet in deze hoek van de code zat.

Ennuh, * curry684 heeft 18 jaar programmeerervaring en komt dit soort grappen ook nog wel eens tegen

[farlane]

Dat zijn idd leuke bugjes. Als het tegen zit kun je er erg lang naar lopen zoeken.

[.oisyn]

Overigens zou Rational Purify deze wel moeten detecteren

[curry684]

Borland CodeGuard trekt ze er over het algemeen ook wel uit.

[Wortelpudding]

Nou, ik heb 3 progs geprobeerd die de bug er allemaal niet uithaalden:

1. Rational Purify => http://www.rational.com
2. TurboPower Sleuth QA Suite 2 (CodeWatch module) => http://www.turbopower.com (zijn trouwens gestopt met de ontwikkeling van dit product).
3. Memory Validator van Software Verification Limited => http://www.softwareverify.com (erg mooi proggie trouwens)

Codeguard is geen optie omdat de app met MS Visual Studio is gemaakt.

Als iemand nog tips heeft voor betere progs, heel graag!

[.oisyn]

je kan ook gewoon betere code schrijven, zodat die tooltjes niet nodig zijn
/flauw

[farlane]

.oisyn schreef op 09 May 2003 @ 15:19:
je kan ook gewoon betere code schrijven, zodat die tooltjes niet nodig zijn
/flauw

Behoorlijk flauw wel. ( t'is vrijdag geloof ik ... )