locked
Potential BSOD in WFPSampler code - Stream layer (fix advised) RRS feed

  • Question

  • Hi,

    Just passing info to anyone else producing a wfpsampler based driver.

    I had two customers reporting a repeatable BSOD with our web-filter which is based on the WfpSampler, stream layer, out-of-band.  The crash showed a double free in BasicStreamInjectionCompletionDataDestroy() within CompletionFunctions_BasicStreamInjectionCallouts.cpp.  

    My initial thoughs were perhaps the NBL ref count was wrong, or perhaps WFP was calling out completion function more than it should.

    As always, the answer was far simpler than this.  Just spending a bit of time to review the function showed a potential problem:

    KeAcquireSpinLock(&(pCompletionData->spinLock),
                      &originalIRQL);
    
    pCompletionData->refCount--;
    
    ...
    
    KeReleaseSpinLock(&(pCompletionData->spinLock),
                      originalIRQL);
    
    if(pCompletionData->refCount == 0 || override)
    {
       HLPR_DELETE(*ppCompletionData,
                   WFPSAMPLER_CALLOUT_DRIVER_TAG);
    }
    

    This looks wrong! Surely it can't be safe to access the structure again after releasing the lock? What if the thread got preempted and a second thread decremented the count to 0 before the 1st thread was rescheduled?

    Not convinced I still decided to make the change anyway and see if it resolved the issue.  It did.  Both customers are unable to reproduce the crash since making this fix.

    The fix I made was:

    KeAcquireSpinLock(&(pCompletionData->spinLock),
                      &originalIRQL);
    
    UINT32 newRefCount = --pCompletionData->refCount;
    
    ...
    
    KeReleaseSpinLock(&(pCompletionData->spinLock),
                      originalIRQL);
    
    if(newRefCount == 0 || override)
    {
       HLPR_DELETE(*ppCompletionData,
                   WFPSAMPLER_CALLOUT_DRIVER_TAG);
    }

    So you may want to consider updating your version of this function, it might save you a bit of customer pain.

    Monday, March 14, 2016 1:41 PM