On 3/29/19 10:55 AM, Eric Blake wrote: >>> @@ -966,7 +985,8 @@ int coroutine_fn >>> nbd_client_co_block_status(BlockDriverState *bs, >>> .from = offset, >>> .len = MIN(MIN_NON_ZERO(QEMU_ALIGN_DOWN(INT_MAX, >>> bs->bl.request_alignment), >>> - client->info.max_block), bytes), >>> + client->info.max_block), >>> + MIN(bytes, client->info.size - offset)), > > This is a complex computation... >
>>> + if (offset >= client->info.size) { >>> + *pnum = bytes; >>> + assert(bytes < BDRV_SECTOR_SIZE); >>> + /* Intentionally don't report offset_valid for the hole */ >>> + return BDRV_BLOCK_ZERO; >>> + } >>> + >>> + if (client->info.min_block) { >>> + assert(QEMU_IS_ALIGNED(request.len, client->info.min_block)); > > ...so the assert is here to say that before we hand a length to the > server, check that our computation ended up being aligned to the > server's requirements on input. > >> >> it will crash if info.size is unaligned.. > > It will crash if our client code is buggy, before the server ever gets a > chance to see things. Our client code should not be buggy (we do not > want to be sending the server unaligned requests). Oh, I see what you are saying - if the server gives us a file length of 1023, in spite of also giving us a min_block of 512, we CANNOT access those trailing bytes - but it can still cause this code to fail the assertion. What I should do is submit an additional patch on the handshake code that if the server sends us an inconsistent length and mandates min_block, then we truncate the size to match what we can actually access while still staying compliant (at which point, client->info.size will always be aligned to client->info.min_block in the rest of the code, such as here, and this assertion is back to being safe about client-only computations that all the other inputs to the computation were also aligned). 7/6 coming up soon... -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature