Re: [ros-dev] [ros-diffs] [tthompson] 71945: [NTFS] *AddRun() - Don't leak RunBuffer when encountering errors. Handle exception from FsRtlAddLargeMcbEntry().

[Pierre Schweitzer]

Comment inline

Le 15/07/2016 à 17:27, tthompson(a)svn.reactos.org a écrit :
>   Author: tthompson
>  Date: Fri Jul 15 15:27:04 2016
>  New Revision: 71945
>  
>  URL: http://svn.reactos.org/svn/reactos?rev=71945&view=rev
>  Log:
>  [NTFS]
>  *AddRun() - Don't leak RunBuffer when encountering errors.
>  Handle exception from FsRtlAddLargeMcbEntry().
>  
>  Modified:
>      branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/attrib.c
>  
>  Modified: branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/attrib.c
>  URL:
> http://svn.reactos.org/svn/reactos/branches/GSoC_2016/NTFS/drivers/filesyst…
>  ==============================================================================
>  --- branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/attrib.c	[iso-8859-1] (original)
>  +++ branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/attrib.c	[iso-8859-1] Fri Jul 15
> 15:27:04 2016
>  @@ -88,25 +88,40 @@
>       ULONGLONG NextVBN = AttrContext->Record.NonResident.LowestVCN;
>   
>       // Allocate some memory for the RunBuffer
>  -    PUCHAR RunBuffer = ExAllocatePoolWithTag(NonPagedPool,
> Vcb->NtfsInfo.BytesPerFileRecord, TAG_NTFS);
>  +    PUCHAR RunBuffer;
>       int RunBufferOffset = 0;
>   
>       if (!AttrContext->Record.IsNonResident)
>           return STATUS_INVALID_PARAMETER;
>  +
>  +    RunBuffer = ExAllocatePoolWithTag(NonPagedPool, Vcb->NtfsInfo.BytesPerFileRecord,
> TAG_NTFS);
>   
>       // Convert the data runs to a map control block
>       Status = ConvertDataRunsToLargeMCB(DataRun, &DataRunsMCB, &NextVBN);
>       if (!NT_SUCCESS(Status))
>       {
>           DPRINT1("Unable to convert data runs to MCB (probably ran out of
> memory)!\n");
>  +        ExFreePoolWithTag(RunBuffer, TAG_NTFS);
>           return Status;
>       }
>   
>       // Add newly-assigned clusters to mcb
>  -    FsRtlAddLargeMcbEntry(&DataRunsMCB,
>  -                          NextVBN,
>  -                          NextAssignedCluster,
>  -                          RunLength);
>  +    _SEH2_TRY{
>  +        if (!FsRtlAddLargeMcbEntry(&DataRunsMCB,
>  +        NextVBN,
>  +                                   NextAssignedCluster,
>  +                                   RunLength))
>  +        {
>  +            FsRtlUninitializeLargeMcb(&DataRunsMCB);
>  +            ExFreePoolWithTag(RunBuffer, TAG_NTFS);
>  +            return STATUS_INSUFFICIENT_RESOURCES;
>  +        }
>  +    } _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER) {
>  +        FsRtlUninitializeLargeMcb(&DataRunsMCB);
>  +        ExFreePoolWithTag(RunBuffer, TAG_NTFS);
>  +        _SEH2_YIELD(return STATUS_INSUFFICIENT_RESOURCES);
>  +    } _SEH2_END; 
Purely cosmetic one here. You should, in your try block, call
ExRaiseStatus with the appropriate status and do the cleanup in the
exception handler. That avoids code duplication.
This also implies you don't want to force the status code, but rather
use _SEH2_GetExceptionCode().
Also note that FsRtlAddLargeMcbEntry may fail for various reasons. I
don't really know which status code to use as you have no way to know
why it failed.

-- 
Pierre Schweitzer <pierre at reactos.org>
System & Network Administrator
Senior Kernel Developer
ReactOS Deutschland e.V.