5:12 p.m.
This is in response to https://gitlab.com/nbdkit/libnbd/-/issues/12
where nbdinfo is having a hard time because nbdkit is outputting data
that is not in compliance with the current NBD spec (which states that
if client and server agreed on a minimum blocksize, then extents must
be aligned to that).
I still have more to write:
- add a unit test for the server change at 4G (in addition to the
unit test I got working for blocksize-policy tripping on the same
problem with a much smaller unaligned-size data image)
- libnbd: patch nbdinfo to tolerate unaligned extents from the
server. Even if the spec says its wrong, existing servers in the
wild do it, so we should be tolerant.
- upstream NBD spec: propose weakening the standard from SHALL to
SHOULD provide aligned extents
- blocksize-policy: Consider patches to allow aligned offset but
unaligned length IF the length extends from offset to the end of
the unaligned-size disk. Document that an unaligned tail is hard
to read, and suggest use of the truncate filter to deal with it.
But as this has taken me the better part of today, I wanted to put out
what I have so far.
Eric Blake (4):
human-size: Add human_size_parse_substr
data: Allow scaling suffixes on @decimal
blocksize-policy: Round extents to match minimum alignment
server: Align the truncation of large block_status results
plugins/data/nbdkit-data-plugin.pod | 3 +-
tests/Makefile.am | 2 +
common/include/human-size-test-cases.h | 83 ++++++++++--------
common/include/human-size.h | 57 ++++++++----
common/include/test-human-size.c | 52 +++++++++--
server/protocol.c | 12 ++-
server/test-public.c | 13 +--
plugins/data/format.c | 115 ++++++++++++-------------
filters/blocksize-policy/policy.c | 15 +++-
tests/test-data-format.sh | 8 +-
tests/test-eval-extents.sh | 6 +-
11 files changed, 230 insertions(+), 136 deletions(-)
--
2.49.0
5:12 p.m.
New subject: [nbdkit PATCH 1/4] human-size: Add human_size_parse_substr
I want to teach the data plugin how to parse @4k out of a larger input
string, but that entails the need for a way to opt in to not erroring
out when an unrecognized suffix is present.
Some of the existing unit tests expose cases where parsing a value
substring out of a larger string can have interesting results; but in
the case of the data plugin, the caller will validate that any
remaining suffix starts with whitespace (and not '.' or a letter),
which should avoid the oddest cases due to the additional context that
the unit test lacks.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
common/include/human-size-test-cases.h | 83 ++++++++++++++------------
common/include/human-size.h | 57 ++++++++++++------
common/include/test-human-size.c | 52 ++++++++++++++--
server/test-public.c | 13 ++--
4 files changed, 137 insertions(+), 68 deletions(-)
diff --git a/common/include/human-size-test-cases.h
b/common/include/human-size-test-cases.h
index f351f7c5..ef845f4c 100644
--- a/common/include/human-size-test-cases.h
+++ b/common/include/human-size-test-cases.h
@@ -38,17 +38,22 @@
/* Just some common test cases shared (in nbdkit) between
* common/include/test-human-size.c and server/test-public.c
*/
-static struct pair {
+static struct tuple {
const char *str;
int64_t res;
-} pairs[] = {
+ const char *tail;
+} tuples[] = {
/* Bogus strings */
{ "", -1 },
- { "0x0", -1 },
+ { " ", -1 },
+ { "+ 1", -1 },
{ "garbage", -1 },
- { "0garbage", -1 },
- { "8E", -1 },
- { "8192P", -1 },
+
+ /* Extracting substring value from larger string */
+ { "0junk", 0, "junk" }, /* no suffix j */
+ { "0garbage", 0, "arbage" }, /* 0g recognized */
+ { "1 1", 1, " 1" },
+ { "1M 1", 1024 * 1024, " 1" },
/* Strings leading to overflow */
{ "9223372036854775808", -1 }, /* INT_MAX + 1 */
@@ -56,6 +61,8 @@ static struct pair {
{ "18446744073709551615", -1 }, /* UINT64_MAX */
{ "18446744073709551616", -1 }, /* UINT64_MAX + 1 */
{ "999999999999999999999999", -1 },
+ { "8E", -1 },
+ { "8192P", -1 },
/* Strings representing negative values */
{ "-1", -1 },
@@ -68,39 +75,41 @@ static struct pair {
{ "-18446744073709551614", -1 }, /* -UINT64_MAX + 1 */
/* Strings we may want to support in the future */
- { "M", -1 },
- { "1MB", -1 },
- { "1MiB", -1 },
- { "1.5M", -1 },
+ { "0x0", 0, "x0" }, /* Nicer would be 0 with tail "" */
+ { "M", -1 }, /* Nicer would be 1024 * 1024 with tail "" */
+ { "1MB", 1024 * 1024, "B" }, /* Nicer would be 1000 * 1000 with
tail "" */
+ { "1MiB", 1024 * 1024, "iB" }, /* Nicer would be 1024 * 1024 with
tail "" */
+ { "1.5M", 1, ".5M" }, /* Nicer would be 512 * 3 * 1024 with tail
"" */
+ { "1.5MB", 1, ".5MB" }, /* Nicer would be 1500 * 1000 with tail
"" */
/* Valid strings */
- { "-0", 0 },
- { "0", 0 },
- { "+0", 0 },
- { " 08", 8 },
- { "1", 1 },
- { "+1", 1 },
- { "1234567890", 1234567890 },
- { "+1234567890", 1234567890 },
- { "9223372036854775807", INT64_MAX },
- { "1s", 512 },
- { "2S", 1024 },
- { "1b", 1 },
- { "1B", 1 },
- { "1k", 1024 },
- { "1K", 1024 },
- { "1m", 1024 * 1024 },
- { "1M", 1024 * 1024 },
- { "+1M", 1024 * 1024 },
- { "1g", 1024 * 1024 * 1024 },
- { "1G", 1024 * 1024 * 1024 },
- { "1t", 1024LL * 1024 * 1024 * 1024 },
- { "1T", 1024LL * 1024 * 1024 * 1024 },
- { "1p", 1024LL * 1024 * 1024 * 1024 * 1024 },
- { "1P", 1024LL * 1024 * 1024 * 1024 * 1024 },
- { "8191p", 1024LL * 1024 * 1024 * 1024 * 1024 * 8191 },
- { "1e", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 },
- { "1E", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024 },
+ { " -0", 0, "" },
+ { "0", 0, "" },
+ { "+0", 0, "" },
+ { " 08", 8, "" },
+ { "1", 1, "" },
+ { "+1", 1, "" },
+ { "1234567890", 1234567890, "" },
+ { "+1234567890", 1234567890, "" },
+ { "9223372036854775807", INT64_MAX, "" },
+ { "1s", 512, "" },
+ { "2S", 1024, "" },
+ { "1b", 1, "" },
+ { "1B", 1, "" },
+ { "1k", 1024, "" },
+ { "1K", 1024, "" },
+ { "1m", 1024 * 1024, "" },
+ { "1M", 1024 * 1024, "" },
+ { "+1M", 1024 * 1024, "" },
+ { "1g", 1024 * 1024 * 1024, "" },
+ { "1G", 1024 * 1024 * 1024, "" },
+ { "1t", 1024LL * 1024 * 1024 * 1024, "" },
+ { "1T", 1024LL * 1024 * 1024 * 1024, "" },
+ { "1p", 1024LL * 1024 * 1024 * 1024 * 1024, "" },
+ { "1P", 1024LL * 1024 * 1024 * 1024 * 1024, "" },
+ { "8191p", 1024LL * 1024 * 1024 * 1024 * 1024 * 8191, "" },
+ { "1e", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024, "" },
+ { "1E", 1024LL * 1024 * 1024 * 1024 * 1024 * 1024, "" },
};
#endif /* NBDKIT_HUMAN_SIZE_TEST_CASES_H */
diff --git a/common/include/human-size.h b/common/include/human-size.h
index 788dbd7b..aa551888 100644
--- a/common/include/human-size.h
+++ b/common/include/human-size.h
@@ -39,14 +39,17 @@
/* Attempt to parse a string with a possible scaling suffix, such as
* "2M". Disk sizes cannot usefully exceed off_t (which is signed)
- * and cannot be negative.
+ * and cannot be negative. If rest is not NULL, the number being
+ * parsed is treated as a substring within a larger input; if a value
+ * was parsed, *rest is set to the first unparsed byte of str. If
+ * rest is NULL, any trailing garbage is treated as an error.
*
* On error, returns -1 and sets *error and *pstr. You can form a
* final error message by appending "<error>: <pstr>".
*/
static inline int64_t
-human_size_parse (const char *str,
- const char **error, const char **pstr)
+human_size_parse_substr (const char *str, char **rest,
+ const char **error, const char **pstr)
{
int64_t size;
char *end;
@@ -56,6 +59,8 @@ human_size_parse (const char *str,
/* XXX Should we allow hex? If so, hex cannot use scaling suffixes,
* because some of them are valid hex digits.
*/
+ if (rest)
+ *rest = NULL;
errno = 0;
size = strtoimax (str, &end, 10);
if (str == end) {
@@ -77,7 +82,7 @@ human_size_parse (const char *str,
switch (*end) {
/* No suffix */
case '\0':
- end--; /* Safe, since we already filtered out empty string */
+ /* Safe, since we already filtered out empty string */
break;
/* Powers of 1024 */
@@ -100,6 +105,7 @@ human_size_parse (const char *str,
scale *= 1024;
/* fallthru */
case 'b': case 'B':
+ end++;
break;
/* "sectors", ie. units of 512 bytes, even if that's not the real
@@ -107,22 +113,8 @@ human_size_parse (const char *str,
*/
case 's': case 'S':
scale = 512;
+ end++;
break;
-
- default:
- *error = "could not parse size: unknown suffix";
- *pstr = end;
- return -1;
- }
-
- /* XXX Maybe we should support 'MiB' as a synonym for 'M'; and
'MB'
- * for powers of 1000, for similarity to GNU tools. But for now,
- * anything beyond 'M' is dropped.
- */
- if (end[1]) {
- *error = "could not parse size: unknown suffix";
- *pstr = end;
- return -1;
}
if (INT64_MAX / scale < size) {
@@ -131,7 +123,34 @@ human_size_parse (const char *str,
return -1;
}
+ /* XXX Maybe we should support 'MiB' as a synonym for 'M'; and
'MB'
+ * for powers of 1000, for similarity to GNU tools. But for now,
+ * anything beyond 'M' is dropped.
+ */
+ if (rest)
+ *rest = end;
+ else if (*end) {
+ *error = "could not parse size: unknown suffix";
+ *pstr = end;
+ return -1;
+ }
+
return size * scale;
}
+/* Attempt to parse a string with a possible scaling suffix, such as
+ * "2M". Disk sizes cannot usefully exceed off_t (which is signed)
+ * and cannot be negative. str must not have any trailing garbage.
+ *
+ * On error, returns -1 and sets *error and *pstr. You can form a
+ * final error message by appending "<error>: <pstr>".
+ */
+static inline int64_t
+human_size_parse (const char *str,
+ const char **error, const char **pstr)
+{
+ return human_size_parse_substr (str, NULL, error, pstr);
+}
+
+
#endif /* NBDKIT_HUMAN_SIZE_H */
diff --git a/common/include/test-human-size.c b/common/include/test-human-size.c
index 7f75bb4b..3e388e39 100644
--- a/common/include/test-human-size.c
+++ b/common/include/test-human-size.c
@@ -36,10 +36,11 @@
#include <stdlib.h>
#include <stdbool.h>
#include <stdint.h>
+#include <string.h>
#include "array-size.h"
#include "human-size.h"
-#include "human-size-test-cases.h" /* defines 'pairs' below */
+#include "human-size-test-cases.h" /* defines 'tuples' below */
int
main (void)
@@ -47,20 +48,59 @@ main (void)
size_t i;
bool pass = true;
- for (i = 0; i < ARRAY_SIZE (pairs); i++) {
+ for (i = 0; i < ARRAY_SIZE (tuples); i++) {
const char *error = NULL, *pstr = NULL;
+ char *rest = NULL;
int64_t r;
+ int64_t expect;
- r = human_size_parse (pairs[i].str, &error, &pstr);
- if (r != pairs[i].res) {
+ r = human_size_parse (tuples[i].str, &error, &pstr);
+ expect = tuples[i].tail && *tuples[i].tail ? -1 : tuples[i].res;
+ if (r != expect) {
fprintf (stderr,
"Wrong parse for %s, got %" PRId64 ", expected %"
PRId64 "\n",
- pairs[i].str, r, pairs[i].res);
+ tuples[i].str, r, expect);
pass = false;
}
if (r == -1) {
if (error == NULL || pstr == NULL) {
- fprintf (stderr, "Wrong error message handling for %s\n",
pairs[i].str);
+ fprintf (stderr, "Wrong error message handling for %s\n",
+ tuples[i].str);
+ pass = false;
+ }
+ }
+
+ r = human_size_parse_substr (tuples[i].str, &rest, &error, &pstr);
+ if (r != tuples[i].res) {
+ fprintf (stderr,
+ "Wrong parse for %s, got %" PRId64 ", expected %"
PRId64 "\n",
+ tuples[i].str, r, tuples[i].res);
+ pass = false;
+ }
+ if (r == -1) {
+ if (error == NULL || pstr == NULL) {
+ fprintf (stderr, "Wrong error message handling for %s\n",
+ tuples[i].str);
+ pass = false;
+ }
+ if (rest != NULL) {
+ fprintf (stderr,
+ "Wrong suffix handling for %s, expected NULL, got
'%s'\n",
+ tuples[i].str, rest);
+ pass = false;
+ }
+ }
+ else {
+ if (rest == NULL) {
+ fprintf (stderr,
+ "Wrong suffix handling for %s, expected '%s', got
NULL\n",
+ tuples[i].str, tuples[i].tail);
+ pass = false;
+ }
+ else if (strcmp (rest, tuples[i].tail) != 0) {
+ fprintf (stderr,
+ "Wrong suffix handling for %s, expected '%s', got
'%s'\n",
+ tuples[i].str, tuples[i].tail, rest);
pass = false;
}
}
diff --git a/server/test-public.c b/server/test-public.c
index 0eddd6ca..bb30134e 100644
--- a/server/test-public.c
+++ b/server/test-public.c
@@ -110,7 +110,7 @@ backend_default_export (struct backend *b, int readonly)
/* Unit tests. */
-#include "human-size-test-cases.h" /* defines 'pairs' below */
+#include "human-size-test-cases.h" /* defines 'tuples' below */
static bool
test_nbdkit_parse_size (void)
@@ -118,19 +118,20 @@ test_nbdkit_parse_size (void)
size_t i;
bool pass = true;
- for (i = 0; i < ARRAY_SIZE (pairs); i++) {
+ for (i = 0; i < ARRAY_SIZE (tuples); i++) {
int64_t r;
+ int64_t expect = tuples[i].tail && *tuples[i].tail ? -1 : tuples[i].res;
error_flagged = false;
- r = nbdkit_parse_size (pairs[i].str);
- if (r != pairs[i].res) {
+ r = nbdkit_parse_size (tuples[i].str);
+ if (r != expect) {
fprintf (stderr,
"Wrong parse for %s, got %" PRId64 ", expected %"
PRId64 "\n",
- pairs[i].str, r, pairs[i].res);
+ tuples[i].str, r, expect);
pass = false;
}
if ((r == -1) != error_flagged) {
- fprintf (stderr, "Wrong error message handling for %s\n", pairs[i].str);
+ fprintf (stderr, "Wrong error message handling for %s\n",
tuples[i].str);
pass = false;
}
}
--
2.49.0
5:12 p.m.
New subject: [nbdkit PATCH 2/4] data: Allow scaling suffixes on @decimal
Although you can write 'nbdkit data "@$((4*1024)) 1"', it is more
convenient to be able to write 'nbdkit data "@4k 1"'. To do that,
make use of our existing code for suffix scaling when we know we have
a decimal value, while still accepting non-decimal values without
scaling for backwards compatability. Since we must do substring
parsing (there is likely more data after the offset directive), we
insist that after the value is parsed, the string must be on
whitespace before the next directive.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
plugins/data/nbdkit-data-plugin.pod | 3 +-
plugins/data/format.c | 115 ++++++++++++++--------------
tests/test-data-format.sh | 8 +-
3 files changed, 65 insertions(+), 61 deletions(-)
diff --git a/plugins/data/nbdkit-data-plugin.pod b/plugins/data/nbdkit-data-plugin.pod
index e94102ba..bb57e912 100644
--- a/plugins/data/nbdkit-data-plugin.pod
+++ b/plugins/data/nbdkit-data-plugin.pod
@@ -230,7 +230,8 @@ generates the 4 byte sequence C<0 0 0 1>.
Moves the current offset to C<OFFSET>. The offset may be specified as
either decimal, octal (prefixed by C<0>) or hexadecimal (prefixed by
-C<0x>). Offset C<@0> is the first byte of the disk.
+C<0x>); when using decimal, it is also possible to supply a scaling
+suffix such as C<k> or C<M>. Offset C<@0> is the first byte of the
disk.
=item B<@+>N
diff --git a/plugins/data/format.c b/plugins/data/format.c
index 81255b07..796a493c 100644
--- a/plugins/data/format.c
+++ b/plugins/data/format.c
@@ -41,6 +41,7 @@
#include <string.h>
#include <assert.h>
#include <signal.h>
+#include <ctype.h>
#define NBDKIT_API_VERSION 2
#include <nbdkit-plugin.h>
@@ -51,6 +52,7 @@
#include "checked-overflow.h"
#include "cleanup.h"
#include "hexdigit.h"
+#include "human-size.h"
#include "ispowerof2.h"
#include "minmax.h"
#include "nbdkit-string.h"
@@ -494,6 +496,9 @@ parser (int level, const char *value, size_t *start, size_t len,
int j, n;
int64_t i64, m;
size_t flen;
+ char sign;
+ char *end;
+ const char *err, *pstr;
switch (value[i]) {
case '#': /* # comment */
@@ -505,68 +510,60 @@ parser (int level, const char *value, size_t *start, size_t len,
case '@': /* @OFFSET */
if (++i == len) goto parse_error;
switch (value[i]) {
- case '+': /* @+N */
- if (++i == len) goto parse_error;
- if (sscanf (&value[i], "%" SCNi64 "%n", &i64, &n)
== 1) {
- if (i64 < 0) {
- nbdkit_error ("data parameter after @+ must not be negative");
- return -1;
- }
- i += n;
- APPEND_EXPR (new_node (expr (EXPR_REL_OFFSET, i64)));
- }
- else
- goto parse_error;
- break;
- case '-': /* @-N */
- if (++i == len) goto parse_error;
- if (sscanf (&value[i], "%" SCNi64 "%n", &i64, &n)
== 1) {
- if (i64 < 0) {
- nbdkit_error ("data parameter after @- must not be negative");
- return -1;
- }
- i += n;
- APPEND_EXPR (new_node (expr (EXPR_REL_OFFSET, -i64)));
- }
- else
- goto parse_error;
- break;
- case '^': /* @^ALIGNMENT */
- if (++i == len) goto parse_error;
- /* We must use %i into i64 in order to parse 0x etc. */
- if (sscanf (&value[i], "%" SCNi64 "%n", &i64, &n)
== 1) {
- if (i64 < 0) {
- nbdkit_error ("data parameter after @^ must not be negative");
- return -1;
- }
- /* XXX fix this arbitrary restriction */
- if (!is_power_of_2 (i64)) {
- nbdkit_error ("data parameter @^%" PRIi64 " must be a power of
2",
- i64);
- return -1;
- }
- i += n;
- APPEND_EXPR (new_node (expr (EXPR_ALIGN_OFFSET, (uint64_t) i64)));
- }
- else
- goto parse_error;
- break;
- case '0': case '1': case '2': case '3': case
'4':
- case '5': case '6': case '7': case '8': case
'9':
- /* We must use %i into i64 in order to parse 0x etc. */
- if (sscanf (&value[i], "%" SCNi64 "%n", &i64, &n)
== 1) {
- if (i64 < 0) {
- nbdkit_error ("data parameter @OFFSET must not be negative");
- return -1;
- }
- i += n;
- APPEND_EXPR (new_node (expr (EXPR_ABS_OFFSET, (uint64_t) i64)));
- }
- else
- goto parse_error;
+ case '+':
+ case '-':
+ case '^':
+ sign = value[i++];
break;
default:
+ sign = '\0';
+ }
+ if (value[i] == '0') {
+ /* hex or octal, no scaling suffix possible */
+ if (sscanf (&value[i], "%" SCNi64 "%n", &i64, &n)
== 1) {
+ if (i64 < 0) {
+ nbdkit_error ("data parameter after @ must not be negative");
+ return -1;
+ }
+ i += n;
+ }
+ else
+ goto parse_error;
+ }
+ else if (isdigit(value[i])) {
+ /* decimal, scaling suffix allowed */
+ i64 = human_size_parse_substr (&value[i], &end, &err, &pstr);
+ if (i64 == -1) {
+ nbdkit_error ("%s: %s", err, pstr);
+ return -1;
+ }
+ if (*end && !isspace (*end))
+ goto parse_error;
+ i = end - value;
+ }
+ else
+ /* empty string or unknown byte */
goto parse_error;
+
+ switch (sign) {
+ case '+': /* @+N */
+ APPEND_EXPR (new_node (expr (EXPR_REL_OFFSET, i64)));
+ break;
+ case '-': /* @-N */
+ APPEND_EXPR (new_node (expr (EXPR_REL_OFFSET, -i64)));
+ break;
+ case '^': /* @^ALIGNMENT */
+ /* XXX fix this arbitrary restriction */
+ if (!is_power_of_2 (i64)) {
+ nbdkit_error ("data parameter @^%" PRIi64 " must be a power of
2",
+ i64);
+ return -1;
+ }
+ APPEND_EXPR (new_node (expr (EXPR_ALIGN_OFFSET, (uint64_t) i64)));
+ break;
+ default:
+ APPEND_EXPR (new_node (expr (EXPR_ABS_OFFSET, (uint64_t) i64)));
+ break;
}
break;
diff --git a/tests/test-data-format.sh b/tests/test-data-format.sh
index 59f2b102..43929136 100755
--- a/tests/test-data-format.sh
+++ b/tests/test-data-format.sh
@@ -66,7 +66,7 @@ if size > 0:
actual = h.pread(size, 0)
else:
actual = b\"\"
-def trunc(s): return s[:128] + (s[128:] and b\"...\")
+def trunc(s): return s[:128] + (s[128:] and b\"...\" + s[-4:])
print(\"actual: %r\" % trunc(actual))
print(\"expected: %r\" % trunc(expected))
assert actual == expected
@@ -117,6 +117,12 @@ do_test '@+4 "\x00"'
'b"\x00\x00\x00\x00\x00"'
do_test '@+4 @-1 "\x00"' 'b"\x00\x00\x00\x00"'
do_test '1 @^4 "\x00"' 'b"\x01\x00\x00\x00\x00"'
+#----------------------------------------------------------------------
+# Larger offsets.
+do_test '@1k "\x01"' 'bytearray(1024) + b"\x01"'
+do_test '@0x400 "\x01"' 'bytearray(1024) + b"\x01"'
+do_test '@02000 "\x01"' 'bytearray(1024) + b"\x01"'
+
#----------------------------------------------------------------------
# Comments.
do_test '#' 'b""'
--
2.49.0
5:12 p.m.
New subject: [nbdkit PATCH 3/4] blocksize-policy: Round extents to match minimum alignment
Since a client is very likely to make further I/O calls at offsets
returned in a block status call (for example, 'nbdinfo --map
--totals'), and the blocksize-policy filter can set a minimum
alignment larger than what the plugin does by default, it makes sense
to force extents to that alignment.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
tests/Makefile.am | 2 ++
filters/blocksize-policy/policy.c | 15 ++++++++++++++-
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 63730821..242a03e4 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1656,11 +1656,13 @@ TESTS += \
test-blocksize-policy.sh \
test-blocksize-error-policy.sh \
test-blocksize-write-disconnect.sh \
+ test-blocksize-policy-extents.sh \
$(NULL)
EXTRA_DIST += \
test-blocksize-policy.sh \
test-blocksize-error-policy.sh \
test-blocksize-write-disconnect.sh \
+ test-blocksize-policy-extents.sh \
$(NULL)
if HAVE_BZLIB
diff --git a/filters/blocksize-policy/policy.c b/filters/blocksize-policy/policy.c
index 51bea91e..d656c956 100644
--- a/filters/blocksize-policy/policy.c
+++ b/filters/blocksize-policy/policy.c
@@ -38,6 +38,7 @@
#include <inttypes.h>
#include <string.h>
#include <errno.h>
+#include <assert.h>
#include <nbdkit-filter.h>
@@ -373,10 +374,22 @@ policy_extents (nbdkit_next *next,
void *handle, uint32_t count, uint64_t offset, uint32_t flags,
struct nbdkit_extents *extents, int *err)
{
+ uint32_t minimum, preferred, maximum;
+ int res;
+
if (check_policy (next, handle, "extents", false, count, offset, err) == -1)
return -1;
- return next->extents (next, count, offset, flags, extents, err);
+ /* Munge the plugin's response to values that match the alignment we
+ * are enforcing; that way, a client that performs later operations
+ * at boundaries determined by the block status result will still be
+ * aligned. Since check_policy succeeded, policy_block_size will too.
+ */
+ res = policy_block_size (next, handle,
+ &minimum, &preferred, &maximum);
+ assert (res == 0);
+ return nbdkit_extents_aligned (next, count, offset, flags,
+ minimum ?: 1, extents, err);
}
static struct nbdkit_filter filter = {
--
2.49.0
5:12 p.m.
New subject: [nbdkit PATCH 4/4] server: Align the truncation of large block_status results
The NBD spec currently requires that block status results be aligned
to any advertised minimum block size (although I'm submitting a patch
in parallel to relax that slightly, to cater to the fact that not all
existing servers comply). And while most plugins and filters _do_
happen to provide aligned results when given aligned inputs, we have
the corner case where an aligned request that results in a 4G+ aligned
response from the plugin is truncated in the server code to an
unaligned answer, which can confuse clients such as 'nbdinfo --map'.
Prior to advertising block sizes, the server didn't have to care about
alignments: the spec only requires aligned results when the client
asks and the server replies with a minimum block size. And even with
the introduction of the .block_size callback, the server still doesn't
have to care for reads or writes: if the client is compliant (or you
use a filter like blocksize or blocksize-policy), the plugin never
sees an unaligned request, and the response will have the same
alignment as the request (because we don't yet implement the ability
to report holes via NBD_CMD_READ - that would require the v3 plugin
API). It is _only_ for extents where we are at the mercy of the
plugin not splitting extents differently than what it advertised as
its minimum block size, which is generally the case, with one obvious
exception: any time the plugin widens an extent and reports 4G or
larger, it is the server (not the plugin) that truncates the request
down to the 32-bit protocol limits.
But since we know that .block_size cannot set a minimum block size
larger than 64k, it is far easier to just truncate the server's
response at a point that is guaranteed to be aligned for all possible
plugins, than to have the server try and munge alignment itself. The
change here is observable in test-eval-extents.sh which was added as
part of the fix for CVE-2025-47711; but fortunately, spec
non-compliance is different than denial of service, so versions of
nbdkit with this bug do NOT represent yet another security lapse, and
the changed test is not any weaker.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
server/protocol.c | 12 +++++++++---
tests/test-eval-extents.sh | 6 +++---
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/server/protocol.c b/server/protocol.c
index b4b1c162..28b1a554 100644
--- a/server/protocol.c
+++ b/server/protocol.c
@@ -493,13 +493,19 @@ extents_to_block_descriptors (struct nbdkit_extents *extents,
if (i == 0)
assert (e.offset == offset);
- /* Must not exceed UINT32_MAX. */
- blocks[i].length = length = MIN (e.length, UINT32_MAX);
+ /* Must not exceed UINT32_MAX. However, if the plugin was
+ * supplying aligned answers, we don't want our truncation to
+ * result in an unaligned point. Rather than tracking whether a
+ * minimumg block size was advertised, it is simpler to just
+ * truncate at 64k shy of 4G, since minimum block size cannot
+ * exceed 64k.
+ */
+ blocks[i].length = length = MIN (e.length, UINT32_MAX & ~0xffff);
blocks[i].status_flags = e.type & 3;
(*nr_blocks)++;
pos += length;
- if (pos >= offset + count) /* this must be the last block */
+ if (pos >= offset + count || length < e.length)
break;
/* If we reach here then we must have consumed this whole
diff --git a/tests/test-eval-extents.sh b/tests/test-eval-extents.sh
index 92b503e6..af0f2885 100755
--- a/tests/test-eval-extents.sh
+++ b/tests/test-eval-extents.sh
@@ -65,7 +65,7 @@ nbdkit eval \
eval-extents.out
cat eval-extents.out
diff -u - eval-extents.out <<EOF
-[4294967294, 1, 1073741824, 2]
-[4294967295, 1]
-[4294967295, 1]
+[4294901760, 1]
+[4294901760, 1]
+[4294901760, 1]
EOF
--
2.49.0
4
days inactive
5
days old
3 comments
1 participants
participants (1)
-
Eric Blake