VHD: first sectors in newly created Data Block is written incorrectly

[Petr Kurtin]

Hello,
in VHD.cpp (vhdWrite function), when you write sector(s) into a newly created Data Block (whose default size is 2Mb), these sectors are not written at the right position and such VHD files are corrupted.

in vhdWrite function, you use:

rc = vdIfIoIntFileWriteUser(pImage->pIfIo, pImage->pStorage,
         pImage->uCurrentEndOfFile + pImage->cDataBlockBitmapSectors * VHD_SECTOR_SIZE,
         pIoCtx, cbWrite,
         vhdAsyncExpansionDataComplete,
         pExpand); 

offset parameter is wrong here, because sectors are always written at the start of the Data Block.

The right offset should be:

uint32_t cSectorInDataBlock = cSector % pImage->cSectorsPerDataBlock;
...
pImage->uCurrentEndOfFile + (pImage->cDataBlockBitmapSectors + cSectorInDataBlock) * VHD_SECTOR_SIZE

Thanks,
Petr Kurtin

[Petr Vones]

Huh. At which stage is the code executed ? Does it mean that all VHD files will be corrupted by the bug ? I have just checked them with chkdsk /f /r and found no errors.

The VHD files were originally created by Virtual PC 2007.

[Petr Kurtin]

vhdWrite was completely rewritten for async/sync IO unification (rev. 44252, 8/1/2013) and it contains this bug. When you have an empty VHD file and write a sector, it'll be stored into newly allocated 2Mb data block.

Old version of vhdWrite calculates where this sector should be stored in this 2Mb data block (i.e. cBATEntryIndex position):

/*
 * Calculate the real offset in the file.
 */
uVhdOffset = ((uint64_t)pImage->pBlockAllocationTable[cBlockAllocationTableEntry] 
                           + pImage->cDataBlockBitmapSectors 
                           + cBATEntryIndex) * VHD_SECTOR_SIZE;

/* Write data. */
vdIfIoIntFileWriteSync(pImage->pIfIo, pImage->pStorage, uVhdOffset,
                       pvBuf, cbBuf);

New version of vhdWrite writes the sector in the newly created 2Mb data block at the beginning (cBATEntryIndex is ignored):

/*
 * Write the new block at the current end of the file.
 */
 rc = vdIfIoIntFileWriteUser(pImage->pIfIo, pImage->pStorage,
                             pImage->uCurrentEndOfFile 
                               + pImage->cDataBlockBitmapSectors * VHD_SECTOR_SIZE,
                             pIoCtx, cbWrite,
                             vhdAsyncExpansionDataComplete,
                             pExpand);

Repro steps are simple:

• create an empty VHD file
• write 512 bytes to sector no. 7
• read sector no. 7 (=> zeros)
• read sector no. 0 (=> your written data)

Repro pseudocode:

uint32_t cSectorSize = 512;
uint32_t cSector = 7;
VDWrite((PVBOXHDD)pImage->pDisk, cSector * cSectorSize, data_in , cSectorSize);
VDRead ((PVBOXHDD)pImage->pDisk, cSector * cSectorSize, data_out, cSectorSize);
VDRead ((PVBOXHDD)pImage->pDisk,       0 * cSectorSize, data_out, cSectorSize);

[aeichner]

Hi,

thanks for the report. This is indeed a bug in the code but it is not triggered unless you pass VD_OPEN_FLAGS_HONOR_SAME when opening the image. If VD_OPEN_FLAGS_HONOR_SAME is not given the generic storage layer will only do full block writes to previously unallocated blocks, so the bug is not triggered in most cases. Do you have a complete code example?

[Petr Kurtin]

You're right, I don't use full blocking writes (my disk container is based on VHD).

[aeichner]

Fixed long ago, closing.