From a9036005c3916e6fda0296026323f00d043300f8 Mon Sep 17 00:00:00 2001 From: vchoi-hdfgroup <55293060+vchoi-hdfgroup@users.noreply.github.com> Date: Tue, 13 Dec 2022 11:40:59 -0600 Subject: =?UTF-8?q?Fix=20for=20HDFFV-11052:=20h5debug=20fails=20on=20a=20c?= =?UTF-8?q?orrupted=20file=20(h5=5Fnrefs=5FPOC)=E2=80=A6=20(#2291)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix for HDFFV-11052: h5debug fails on a corrupted file (h5_nrefs_POC) producing a core dump. When h5debug closes the corrupted file, the library calls H5F__dest() which performs all the closing operations for the file "f" (H5F_t *) but just keeping note of errors in "ret_value" all the way till the end of the routine. The user-provided corrupted file has an illegal file size causing failure when reading the image during the closing process. At the end of this routine it sets f->shared to NULL and then frees "f". This is done whether there is error or not in "ret_value". Due to the failure in reading the file earlier, the routine then returns error. The error return from H5F__dest() causes the file object "f" not being removed from the ID node table. When the library finally exits, it will try to close the file objects in the table. This causes assert failure when H5F_ID_EXISTS(f) or H5F_NREFS(f). Fix: a) H5F_dest(): free the f only when there is no error in "ret_value" at the end of the routine. b) H5VL__native_file_close(): if f->shared is NULL, free "f"; otherwise, perform closing on "f" as before. c) h5debug.c main(): track error return from H5Fclose(). * Committing clang-format changes Co-authored-by: vchoi Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> --- src/H5Fint.c | 4 +++- src/H5VLnative_file.c | 52 +++++++++++++++++++++++++++--------------------- tools/src/misc/h5debug.c | 8 ++++++-- 3 files changed, 38 insertions(+), 26 deletions(-) diff --git a/src/H5Fint.c b/src/H5Fint.c index 2c1b4b2..7ad35fc 100644 --- a/src/H5Fint.c +++ b/src/H5Fint.c @@ -1615,7 +1615,9 @@ H5F__dest(H5F_t *f, hbool_t flush) if (H5FO_top_dest(f) < 0) HDONE_ERROR(H5E_FILE, H5E_CANTINIT, FAIL, "problems closing file") f->shared = NULL; - f = H5FL_FREE(H5F_t, f); + + if (ret_value >= 0) + f = H5FL_FREE(H5F_t, f); FUNC_LEAVE_NOAPI(ret_value) } /* end H5F__dest() */ diff --git a/src/H5VLnative_file.c b/src/H5VLnative_file.c index 907a12d..f2f0ea7 100644 --- a/src/H5VLnative_file.c +++ b/src/H5VLnative_file.c @@ -753,29 +753,35 @@ H5VL__native_file_close(void *file, hid_t H5_ATTR_UNUSED dxpl_id, void H5_ATTR_U FUNC_ENTER_PACKAGE /* This routine should only be called when a file ID's ref count drops to zero */ - HDassert(H5F_ID_EXISTS(f)); - - /* Flush file if this is the last reference to this id and we have write - * intent, unless it will be flushed by the "shared" file being closed. - * This is only necessary to replicate previous behaviour, and could be - * disabled by an option/property to improve performance. - */ - if ((H5F_NREFS(f) > 1) && (H5F_INTENT(f) & H5F_ACC_RDWR)) { - /* Get the file ID corresponding to the H5F_t struct */ - if (H5I_find_id(f, H5I_FILE, &file_id) < 0 || H5I_INVALID_HID == file_id) - HGOTO_ERROR(H5E_ID, H5E_CANTGET, FAIL, "invalid ID") - - /* Get the number of references outstanding for this file ID */ - if ((nref = H5I_get_ref(file_id, FALSE)) < 0) - HGOTO_ERROR(H5E_ID, H5E_CANTGET, FAIL, "can't get ID ref count") - if (nref == 1) - if (H5F__flush(f) < 0) - HGOTO_ERROR(H5E_FILE, H5E_CANTFLUSH, FAIL, "unable to flush cache") - } /* end if */ - - /* Close the file */ - if (H5F__close(f) < 0) - HGOTO_ERROR(H5E_FILE, H5E_CANTDEC, FAIL, "can't close file") + HDassert(f->shared == NULL || H5F_ID_EXISTS(f)); + + if (f->shared == NULL) + f = H5FL_FREE(H5F_t, f); + + else { + + /* Flush file if this is the last reference to this id and we have write + * intent, unless it will be flushed by the "shared" file being closed. + * This is only necessary to replicate previous behaviour, and could be + * disabled by an option/property to improve performance. + */ + if ((H5F_NREFS(f) > 1) && (H5F_INTENT(f) & H5F_ACC_RDWR)) { + /* Get the file ID corresponding to the H5F_t struct */ + if (H5I_find_id(f, H5I_FILE, &file_id) < 0 || H5I_INVALID_HID == file_id) + HGOTO_ERROR(H5E_ID, H5E_CANTGET, FAIL, "invalid ID") + + /* Get the number of references outstanding for this file ID */ + if ((nref = H5I_get_ref(file_id, FALSE)) < 0) + HGOTO_ERROR(H5E_ID, H5E_CANTGET, FAIL, "can't get ID ref count") + if (nref == 1) + if (H5F__flush(f) < 0) + HGOTO_ERROR(H5E_FILE, H5E_CANTFLUSH, FAIL, "unable to flush cache") + } /* end if */ + + /* Close the file */ + if (H5F__close(f) < 0) + HGOTO_ERROR(H5E_FILE, H5E_CANTDEC, FAIL, "can't close file") + } done: FUNC_LEAVE_NOAPI(ret_value) diff --git a/tools/src/misc/h5debug.c b/tools/src/misc/h5debug.c index 05d37b8..b15ae09 100644 --- a/tools/src/misc/h5debug.c +++ b/tools/src/misc/h5debug.c @@ -815,8 +815,12 @@ main(int argc, char *argv[]) done: if (fapl > 0) H5Pclose(fapl); - if (fid > 0) - H5Fclose(fid); + if (fid > 0) { + if (H5Fclose(fid) < 0) { + HDfprintf(stderr, "Error in closing file!\n"); + exit_value = 1; + } + } /* Pop API context */ if (api_ctx_pushed) -- cgit v0.12