From d2d27f975df13c3833898611dacff940a5ba3e2a Mon Sep 17 00:00:00 2001 From: Cheyenne Wills Date: Fri, 19 Jun 2020 08:01:14 -0600 Subject: [PATCH 1/1] afs: Avoid panics on failed return from afs_CFileOpen afs_CFileOpen is a macro that invokes the open "method" of the afs_cacheOps structure, and for disk caches the osi_UFSOpen function is used. Currently osi_UFSOpen will panic if there is an error encountered while opening a file. Prepare to handle osi_UFSOpen function returning a NULL instead of issuing a panic (future commit). Update callers of afs_CFileOpen to test for an error and to return an error instead of issuing a panic. While this commit eliminates some panics, it does not address some of the more complex cases associated with errors from afs_CFileOpen. Change-Id: I2bdd525633dd44ebf8e26fcfd7059dfdfffb6142 Reviewed-on: https://gerrit.openafs.org/14241 Reviewed-by: Andrew Deason Reviewed-by: Mark Vitale Tested-by: BuildBot Reviewed-by: Benjamin Kaduk --- src/afs/VNOPS/afs_vnop_symlink.c | 11 ++++++++--- src/afs/afs_buffer.c | 33 +++++++++++++++++++-------------- src/afs/afs_dcache.c | 26 ++++++++++++++++++-------- src/afs/afs_disconnected.c | 7 ++++++- src/afs/afs_fetchstore.c | 4 +++- src/afs/afs_init.c | 4 +++- src/afs/afs_segments.c | 7 ++++++- 7 files changed, 63 insertions(+), 29 deletions(-) diff --git a/src/afs/VNOPS/afs_vnop_symlink.c b/src/afs/VNOPS/afs_vnop_symlink.c index 02ba3b4..e88e09e 100644 --- a/src/afs/VNOPS/afs_vnop_symlink.c +++ b/src/afs/VNOPS/afs_vnop_symlink.c @@ -43,6 +43,7 @@ afs_DisconCreateSymlink(struct vcache *avc, char *aname, struct dcache *tdc; struct osi_file *tfile; afs_size_t offset, len; + int code = 0; tdc = afs_GetDCache(avc, 0, areq, &offset, &len, 0); if (!tdc) { @@ -54,14 +55,18 @@ afs_DisconCreateSymlink(struct vcache *avc, char *aname, avc->f.m.Length = len; ObtainWriteLock(&tdc->lock, 720); + tfile = afs_CFileOpen(&tdc->f.inode); + if (!tfile) { + code = EIO; + goto done; + } afs_AdjustSize(tdc, len); tdc->validPos = len; - tfile = afs_CFileOpen(&tdc->f.inode); - osi_Assert(tfile); afs_CFileWrite(tfile, 0, aname, len); afs_CFileClose(tfile); + done: ReleaseWriteLock(&tdc->lock); - return 0; + return code; } /* don't set CDirty in here because RPC is called synchronously */ diff --git a/src/afs/afs_buffer.c b/src/afs/afs_buffer.c index 8bccee4..1f2a470 100644 --- a/src/afs/afs_buffer.c +++ b/src/afs/afs_buffer.c @@ -236,28 +236,24 @@ DRead(struct dcache *adc, int page, struct DirBuffer *entry) /* The directory blob is empty, apparently. This is not a valid dir * blob, so throw an error. */ code = EIO; + goto error; } else if (page * AFS_BUFFER_PAGESIZE >= adc->f.chunkBytes) { code = ENOENT; /* past the end */ + goto error; } - if (code) { - tb->fid = NULLIDX; - afs_reset_inode(&tb->inode); - tb->lockers--; - ReleaseWriteLock(&tb->lock); - return code; - } + tfile = afs_CFileOpen(&adc->f.inode); - osi_Assert(tfile); + if (!tfile) { + code = EIO; + goto error; + } code = afs_CFileRead(tfile, tb->page * AFS_BUFFER_PAGESIZE, tb->data, AFS_BUFFER_PAGESIZE); afs_CFileClose(tfile); if (code < AFS_BUFFER_PAGESIZE) { - tb->fid = NULLIDX; - afs_reset_inode(&tb->inode); - tb->lockers--; - ReleaseWriteLock(&tb->lock); - return EIO; + code = EIO; + goto error; } /* Note that findslot sets the page field in the buffer equal to * what it is searching for. */ @@ -265,6 +261,13 @@ DRead(struct dcache *adc, int page, struct DirBuffer *entry) entry->buffer = tb; entry->data = tb->data; return 0; + + error: + tb->fid = NULLIDX; + afs_reset_inode(&tb->inode); + tb->lockers--; + ReleaseWriteLock(&tb->lock); + return code; } static void @@ -381,7 +384,9 @@ afs_newslot(struct dcache *adc, afs_int32 apage, struct buffer *lp) if (lp->dirty) { /* see DFlush for rationale for not getting and locking the dcache */ tfile = afs_CFileOpen(&lp->inode); - osi_Assert(tfile); + if (!tfile) + return NULL; /* Callers will flag as EIO */ + afs_CFileWrite(tfile, lp->page * AFS_BUFFER_PAGESIZE, lp->data, AFS_BUFFER_PAGESIZE); lp->dirty = 0; diff --git a/src/afs/afs_dcache.c b/src/afs/afs_dcache.c index 8d97143..d39350c 100644 --- a/src/afs/afs_dcache.c +++ b/src/afs/afs_dcache.c @@ -3810,9 +3810,9 @@ afs_MakeShadowDir(struct vcache *avc, struct dcache *adc) { int i, code, ret_code = 0, written, trans_size; struct dcache *new_dc = NULL; - struct osi_file *tfile_src, *tfile_dst; + struct osi_file *tfile_src = NULL, *tfile_dst = NULL; struct VenusFid shadow_fid; - char *data; + char *data = NULL; /* Is this a dir? */ if (vType(avc) != VDIR) @@ -3867,9 +3867,16 @@ afs_MakeShadowDir(struct vcache *avc, struct dcache *adc) /* Open the files. */ tfile_src = afs_CFileOpen(&adc->f.inode); + if (!tfile_src) { + ret_code = EIO; + goto done; + } + tfile_dst = afs_CFileOpen(&new_dc->f.inode); - osi_Assert(tfile_src); - osi_Assert(tfile_dst); + if (!tfile_dst) { + ret_code = EIO; + goto done; + } /* And now copy dir dcache data into this dcache, * 4k at a time. @@ -3897,10 +3904,14 @@ afs_MakeShadowDir(struct vcache *avc, struct dcache *adc) written+=trans_size; } - afs_CFileClose(tfile_dst); - afs_CFileClose(tfile_src); + done: + if (tfile_dst) + afs_CFileClose(tfile_dst); + if (tfile_src) + afs_CFileClose(tfile_src); - afs_osi_Free(data, 4096); + if (data) + afs_osi_Free(data, 4096); ReleaseWriteLock(&new_dc->lock); afs_PutDCache(new_dc); @@ -3917,7 +3928,6 @@ afs_MakeShadowDir(struct vcache *avc, struct dcache *adc) avc->f.shadow.unique = shadow_fid.Fid.Unique; } -done: return ret_code; } diff --git a/src/afs/afs_disconnected.c b/src/afs/afs_disconnected.c index ee79d31..3b88585 100644 --- a/src/afs/afs_disconnected.c +++ b/src/afs/afs_disconnected.c @@ -709,7 +709,12 @@ afs_ProcessOpCreate(struct vcache *avc, struct vrequest *areq, } ObtainReadLock(&tdc->lock); tfile = afs_CFileOpen(&tdc->f.inode); - osi_Assert(tfile); + if (!tfile) { + ReleaseReadLock(&tdc->lock); + afs_PutDCache(tdc); + code = EIO; + goto end; + } code = afs_CFileRead(tfile, 0, ttargetName, tlen); ttargetName[tlen-1] = '\0'; afs_CFileClose(tfile); diff --git a/src/afs/afs_fetchstore.c b/src/afs/afs_fetchstore.c index 06325b9..e2b3f2b 100644 --- a/src/afs/afs_fetchstore.c +++ b/src/afs/afs_fetchstore.c @@ -270,7 +270,9 @@ afs_GenericStoreProc(struct storeOps *ops, void *rock, size = tdc->f.chunkBytes; tfile = afs_CFileOpen(&tdc->f.inode); - osi_Assert(tfile); + if (!tfile) { + return EIO; + } while ( size > 0 ) { code = (*ops->prepare)(rock, size, &tlen); diff --git a/src/afs/afs_init.c b/src/afs/afs_init.c index f2414f7..5d72319 100644 --- a/src/afs/afs_init.c +++ b/src/afs/afs_init.c @@ -334,7 +334,9 @@ afs_InitVolumeInfo(char *afile) if (code) return code; tfile = afs_CFileOpen(&volumeInode); - osi_Assert(tfile); + if (!tfile) { + return EIO; + } afs_CFileTruncate(tfile, 0); afs_CFileClose(tfile); return 0; diff --git a/src/afs/afs_segments.c b/src/afs/afs_segments.c index b1de015..91e6f28 100644 --- a/src/afs/afs_segments.c +++ b/src/afs/afs_segments.c @@ -727,7 +727,12 @@ afs_ExtendSegments(struct vcache *avc, afs_size_t alen, struct vrequest *areq) toAdd = AFS_CHUNKTOSIZE(tdc->f.chunk) - offset; } tfile = afs_CFileOpen(&tdc->f.inode); - osi_Assert(tfile); + if (!tfile) { + ReleaseWriteLock(&tdc->lock); + afs_PutDCache(tdc); + code = EIO; + break; + } while(tdc->validPos < avc->f.m.Length + toAdd) { afs_size_t towrite; -- 1.9.4