On Wed, Sep 25, 2013 at 05:02:53PM -0400, Jeff Cody wrote:
> +static int vhdx_log_read_desc(BlockDriverState *bs, BDRVVHDXState *s,
> + VHDXLogEntries *log, VHDXLogDescEntries
> **buffer)
> +{
> + int ret = 0;
> + uint32_t desc_sectors;
> + uint32_t sectors_read;
> + VHDXLogEntryHeader hdr;
> + VHDXLogDescEntries *desc_entries = NULL;
> + int i;
> +
> + assert(*buffer == NULL);
> +
> + ret = vhdx_log_peek_hdr(bs, log, &hdr);
> + if (ret < 0) {
> + goto exit;
> + }
> + vhdx_log_entry_hdr_le_import(&hdr);
> + if (vhdx_log_hdr_is_valid(log, &hdr, s) == false) {
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> + desc_sectors = vhdx_compute_desc_sectors(hdr.descriptor_count);
I don't see input validation for hdr.descriptor_count. It not exceed
log length.
> +static int vhdx_log_flush_desc(BlockDriverState *bs, VHDXLogDescriptor *desc,
> + VHDXLogDataSector *data)
> +{
> + int ret = 0;
> + uint64_t seq, file_offset;
> + uint32_t offset = 0;
> + void *buffer = NULL;
> + uint64_t count = 1;
> + int i;
> +
> + buffer = qemu_blockalign(bs, VHDX_LOG_SECTOR_SIZE);
> +
> + if (!memcmp(&desc->signature, "desc", 4)) {
> + /* data sector */
> + if (data == NULL) {
> + ret = -EFAULT;
> + goto exit;
> + }
> +
> + /* The sequence number of the data sector must match that
> + * in the descriptor */
> + seq = data->sequence_high;
> + seq <<= 32;
> + seq |= data->sequence_low & 0xffffffff;
> +
> + if (seq != desc->sequence_number) {
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> + /* Each data sector is in total 4096 bytes, however the first
> + * 8 bytes, and last 4 bytes, are located in the descriptor */
> + memcpy(buffer, &desc->leading_bytes, 8);
> + offset += 8;
> +
> + memcpy(buffer+offset, data->data, 4084);
> + offset += 4084;
> +
> + memcpy(buffer+offset, &desc->trailing_bytes, 4);
> +
> + } else if (!memcmp(&desc->signature, "zero", 4)) {
> + /* write 'count' sectors of sector */
> + memset(buffer, 0, VHDX_LOG_SECTOR_SIZE);
> + count = desc->zero_length / VHDX_LOG_SECTOR_SIZE;
Zero descriptors also have a sequence number that should be checked.
> +/* Parse the replay log. Per the VHDX spec, if the log is present
> + * it must be replayed prior to opening the file, even read-only.
> + *
> + * If read-only, we must replay the log in RAM (or refuse to open
> + * a dirty VHDX file read-only */
read-only) <-- missing parenthesis
> @@ -794,10 +753,12 @@ static int vhdx_open(BlockDriverState *bs, QDict
> *options, int flags,
> uint32_t i;
> uint64_t signature;
> uint32_t data_blocks_cnt, bitmap_blocks_cnt;
> + bool log_flushed = false;
>
>
> s->bat = NULL;
> s->first_visible_write = true;
> + s->log.write = s->log.read = 0;
This is not really necessary since bs->opaque is zeroed before block.c
calls vhdx_open().
>
> qemu_co_mutex_init(&s->lock);
>
> @@ -821,20 +782,33 @@ static int vhdx_open(BlockDriverState *bs, QDict
> *options, int flags,
> goto fail;
> }
>
> - ret = vhdx_parse_log(bs, s);
> + ret = vhdx_open_region_tables(bs, s);
> if (ret) {
> goto fail;
> }
>
> - ret = vhdx_open_region_tables(bs, s);
> + ret = vhdx_parse_metadata(bs, s);
> if (ret) {
> goto fail;
> }
>
> - ret = vhdx_parse_metadata(bs, s);
> + ret = vhdx_parse_log(bs, s, &log_flushed);
> if (ret) {
> goto fail;
> }
Why replay the log *after* reading logged metadata? We could read stale
or corrupted values.
I guess there is a dependency here - the log replay code needs some
field that vhdx_open_region_tables() or vhdx_parse_metadata() load?
