truncate: Fix corruption when plugin changes per-connection size
commitf71080b1651aa12510a23b263080d7ad73b7e889
authorEric Blake <eblake@redhat.com>
Sat, 27 Apr 2019 20:15:43 +0000 (27 15:15 -0500)
committerEric Blake <eblake@redhat.com>
Mon, 29 Apr 2019 19:23:29 +0000 (29 14:23 -0500)
tree014701ea2fceae4843136677ba7a17ababe1332f
parentfbd1058761b4dca7c2aa5e067886200e7500dfd5
truncate: Fix corruption when plugin changes per-connection size

The truncate filter tried to be careful to lock access to setting or
reading the real_size variable learned from calling
next_ops->get_size, in anticipation of not behaving incorrectly if the
NBD protocol makes dynamic resize supported, and where the global
variable could serve as a cache rather than having to always call
next_ops->get_size to recompute things. However, nbdkit-plugin.pod
already documents that .get_size and other option negotiation
callbacks may return different values on a per-connection basis, but
that within a connection those results should be constant because they
may be cached.  And our lock does not prevent the following race:

thread A      thread B
.prepare
 - lock
 - next_ops->get_size returns A
 - set real_size based on A
 - unlock
.pread
             .prepare
               - lock
               - next_ops->get_size returns B
               - set real_size based on B
               - unlock
 - lock
 - set real_size_copy based on B
 - unlock
 - decide whether to call next_ops->pread using wrong real_size

If we are worried that next_ops->get_size() can change (even if our
current documentation says it should not), then we must call it every
time, rather than relying on an older cached version of the size;
conversely, if we are trying to cache things, then we are better off
caching it in a per-connection handle instead of globally between
connections. Until the NBD protocol provides a way to advertise new
sizes to clients, then our per-connection handle is effectively
readonly after .prepare, so we don't even have to worry about locks
(and if we DO want correct locking, it would be better to have a
rwlock with get_size holding write and all other ops holding a read
for the duration of the call, rather than just for the moment it
copies from a global variable).

The next commit abuses the file plugin to demonstrate the above race
where a global size cache changed by connection B can affect the
behavior seen by connection A.

Signed-off-by: Eric Blake <eblake@redhat.com>
filters/truncate/truncate.c