On 03/30/2018 11:59 AM, Vladimir Sementsov-Ogievskiy wrote: > 30.03.2018 02:18, Eric Blake wrote: >> It's never a good idea to blindly read for size bytes as >> returned by the server without first validating that the size >> is within bounds; a malicious or buggy server could cause us >> to hang or get out of sync from reading further messages. >> >> It may be smarter to try and teach the client to cope with >> unexpected context ids by silently ignoring them instead of >> hanging up on the server, but for now, if the server doesn't >> reply with exactly the one context we expect, it's easier to >> just give up - however, if we give up for any reason other >> than an I/O failure, we might as well try to politely tell >> the server we are quitting rather than continuing.
>> @@ -651,6 +651,14 @@ static int >> nbd_negotiate_simple_meta_context(QIOChannel *ioc, >> char *name; >> size_t len; >> >> + if (reply.length != sizeof(received_id) + context_len) { >> + error_setg(errp, "Failed to negotiate meta context '%s', >> server " >> + "answered with unexpected length %u", context, > > uint32_t, is it worth PRIu32 ? Or %u is absolutely portable in this case? For trace-events, casting uint32_t to unsigned int is always safe, at which point using %u is less typing (because the trace goes through a function prototype conversion). But when directly printing a uint32_t, you are correct that some oddball 32-bit platforms might have uint32_t be long, which would then trigger needless warnings if we don't use PRIu32. So I'll fix that. > >> + reply.length); >> + nbd_send_opt_abort(ioc); >> + return -1; >> + } > > hmm, after this check, len variable is not actually needed, we can use > context_len > Okay, I'm squashing this in: diff --git i/nbd/client.c w/nbd/client.c index 4ee1d9a4a2c..dd0174b036e 100644 --- i/nbd/client.c +++ w/nbd/client.c @@ -649,11 +649,10 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, if (reply.type == NBD_REP_META_CONTEXT) { char *name; - size_t len; if (reply.length != sizeof(received_id) + context_len) { error_setg(errp, "Failed to negotiate meta context '%s', server " - "answered with unexpected length %u", context, + "answered with unexpected length %" PRIu32, context, reply.length); nbd_send_opt_abort(ioc); return -1; @@ -664,13 +663,13 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, } be32_to_cpus(&received_id); - len = reply.length - sizeof(received_id); - name = g_malloc(len + 1); - if (nbd_read(ioc, name, len, errp) < 0) { + reply.length -= sizeof(received_id); + name = g_malloc(reply.length + 1); + if (nbd_read(ioc, name, reply.length, errp) < 0) { g_free(name); return -1; } - name[len] = '\0'; + name[reply.length] = '\0'; if (strcmp(context, name)) { error_setg(errp, "Failed to negotiate meta context '%s', server " "answered with different context '%s'", context, >> @@ -690,6 +699,12 @@ static int >> nbd_negotiate_simple_meta_context(QIOChannel *ioc, >> if (reply.type != NBD_REP_ACK) { >> error_setg(errp, "Unexpected reply type %" PRIx32 " expected >> %x", >> reply.type, NBD_REP_ACK); >> + nbd_send_opt_abort(ioc); >> + return -1; >> + } >> + if (reply.length) { > > this check is very common for REP_ACK, it may be better to move it to > nbd_handle_reply_err... (and rename this function? and combine it > somehow with _option_request() and _option_reply()?) > >> + error_setg(errp, "Unexpected length to ACK response"); >> + nbd_send_opt_abort(ioc); > > hmm, looks like we want nbd_send_opt_abort() before most of return -1. > Looks like it lacks some generalization, may be want to send it at some > common point.. > >> return -1; >> } >> > > mostly, just ideas for future refactoring, so: Indeed, any refactoring we do in that area belongs in 2.13 patches. > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> Thanks; I'm including this in my NBD pull request today. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature