On Mon, Jun 17, 2019 at 03:05:06PM -0500, Eric Blake wrote:
A devious plugin can cause an 8-fold increase in the reply size in
relation to the request size, by alternating status every byte of the
request. In the worst case, this can cause the client to reject our
response and kill the connection because the response is too large.
What's more, it consumes a lot of memory to track that many extents.
Let's put an upper bound on the maximum number of extents we are
willing to return (1M extents is an 8M reply chunk).
Pre-patch, this can be used to demonstrate the problem:
$ nbdkit -f sh - <<\EOF
#!/bin/bash
size=$((9*1024*1024))
case $1 in
get_size) echo $size ;;
pread) dd iflag=skip_bytes,count_bytes skip=$4 count=$3 if=/dev/zero || exit 1 ;;
can_extents) ;;
extents)
# Unrealistic in real life, but works to provoke the bug. For a full 9M
# query, this produces ~100M for nbdkit to parse, and in turn tries to
# produce a 72M reply chunk if we don't cap extents.
for ((i=$4;i<$4+$3;i++)); do
echo $i 1 $((i&1))
done ;;
*) exit 2 ;;
esac
EOF
$ ~/libnbd/run sh/nbdsh
...
nbd> h.connect_tcp("localhost","10809")
nbd> def f(data,metacontext,offset,e):
... print ("entries:%d" % len(e))
...
nbd> h.block_status(9*1024*1024,0,0,f)
Traceback (most recent call last):
File "/usr/lib64/python3.7/code.py", line 90, in runcode
exec(code, self.locals)
File "<console>", line 1, in <module>
File "/home/eblake/libnbd/python/nbd.py", line 577, in block_status
return libnbdmod.block_status (self._o, count, offset, data, extent, flags)
RuntimeError: nbd_block_status: invalid server reply length
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
server/extents.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/server/extents.c b/server/extents.c
index c4224916..0ebd77b1 100644
--- a/server/extents.c
+++ b/server/extents.c
@@ -45,6 +45,12 @@
#include "internal.h"
+/* Cap nr_extents to avoid sending over-large replies to the client,
+ * and to avoid a plugin with frequent alternations consuming too much
+ * memory.
+ */
+#define MAX_EXTENTS (1 * 1024 * 1024)
+
struct nbdkit_extents {
struct nbdkit_extent *extents;
size_t nr_extents, allocated;
@@ -163,8 +169,8 @@ nbdkit_add_extent (struct nbdkit_extents *exts,
if (length == 0)
return 0;
- /* Ignore extents beyond the end of the range. */
- if (offset >= exts->end)
+ /* Ignore extents beyond the end of the range, or if list is full. */
+ if (offset >= exts->end || exts->nr_extents == MAX_EXTENTS)
return 0;
It looks OK, just one thought would be to either return the information to the
caller or expose the MAX_EXTENTS somehow. That way the plugin could just
immediately return without wasting time on (not) adding more and more extents.
But I can't think of a way to do it nicely. This function could return
something else than 0, but this does not seem to be used in nbdkit very much.
Exposing MAX_EXTENTS would, on the other hand, had to be done at runtime and
wiring it into the plugins would be too much duplicated code.
So that's just an idea.
/* Shorten extents that overlap the end of the range. */
--
2.20.1
_______________________________________________
Libguestfs mailing list
Libguestfs(a)redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs