VirtualBox

Opened 11 years ago

Closed 8 years ago

#12465 closed defect (fixed)

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

Reported by: Petr Kurtin Owned by:
Component: virtual disk Version: VirtualBox 4.3.4
Keywords: Cc:
Guest type: other Host type: other

Description

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

Change History (5)

comment:1 by Petr Vones, 11 years ago

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.

Last edited 11 years ago by Petr Vones (previous) (diff)

comment:2 by Petr Kurtin, 11 years ago

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);
Last edited 11 years ago by Petr Kurtin (previous) (diff)

comment:3 by aeichner, 11 years ago

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?

comment:4 by Petr Kurtin, 11 years ago

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

comment:5 by aeichner, 8 years ago

Resolution: fixed
Status: newclosed

Fixed long ago, closing.

Note: See TracTickets for help on using tickets.

© 2024 Oracle Support Privacy / Do Not Sell My Info Terms of Use Trademark Policy Automated Access Etiquette