* [Qemu-devel] [PATCH for-2.12] nbd/client: Correctly handle bad server REP_META_CONTEXT
@ 2018-03-29 23:18 Eric Blake
2018-03-30 16:59 ` Vladimir Sementsov-Ogievskiy
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Eric Blake @ 2018-03-29 23:18 UTC (permalink / raw)
To: qemu-devel; +Cc: vsementsov, Paolo Bonzini, open list:Network Block Dev...
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.
Fix some typos in the process.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
nbd/client.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index 9b9b7f0ea29..4ee1d9a4a2c 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -599,8 +599,8 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
* Set one meta context. Simple means that reply must contain zero (not
* negotiated) or one (negotiated) contexts. More contexts would be considered
* as a protocol error. It's also implied that meta-data query equals queried
- * context name, so, if server replies with something different then @context,
- * it considered as error too.
+ * context name, so, if server replies with something different than @context,
+ * it is considered an error too.
* return 1 for successful negotiation, context_id is set
* 0 if operation is unsupported,
* -1 with errp set for any other error
@@ -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,
+ reply.length);
+ nbd_send_opt_abort(ioc);
+ return -1;
+ }
+
if (nbd_read(ioc, &received_id, sizeof(received_id), errp) < 0) {
return -1;
}
@@ -668,6 +676,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc,
"answered with different context '%s'", context,
name);
g_free(name);
+ nbd_send_opt_abort(ioc);
return -1;
}
g_free(name);
@@ -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) {
+ error_setg(errp, "Unexpected length to ACK response");
+ nbd_send_opt_abort(ioc);
return -1;
}
--
2.14.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12] nbd/client: Correctly handle bad server REP_META_CONTEXT
2018-03-29 23:18 [Qemu-devel] [PATCH for-2.12] nbd/client: Correctly handle bad server REP_META_CONTEXT Eric Blake
@ 2018-03-30 16:59 ` Vladimir Sementsov-Ogievskiy
2018-04-02 13:59 ` Eric Blake
2018-03-31 7:44 ` no-reply
2018-03-31 9:05 ` no-reply
2 siblings, 1 reply; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-03-30 16:59 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: Paolo Bonzini, open list:Network Block Dev...
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.
>
> Fix some typos in the process.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> nbd/client.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/nbd/client.c b/nbd/client.c
> index 9b9b7f0ea29..4ee1d9a4a2c 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -599,8 +599,8 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
> * Set one meta context. Simple means that reply must contain zero (not
> * negotiated) or one (negotiated) contexts. More contexts would be considered
> * as a protocol error. It's also implied that meta-data query equals queried
> - * context name, so, if server replies with something different then @context,
> - * it considered as error too.
> + * context name, so, if server replies with something different than @context,
> + * it is considered an error too.
> * return 1 for successful negotiation, context_id is set
> * 0 if operation is unsupported,
> * -1 with errp set for any other error
> @@ -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?
> + reply.length);
> + nbd_send_opt_abort(ioc);
> + return -1;
> + }
hmm, after this check, len variable is not actually needed, we can use
context_len
> +
> if (nbd_read(ioc, &received_id, sizeof(received_id), errp) < 0) {
> return -1;
> }
> @@ -668,6 +676,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc,
> "answered with different context '%s'", context,
> name);
> g_free(name);
> + nbd_send_opt_abort(ioc);
> return -1;
> }
> g_free(name);
> @@ -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:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12] nbd/client: Correctly handle bad server REP_META_CONTEXT
2018-03-29 23:18 [Qemu-devel] [PATCH for-2.12] nbd/client: Correctly handle bad server REP_META_CONTEXT Eric Blake
2018-03-30 16:59 ` Vladimir Sementsov-Ogievskiy
@ 2018-03-31 7:44 ` no-reply
2018-03-31 9:05 ` no-reply
2 siblings, 0 replies; 5+ messages in thread
From: no-reply @ 2018-03-31 7:44 UTC (permalink / raw)
To: eblake; +Cc: famz, qemu-devel, pbonzini, vsementsov, qemu-block
Hi,
This series failed docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
Type: series
Message-id: 20180329231837.1914680-1-eblake@redhat.com
Subject: [Qemu-devel] [PATCH for-2.12] nbd/client: Correctly handle bad server REP_META_CONTEXT
=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
time make docker-test-mingw@fedora
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
3209a590d9 nbd/client: Correctly handle bad server REP_META_CONTEXT
=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into '/var/tmp/patchew-tester-tmp-zma0j9sd/src/dtc'...
Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42'
BUILD fedora
make[1]: Entering directory '/var/tmp/patchew-tester-tmp-zma0j9sd/src'
GEN /var/tmp/patchew-tester-tmp-zma0j9sd/src/docker-src.2018-03-31-03.43.29.27947/qemu.tar
Cloning into '/var/tmp/patchew-tester-tmp-zma0j9sd/src/docker-src.2018-03-31-03.43.29.27947/qemu.tar.vroot'...
done.
Checking out files: 41% (2534/6066)
Checking out files: 42% (2548/6066)
Checking out files: 43% (2609/6066)
Checking out files: 44% (2670/6066)
Checking out files: 44% (2692/6066)
Checking out files: 45% (2730/6066)
Checking out files: 46% (2791/6066)
Checking out files: 46% (2851/6066)
Checking out files: 47% (2852/6066)
Checking out files: 48% (2912/6066)
Checking out files: 49% (2973/6066)
Checking out files: 50% (3033/6066)
Checking out files: 51% (3094/6066)
Checking out files: 52% (3155/6066)
Checking out files: 53% (3215/6066)
Checking out files: 54% (3276/6066)
Checking out files: 55% (3337/6066)
Checking out files: 55% (3380/6066)
Checking out files: 56% (3397/6066)
Checking out files: 57% (3458/6066)
Checking out files: 58% (3519/6066)
Checking out files: 59% (3579/6066)
Checking out files: 60% (3640/6066)
Checking out files: 61% (3701/6066)
Checking out files: 62% (3761/6066)
Checking out files: 63% (3822/6066)
Checking out files: 64% (3883/6066)
Checking out files: 65% (3943/6066)
Checking out files: 66% (4004/6066)
Checking out files: 67% (4065/6066)
Checking out files: 68% (4125/6066)
Checking out files: 69% (4186/6066)
Checking out files: 70% (4247/6066)
Checking out files: 71% (4307/6066)
Checking out files: 72% (4368/6066)
Checking out files: 73% (4429/6066)
Checking out files: 74% (4489/6066)
Checking out files: 75% (4550/6066)
Checking out files: 76% (4611/6066)
Checking out files: 77% (4671/6066)
Checking out files: 78% (4732/6066)
Checking out files: 79% (4793/6066)
Checking out files: 80% (4853/6066)
Checking out files: 81% (4914/6066)
Checking out files: 82% (4975/6066)
Checking out files: 83% (5035/6066)
Checking out files: 84% (5096/6066)
Checking out files: 85% (5157/6066)
Checking out files: 86% (5217/6066)
Checking out files: 87% (5278/6066)
Checking out files: 88% (5339/6066)
Checking out files: 89% (5399/6066)
Checking out files: 90% (5460/6066)
Checking out files: 91% (5521/6066)
Checking out files: 92% (5581/6066)
Checking out files: 93% (5642/6066)
Checking out files: 94% (5703/6066)
Checking out files: 95% (5763/6066)
Checking out files: 95% (5798/6066)
Checking out files: 96% (5824/6066)
Checking out files: 97% (5885/6066)
Checking out files: 98% (5945/6066)
Checking out files: 99% (6006/6066)
Checking out files: 100% (6066/6066)
Checking out files: 100% (6066/6066), done.
Your branch is up-to-date with 'origin/test'.
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into '/var/tmp/patchew-tester-tmp-zma0j9sd/src/docker-src.2018-03-31-03.43.29.27947/qemu.tar.vroot/dtc'...
Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42'
Submodule 'ui/keycodemapdb' (git://git.qemu.org/keycodemapdb.git) registered for path 'ui/keycodemapdb'
Cloning into '/var/tmp/patchew-tester-tmp-zma0j9sd/src/docker-src.2018-03-31-03.43.29.27947/qemu.tar.vroot/ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce'
tar: /var/tmp/patchew-tester-tmp-zma0j9sd/src/docker-src.2018-03-31-03.43.29.27947/qemu.tar: Wrote only 2048 of 10240 bytes
tar: Error is not recoverable: exiting now
failed to create tar file
COPY RUNNER
RUN test-mingw in qemu:fedora
tar: Unexpected EOF in archive
tar: Unexpected EOF in archive
tar: Error is not recoverable: exiting now
/var/tmp/qemu/run: line 32: prep_fail: command not found
Packages installed:
PyYAML-3.12-5.fc27.x86_64
SDL-devel-1.2.15-29.fc27.x86_64
bc-1.07.1-3.fc27.x86_64
bison-3.0.4-8.fc27.x86_64
bzip2-1.0.6-24.fc27.x86_64
ccache-3.3.6-1.fc27.x86_64
clang-5.0.1-3.fc27.x86_64
findutils-4.6.0-16.fc27.x86_64
flex-2.6.1-5.fc27.x86_64
gcc-7.3.1-5.fc27.x86_64
gcc-c++-7.3.1-5.fc27.x86_64
gettext-0.19.8.1-12.fc27.x86_64
git-2.14.3-3.fc27.x86_64
glib2-devel-2.54.3-2.fc27.x86_64
hostname-3.18-4.fc27.x86_64
libaio-devel-0.3.110-9.fc27.x86_64
libasan-7.3.1-5.fc27.x86_64
libfdt-devel-1.4.6-1.fc27.x86_64
libubsan-7.3.1-5.fc27.x86_64
llvm-5.0.1-3.fc27.x86_64
make-4.2.1-4.fc27.x86_64
mingw32-SDL-1.2.15-9.fc27.noarch
mingw32-bzip2-1.0.6-9.fc27.noarch
mingw32-curl-7.54.1-2.fc27.noarch
mingw32-glib2-2.54.1-1.fc27.noarch
mingw32-gmp-6.1.2-2.fc27.noarch
mingw32-gnutls-3.5.13-2.fc27.noarch
mingw32-gtk2-2.24.31-4.fc27.noarch
mingw32-gtk3-3.22.16-1.fc27.noarch
mingw32-libjpeg-turbo-1.5.1-3.fc27.noarch
mingw32-libpng-1.6.29-2.fc27.noarch
mingw32-libssh2-1.8.0-3.fc27.noarch
mingw32-libtasn1-4.13-1.fc27.noarch
mingw32-nettle-3.3-3.fc27.noarch
mingw32-pixman-0.34.0-3.fc27.noarch
mingw32-pkg-config-0.28-9.fc27.x86_64
mingw64-SDL-1.2.15-9.fc27.noarch
mingw64-bzip2-1.0.6-9.fc27.noarch
mingw64-curl-7.54.1-2.fc27.noarch
mingw64-glib2-2.54.1-1.fc27.noarch
mingw64-gmp-6.1.2-2.fc27.noarch
mingw64-gnutls-3.5.13-2.fc27.noarch
mingw64-gtk2-2.24.31-4.fc27.noarch
mingw64-gtk3-3.22.16-1.fc27.noarch
mingw64-libjpeg-turbo-1.5.1-3.fc27.noarch
mingw64-libpng-1.6.29-2.fc27.noarch
mingw64-libssh2-1.8.0-3.fc27.noarch
mingw64-libtasn1-4.13-1.fc27.noarch
mingw64-nettle-3.3-3.fc27.noarch
mingw64-pixman-0.34.0-3.fc27.noarch
mingw64-pkg-config-0.28-9.fc27.x86_64
nettle-devel-3.4-1.fc27.x86_64
perl-5.26.1-403.fc27.x86_64
pixman-devel-0.34.0-4.fc27.x86_64
python3-3.6.2-13.fc27.x86_64
sparse-0.5.1-2.fc27.x86_64
tar-1.29-7.fc27.x86_64
which-2.21-4.fc27.x86_64
zlib-devel-1.2.11-4.fc27.x86_64
Environment variables:
TARGET_LIST=
PACKAGES=ccache gettext git tar PyYAML sparse flex bison python3 bzip2 hostname glib2-devel pixman-devel zlib-devel SDL-devel libfdt-devel gcc gcc-c++ llvm clang make perl which bc findutils libaio-devel nettle-devel libasan libubsan mingw32-pixman mingw32-glib2 mingw32-gmp mingw32-SDL mingw32-pkg-config mingw32-gtk2 mingw32-gtk3 mingw32-gnutls mingw32-nettle mingw32-libtasn1 mingw32-libjpeg-turbo mingw32-libpng mingw32-curl mingw32-libssh2 mingw32-bzip2 mingw64-pixman mingw64-glib2 mingw64-gmp mingw64-SDL mingw64-pkg-config mingw64-gtk2 mingw64-gtk3 mingw64-gnutls mingw64-nettle mingw64-libtasn1 mingw64-libjpeg-turbo mingw64-libpng mingw64-curl mingw64-libssh2 mingw64-bzip2
J=8
V=
HOSTNAME=fd0019922340
DEBUG=
SHOW_ENV=1
PWD=/
HOME=/root
CCACHE_DIR=/var/tmp/ccache
DISTTAG=f27container
QEMU_CONFIGURE_OPTS=--python=/usr/bin/python3
FGC=f27
TEST_DIR=/tmp/qemu-test
SHLVL=1
FEATURES=mingw clang pyyaml asan dtc
PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
MAKEFLAGS= -j8
EXTRA_CONFIGURE_OPTS=
_=/usr/bin/env
/var/tmp/qemu/run: line 52: cd: /tmp/qemu-test/src/tests/docker: No such file or directory
/var/tmp/qemu/run: line 57: /test-mingw: No such file or directory
Traceback (most recent call last):
File "./tests/docker/docker.py", line 407, in <module>
sys.exit(main())
File "./tests/docker/docker.py", line 404, in main
return args.cmdobj.run(args, argv)
File "./tests/docker/docker.py", line 261, in run
return Docker().run(argv, args.keep, quiet=args.quiet)
File "./tests/docker/docker.py", line 229, in run
quiet=quiet)
File "./tests/docker/docker.py", line 147, in _do_check
return subprocess.check_call(self._command + cmd, **kwargs)
File "/usr/lib64/python2.7/subprocess.py", line 186, in check_call
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['docker', 'run', '--label', 'com.qemu.instance.uuid=40423eaa34b711e883c952540069c830', '-u', '0', '--security-opt', 'seccomp=unconfined', '--rm', '--net=none', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=8', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/root/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-zma0j9sd/src/docker-src.2018-03-31-03.43.29.27947:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 127
make[1]: *** [tests/docker/Makefile.include:129: docker-run] Error 1
make[1]: Leaving directory '/var/tmp/patchew-tester-tmp-zma0j9sd/src'
make: *** [tests/docker/Makefile.include:163: docker-run-test-mingw@fedora] Error 2
real 0m35.175s
user 0m9.715s
sys 0m6.920s
=== OUTPUT END ===
Test command exited with code: 2
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12] nbd/client: Correctly handle bad server REP_META_CONTEXT
2018-03-29 23:18 [Qemu-devel] [PATCH for-2.12] nbd/client: Correctly handle bad server REP_META_CONTEXT Eric Blake
2018-03-30 16:59 ` Vladimir Sementsov-Ogievskiy
2018-03-31 7:44 ` no-reply
@ 2018-03-31 9:05 ` no-reply
2 siblings, 0 replies; 5+ messages in thread
From: no-reply @ 2018-03-31 9:05 UTC (permalink / raw)
To: eblake; +Cc: famz, qemu-devel, pbonzini, vsementsov, qemu-block
Hi,
This series failed docker-build@min-glib build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
Type: series
Message-id: 20180329231837.1914680-1-eblake@redhat.com
Subject: [Qemu-devel] [PATCH for-2.12] nbd/client: Correctly handle bad server REP_META_CONTEXT
=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
time make docker-test-build@min-glib
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
3209a590d9 nbd/client: Correctly handle bad server REP_META_CONTEXT
=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into '/var/tmp/patchew-tester-tmp-un0cnd3r/src/dtc'...
Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42'
BUILD min-glib
make[1]: Entering directory '/var/tmp/patchew-tester-tmp-un0cnd3r/src'
GEN /var/tmp/patchew-tester-tmp-un0cnd3r/src/docker-src.2018-03-31-05.05.27.3469/qemu.tar
Cloning into '/var/tmp/patchew-tester-tmp-un0cnd3r/src/docker-src.2018-03-31-05.05.27.3469/qemu.tar.vroot'...
done.
Checking out files: 37% (2247/6066)
Checking out files: 38% (2306/6066)
Checking out files: 39% (2366/6066)
Checking out files: 40% (2427/6066)
Checking out files: 41% (2488/6066)
Checking out files: 42% (2548/6066)
Checking out files: 43% (2609/6066)
Checking out files: 44% (2670/6066)
Checking out files: 45% (2730/6066)
Checking out files: 46% (2791/6066)
Checking out files: 47% (2852/6066)
Checking out files: 48% (2912/6066)
Checking out files: 49% (2973/6066)
Checking out files: 50% (3033/6066)
Checking out files: 50% (3074/6066)
Checking out files: 51% (3094/6066)
Checking out files: 52% (3155/6066)
Checking out files: 53% (3215/6066)
Checking out files: 54% (3276/6066)
Checking out files: 55% (3337/6066)
Checking out files: 56% (3397/6066)
Checking out files: 57% (3458/6066)
Checking out files: 58% (3519/6066)
Checking out files: 59% (3579/6066)
Checking out files: 60% (3640/6066)
Checking out files: 61% (3701/6066)
Checking out files: 62% (3761/6066)
Checking out files: 63% (3822/6066)
Checking out files: 64% (3883/6066)
Checking out files: 65% (3943/6066)
Checking out files: 66% (4004/6066)
Checking out files: 67% (4065/6066)
Checking out files: 68% (4125/6066)
Checking out files: 69% (4186/6066)
Checking out files: 70% (4247/6066)
Checking out files: 71% (4307/6066)
Checking out files: 72% (4368/6066)
Checking out files: 73% (4429/6066)
Checking out files: 74% (4489/6066)
Checking out files: 75% (4550/6066)
Checking out files: 75% (4554/6066)
Checking out files: 76% (4611/6066)
Checking out files: 77% (4671/6066)
Checking out files: 78% (4732/6066)
Checking out files: 79% (4793/6066)
Checking out files: 80% (4853/6066)
Checking out files: 81% (4914/6066)
Checking out files: 82% (4975/6066)
Checking out files: 83% (5035/6066)
Checking out files: 84% (5096/6066)
Checking out files: 85% (5157/6066)
Checking out files: 86% (5217/6066)
Checking out files: 87% (5278/6066)
Checking out files: 88% (5339/6066)
Checking out files: 89% (5399/6066)
Checking out files: 90% (5460/6066)
Checking out files: 91% (5521/6066)
Checking out files: 92% (5581/6066)
Checking out files: 93% (5642/6066)
Checking out files: 94% (5703/6066)
Checking out files: 95% (5763/6066)
Checking out files: 96% (5824/6066)
Checking out files: 97% (5885/6066)
Checking out files: 97% (5939/6066)
Checking out files: 98% (5945/6066)
Checking out files: 99% (6006/6066)
Checking out files: 100% (6066/6066)
Checking out files: 100% (6066/6066), done.
Your branch is up-to-date with 'origin/test'.
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into '/var/tmp/patchew-tester-tmp-un0cnd3r/src/docker-src.2018-03-31-05.05.27.3469/qemu.tar.vroot/dtc'...
Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42'
Submodule 'ui/keycodemapdb' (git://git.qemu.org/keycodemapdb.git) registered for path 'ui/keycodemapdb'
Cloning into '/var/tmp/patchew-tester-tmp-un0cnd3r/src/docker-src.2018-03-31-05.05.27.3469/qemu.tar.vroot/ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce'
tar: /var/tmp/patchew-tester-tmp-un0cnd3r/src/docker-src.2018-03-31-05.05.27.3469/qemu.tar: Wrote only 2048 of 10240 bytes
tar: Error is not recoverable: exiting now
failed to create tar file
COPY RUNNER
RUN test-build in qemu:min-glib
tar: Unexpected EOF in archive
tar: Unexpected EOF in archive
tar: Error is not recoverable: exiting now
/var/tmp/qemu/run: line 32: prep_fail: command not found
Environment variables:
HOSTNAME=547f25a98335
MAKEFLAGS= -j8
J=8
CCACHE_DIR=/var/tmp/ccache
EXTRA_CONFIGURE_OPTS=
V=
SHOW_ENV=1
PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/
TARGET_LIST=
SHLVL=1
HOME=/root
TEST_DIR=/tmp/qemu-test
FEATURES= dtc
DEBUG=
_=/usr/bin/env
/var/tmp/qemu/run: line 52: cd: /tmp/qemu-test/src/tests/docker: No such file or directory
/var/tmp/qemu/run: line 57: /test-build: No such file or directory
/var/tmp/qemu/run: line 57: exec: /test-build: cannot execute: No such file or directory
Traceback (most recent call last):
File "./tests/docker/docker.py", line 407, in <module>
sys.exit(main())
File "./tests/docker/docker.py", line 404, in main
return args.cmdobj.run(args, argv)
File "./tests/docker/docker.py", line 261, in run
return Docker().run(argv, args.keep, quiet=args.quiet)
File "./tests/docker/docker.py", line 229, in run
quiet=quiet)
File "./tests/docker/docker.py", line 147, in _do_check
return subprocess.check_call(self._command + cmd, **kwargs)
File "/usr/lib64/python2.7/subprocess.py", line 186, in check_call
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['docker', 'run', '--label', 'com.qemu.instance.uuid=b0959c3234c211e8af9552540069c830', '-u', '0', '--security-opt', 'seccomp=unconfined', '--rm', '--net=none', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=8', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/root/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-un0cnd3r/src/docker-src.2018-03-31-05.05.27.3469:/var/tmp/qemu:z,ro', 'qemu:min-glib', '/var/tmp/qemu/run', 'test-build']' returned non-zero exit status 126
make[1]: *** [tests/docker/Makefile.include:129: docker-run] Error 1
make[1]: Leaving directory '/var/tmp/patchew-tester-tmp-un0cnd3r/src'
make: *** [tests/docker/Makefile.include:163: docker-run-test-build@min-glib] Error 2
real 0m28.119s
user 0m9.507s
sys 0m6.717s
=== OUTPUT END ===
Test command exited with code: 2
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12] nbd/client: Correctly handle bad server REP_META_CONTEXT
2018-03-30 16:59 ` Vladimir Sementsov-Ogievskiy
@ 2018-04-02 13:59 ` Eric Blake
0 siblings, 0 replies; 5+ messages in thread
From: Eric Blake @ 2018-04-02 13:59 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel
Cc: Paolo Bonzini, open list:Network Block Dev...
[-- Attachment #1: Type: text/plain, Size: 4858 bytes --]
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 <vsementsov@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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-04-02 13:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-29 23:18 [Qemu-devel] [PATCH for-2.12] nbd/client: Correctly handle bad server REP_META_CONTEXT Eric Blake
2018-03-30 16:59 ` Vladimir Sementsov-Ogievskiy
2018-04-02 13:59 ` Eric Blake
2018-03-31 7:44 ` no-reply
2018-03-31 9:05 ` no-reply
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).