From a1065ca054c4c0da4dca15926dd7ab49bb86092a Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 9 Apr 2015 19:58:51 -0500 Subject: [PATCH] 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. Reviewed-on: http://gerrit.openafs.org/11828 Reviewed-by: Benjamin Kaduk Reviewed-by: Michael Meffie Tested-by: BuildBot Reviewed-by: Chas Williams <3chas3@gmail.com> (cherry picked from commit 3caee7575491c33920b9c84f5dc4098d99373cf6) Change-Id: Ibbecd779050f72e90db991e5a0695c86dadf484b Reviewed-on: https://gerrit.openafs.org/12213 Tested-by: BuildBot Reviewed-by: Mark Vitale Reviewed-by: Michael Meffie Reviewed-by: Stephan Wiesand --- src/afs/afs_dcache.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/afs/afs_dcache.c b/src/afs/afs_dcache.c index c05393c15..7449c5d4b 100644 --- a/src/afs/afs_dcache.c +++ b/src/afs/afs_dcache.c @@ -2207,10 +2207,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 */ @@ -2234,12 +2237,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) -- 2.39.5