On Fri, May 27, 2022 at 05:25:43PM -0500, Eric Blake wrote:
Commit c7fbc71d missed an optimization: when we guarantee that a
pread
buffer is pre-zeroed, we don't need to waste time memset()ing it when
the server sends a hole. But the next commit e0953cb7 lets the user
skip pre-zeroing, at which point the memset is required to avoid data
corruption. Thankfully, we are not leaking bogus bytes when a server
sends a hole, but we CAN speed up the case where we have pre-zeroed
the buffer to avoid a second memset(). Because the user can change
the state of pread_initialize on the fly (including while a pread is
still in flight), we must track the state per-command as it was before
we send the request to the server.
---
lib/internal.h | 1 +
generator/states-reply-structured.c | 5 +++--
lib/rw.c | 18 +++++++++++-------
3 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h
index 6aaced3..885cee1 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -353,6 +353,7 @@ struct command {
struct command_cb cb;
enum state state; /* State to resume with on next POLLIN */
bool data_seen; /* For read, true if at least one data chunk seen */
+ bool initialized; /* For read, true if getting a hole may skip memset */
uint32_t error; /* Local errno value */
};
diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c
index 7001047..12c24f5 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -1,5 +1,5 @@
/* nbd client library in userspace: state machine
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -436,7 +436,8 @@ STATE_MACHINE {
* 0-length replies are broken. Still, it's easy enough to support
* them as an extension, and this works even when length == 0.
*/
- memset (cmd->data + offset, 0, length);
+ if (!cmd->initialized)
+ memset (cmd->data + offset, 0, length);
if (CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) {
int error = cmd->error;
diff --git a/lib/rw.c b/lib/rw.c
index c5d041d..f32481a 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -244,14 +244,18 @@ nbd_internal_command_common (struct nbd_handle *h,
if (cb)
cmd->cb = *cb;
- /* For NBD_CMD_READ, cmd->data was pre-zeroed in the prologue
- * created by the generator. Thus, if a (non-compliant) server with
- * structured replies fails to send back sufficient data to cover
- * the whole buffer, we still behave as if it had sent zeroes for
- * those portions, rather than leaking any uninitialized data, and
- * without having to complicate our state machine to track which
- * portions of the read buffer were actually populated.
+ /* For NBD_CMD_READ, cmd->data defaults to being pre-zeroed in the
+ * prologue created by the generator. Thus, if a (non-compliant)
+ * server with structured replies fails to send back sufficient data
+ * to cover the whole buffer, we still behave as if it had sent
+ * zeroes for those portions, rather than leaking any uninitialized
+ * data, and without having to complicate our state machine to track
+ * which portions of the read buffer were actually populated. But
+ * if the user opts in to disabling set_pread_initialize, then we
+ * need to memset zeroes as they are read (and the user gets their
+ * own garbage back in the case of a non-compliant server).
*/
+ cmd->initialized = h->pread_initialize;
/* Add the command to the end of the queue. Kick the state machine
* if there is no other command being processed, otherwise, it will
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat
http://people.redhat.com/~rjones
Read my programming and virtualization blog:
http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW