On 23.08.2022 11:47, Vladimir Sementsov-Ogievskiy wrote:
On 8/23/22 12:34, Denis V. Lunev wrote:
On 23.08.2022 08:58, Vladimir Sementsov-Ogievskiy wrote:
On 8/22/22 12:05, Alexander Ivanov wrote:
data_end field in BDRVParallelsState is set to the biggest offset
present
in BAT. If this offset is outside of the image, any further write
will create
the cluster at this offset and/or the image will be truncated to this
offset on close. This is definitely not correct.
Raise an error in parallels_open() if data_end points outside the
image and
it is not a check (let the check to repaire the image).
Signed-off-by: Alexander Ivanov <[email protected]>
---
block/parallels.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index a229c06f25..c245ca35cd 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs,
QDict *options, int flags,
BDRVParallelsState *s = bs->opaque;
ParallelsHeader ph;
int ret, size, i;
+ int64_t file_size;
QemuOpts *opts = NULL;
Error *local_err = NULL;
char *buf;
@@ -811,6 +812,19 @@ static int parallels_open(BlockDriverState
*bs, QDict *options, int flags,
}
}
+ file_size = bdrv_getlength(bs->file->bs);
+ if (file_size < 0) {
+ ret = file_size;
+ goto fail;
+ }
+
+ file_size >>= BDRV_SECTOR_BITS;
+ if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
+ error_setg(errp, "parallels: Offset in BAT is out of image");
+ ret = -EINVAL;
+ goto fail;
+ }
If image is unaligned to sector size, and image size is less than
s->data_end, but the difference itself is less than sector, the
error message would be misleading.
Should we consider "file_size = DIV_ROUND_UP(file_size,
BDRV_SECTOR_SIZE)" instead of "file_size >>= BDRV_SECTOR_BITS"?
That is a different check. We need to be sure that file_size is
authentic,
which worth additional check.
At my opinion, this worth additional patch later on. Let us agree here,
queue and proceed with further pending fixes.
We should use something like the following
data_start = le32_to_cpu(s->header->data_off);
if (data_start == 0) {
data_start = ROUND_UP(bat_entry_off(s->bat_size),
BDRV_SECTOR_SIZE);
}
if ((file_size - data_start) % cluster_size != 0) {
error();
}
Do you think that we should error-out in such conditions? It doesn't
break the spec, is it? Can the last cluster be half allocated?
absolutely no!
We should adjust the spec on this.
Den
