afs: Avoid incorrect size when fetching beyond EOF

Currently, afs_GetDCache contains a couple of calculations that look
similar to this:

    if (position + size > file_length) {
        size = file_length - position;
    }
    if (size < 0) {
        size = 0;
    }

Most of the time, this is fine. However, if 'position' is more than
2GiB greater than file_length, 'size' will calculated to be smaller
than -2GiB. Since 'size' in this code is a signed 32-bit integer, this
can cause 'size' to underflow, and result in a value closer to
(positive) 2GiB.

This has two potential effects:

The afs_AdjustSize call in afs_GetDCache will cause the underlying
cache file for this dcache to be very large (if our offset is around
2GiB larger than the file size). This can confuse other parts of the
client, since our cache usage reporting will be incorrect (and can be
even way larger than the max configured cache size).

This will also cause a read request to the fileserver that is larger
than necessary. Although 'size' will be capped at our chunksize, it
should be 0 in this situation, since we know there is no data to
fetch. At worst, this currently can just result in worse performance
in rare situations, but it can also just be very confusing.

Note that an afs_GetDCache request beyond EOF can currently happen in
non-race conditions on at least Solaris when performing a file write.
For example, with a chunksize of 256KiB, something like this will
trigger the overflow in 'size' in most cases:

    $ printf '' > smallfile && printf b | dd of=smallfile bs=1 oseek=2147745793

But there are probably other similar scenarios.

To fix this, just check if our offset is beyond the relevant file
size, and do not depend on 'size' having sane values in edge cases
such as this.

Change-Id: Ie36f66ce11fbee905062b3a787871ec077c15354
Reviewed-on: http://gerrit.openafs.org/11828
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Chas Williams <3chas3@gmail.com>

diff --git a/src/afs/afs_dcache.c b/src/afs/afs_dcache.c
index 1bb3bd3..9ccadae 100644 (file)
--- a/src/afs/afs_dcache.c
+++ b/src/afs/afs_dcache.c
@@ -2232,10 +2232,13 @@ afs_GetDCache(struct vcache *avc, afs_size_t abyte,
                maxGoodLength = avc->f.truncPos;
 
            size = AFS_CHUNKSIZE(abyte);        /* expected max size */
-           if (Position + size > maxGoodLength)
+            if (Position > maxGoodLength) { /* If we're beyond EOF */
+                size = 0;
+           } else if (Position + size > maxGoodLength) {
                size = maxGoodLength - Position;
-           if (size < 0)
-               size = 0;       /* Handle random races */
+            }
+            osi_Assert(size >= 0);
+
            if (size > tdc->f.chunkBytes) {
                /* pre-reserve estimated space for file */
                afs_AdjustSize(tdc, size);      /* changes chunkBytes */
@@ -2259,12 +2262,12 @@ afs_GetDCache(struct vcache *avc, afs_size_t abyte,
                 * avc->f.truncPos to reappear, instead of extending the file
                 * with NUL bytes. */
                size = AFS_CHUNKSIZE(abyte);
-               if (Position + size > avc->f.truncPos) {
+                if (Position > avc->f.truncPos) {
+                    size = 0;
+               } else if (Position + size > avc->f.truncPos) {
                    size = avc->f.truncPos - Position;
                }
-               if (size < 0) {
-                   size = 0;
-               }
+                osi_Assert(size >= 0);
            }
        }
        if (afs_mariner && !tdc->f.chunk)