On 8 Apr 2016, at 23:05, Eric Blake <ebl...@redhat.com> wrote:
> Since we know that the maximum name we are willing to accept
> is small enough to stack-allocate, rework the iteration over
> NBD_OPT_LIST responses to reuse a stack buffer rather than
> allocating every time. Furthermore, we don't even have to
> allocate if we know the server's length doesn't match what
> we are searching for.
>
> Not fixed here: Upstream NBD Protocol recently added this
> clarification:
> https://github.com/yoe/nbd/blob/18918eb/doc/proto.md#conventions
>
> Where this document refers to a string, then unless otherwise
> stated, that string is a sequence of UTF-8 code points, which
> is not NUL terminated, MUST NOT contain NUL characters, SHOULD
> be no longer than 256 bytes and MUST be no longer than 4096
> bytes. This applies to export names and error messages (amongst
> others).
>
> To be fully compliant to that, we need to bump our export name
> limit from 255 to at least 256, and need to decide whether we
> can bump it higher (bumping it all the way to 4096 is annoying
> in that we could no longer safely stack-allocate a worst-case
> string, so we may still want to take the leeway offered by SHOULD
> to force a reasonable smaller limit).
Is there a limit in qemu-world to safe stack allocation? I thought
that was (in general) only a kernel consideration? (probably my
ignorance here).
Otherwise:
Reviewed-by: Alex Bligh <a...@alex.org.uk>
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> nbd/client.c | 130 +++++++++++++++++++++++++++++------------------------------
> 1 file changed, 65 insertions(+), 65 deletions(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index b2dfc11..d4e37d5 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -230,14 +230,17 @@ static int nbd_handle_reply_err(QIOChannel *ioc,
> nbd_opt_reply *reply,
> return result;
> }
>
> -static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp)
> +/* Return -1 if unrecoverable error occurs, 0 if NBD_OPT_LIST is
> + * unsupported, 1 if iteration is done, 2 to keep looking, and 3 if
> + * this entry matches want. */
> +static int nbd_receive_list(QIOChannel *ioc, const char *want, Error **errp)
> {
> nbd_opt_reply reply;
> uint32_t len;
> uint32_t namelen;
> + char name[NBD_MAX_NAME_SIZE + 1];
> int error;
>
> - *name = NULL;
> if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, &reply, errp) < 0) {
> return -1;
> }
> @@ -252,97 +255,94 @@ static int nbd_receive_list(QIOChannel *ioc, char
> **name, Error **errp)
> error_setg(errp, "length too long for option end");
> return -1;
> }
> - } else if (reply.type == NBD_REP_SERVER) {
> - if (len < sizeof(namelen) || len > NBD_MAX_BUFFER_SIZE) {
> - error_setg(errp, "incorrect option length %"PRIu32, len);
> - return -1;
> - }
> - if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
> - error_setg(errp, "failed to read option name length");
> - return -1;
> - }
> - namelen = be32_to_cpu(namelen);
> - len -= sizeof(namelen);
> - if (len < namelen) {
> - error_setg(errp, "incorrect option name length");
> - return -1;
> - }
> - if (namelen > NBD_MAX_NAME_SIZE) {
> - error_setg(errp, "export name length too long %" PRIu32,
> namelen);
> - return -1;
> - }
> -
> - *name = g_new0(char, namelen + 1);
> - if (read_sync(ioc, *name, namelen) != namelen) {
> - error_setg(errp, "failed to read export name");
> - g_free(*name);
> - *name = NULL;
> - return -1;
> - }
> - (*name)[namelen] = '\0';
> - len -= namelen;
> - if (drop_sync(ioc, len) != len) {
> - error_setg(errp, "failed to read export description");
> - g_free(*name);
> - *name = NULL;
> - return -1;
> - }
> - } else {
> + return 1;
> + } else if (reply.type != NBD_REP_SERVER) {
> error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
> reply.type, NBD_REP_SERVER);
> return -1;
> }
> - return 1;
> +
> + if (len < sizeof(namelen) || len > NBD_MAX_BUFFER_SIZE) {
> + error_setg(errp, "incorrect option length %"PRIu32, len);
> + return -1;
> + }
> + if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) {
> + error_setg(errp, "failed to read option name length");
> + return -1;
> + }
> + namelen = be32_to_cpu(namelen);
> + len -= sizeof(namelen);
> + if (len < namelen) {
> + error_setg(errp, "incorrect option name length");
> + return -1;
> + }
> + if (namelen != strlen(want)) {
> + if (drop_sync(ioc, len) != len) {
> + error_setg(errp, "failed to skip export name with wrong length");
> + return -1;
> + }
> + return 2;
> + }
> +
> + assert(namelen < sizeof(name));
> + if (read_sync(ioc, name, namelen) != namelen) {
> + error_setg(errp, "failed to read export name");
> + return -1;
> + }
> + name[namelen] = '\0';
> + len -= namelen;
> + if (drop_sync(ioc, len) != len) {
> + error_setg(errp, "failed to read export description");
> + return -1;
> + }
> + return strcmp(name, want) == 0 ? 3 : 2;
> }
>
>
> +/* Return -1 on failure, 0 if wantname is an available export. */
> static int nbd_receive_query_exports(QIOChannel *ioc,
> const char *wantname,
> Error **errp)
> {
> bool foundExport = false;
>
> - TRACE("Querying export list");
> + TRACE("Querying export list for '%s'", wantname);
> if (nbd_send_option_request(ioc, NBD_OPT_LIST, 0, NULL, errp) < 0) {
> return -1;
> }
>
> TRACE("Reading available export names");
> while (1) {
> - char *name = NULL;
> - int ret = nbd_receive_list(ioc, &name, errp);
> + int ret = nbd_receive_list(ioc, wantname, errp);
>
> - if (ret < 0) {
> - g_free(name);
> - name = NULL;
> + switch (ret) {
> + default:
> + /* Server gave unexpected reply */
> + assert(ret < 0);
> return -1;
> - }
> - if (ret == 0) {
> + case 0:
> /* Server doesn't support export listing, so
> * we will just assume an export with our
> * wanted name exists */
> - foundExport = true;
> - break;
> - }
> - if (name == NULL) {
> - TRACE("End of export name list");
> + return 0;
> + case 1:
> + /* Done iterating. */
> + if (!foundExport) {
> + error_setg(errp, "No export with name '%s' available",
> + wantname);
> + return -1;
> + }
> + return 0;
> + case 2:
> + /* Wasn't this one, keep going. */
> break;
> - }
> - if (g_str_equal(name, wantname)) {
> + case 3:
> + /* Found a match, but must finish parsing reply. */
> + TRACE("Found desired export name '%s'", wantname);
> foundExport = true;
> - TRACE("Found desired export name '%s'", name);
> - } else {
> - TRACE("Ignored export name '%s'", name);
> + break;
> }
> - g_free(name);
> - }
> -
> - if (!foundExport) {
> - error_setg(errp, "No export with name '%s' available", wantname);
> - return -1;
> }
> -
> - return 0;
> }
>
> static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
> --
> 2.5.5
>
>
--
Alex Bligh
