From 050f608de3d1953718456b0c128a8ff806d02ea8 Mon Sep 17 00:00:00 2001 From: Binh-Minh Date: Tue, 12 Mar 2024 03:06:49 -0400 Subject: [PATCH] Fix memory leak The list of cdf structures were not properly de-allocated, causing memory leaks. However, releasing the memory caused a number segfaults in several tests. Re-worked the code that increases the size of the cdf list when application attempts to open more files than the current max number of files that can be opened at the same time. This allowed the library to properly release the allocated memory and eliminated the memory leaks. Verified with valgrind. --- mfhdf/nctest/cdftests.c | 6 +- mfhdf/src/file.c | 207 ++++++++++++++++++++++++---------------- mfhdf/test/tfile.c | 18 ++-- 3 files changed, 143 insertions(+), 88 deletions(-) diff --git a/mfhdf/nctest/cdftests.c b/mfhdf/nctest/cdftests.c index edc8d9822c..52f26cf3ba 100644 --- a/mfhdf/nctest/cdftests.c +++ b/mfhdf/nctest/cdftests.c @@ -30,8 +30,12 @@ * try again with NC_NOCLOBBER mode, check error return * On exit, netcdf files are closed. * Uses: nccreate, ncendef, ncclose, ncopen. + * + * path - name of cdf file to create + * + * NOTE: There is no way to create a new cdf file in hdf4, so this test + * is not reliable where it's counting on the new file to be a cdf file. */ -/* path - name of cdf file to create */ void test_nccreate(char *path) { diff --git a/mfhdf/src/file.c b/mfhdf/src/file.c index 11cde48315..b006aba88d 100644 --- a/mfhdf/src/file.c +++ b/mfhdf/src/file.c @@ -45,15 +45,9 @@ struct rlimit rlim; (((MAX_SYS_OPENFILES - 3) > H4_MAX_AVAIL_OPENFILES) ? H4_MAX_AVAIL_OPENFILES : (MAX_SYS_OPENFILES - 3)) static int _curr_opened = 0; /* the number of files currently opened */ -/* NOTE: _ncdf might have been the number of files currently opened, yet it - is not decremented when ANY file is closed but only when the file that - has the same index as _ncdf-1 is closed. Thus, it indicates the last - index in _cdfs instead of the number of files currently opened. So, I - added _curr_opened to keep track of the number of files currently opened. - QAK suggested to use atom as in other interfaces and that would eliminate - similar issues. - BMR - 11/03/07 */ -static int _ncdf = 0; /* high water mark on open cdf's */ -static NC **_cdfs; +static int _ncdf = 0; /* high water mark on open cdf's */ +static NC **_cdfs = NULL; /* list of pointers to cdf structures */ +static int _cdfs_size = 0; /* size of _cdfs list */ #define HNDLE(id) (((id) >= 0 && (id) < _ncdf) ? _cdfs[(id)] : NULL) #define STASH(id) (((id) >= 0 && (id) < _ncdf) ? HNDLE(_cdfs[(id)]->redefid) : NULL) @@ -70,28 +64,40 @@ static int max_NC_open = H4_MAX_NC_OPEN; /* current netCDF default */ /* * Resets _cdfs */ -static void +static int ncreset_cdflist(void) { + /* Check for non-NULL pointers in the _cdfs list, and if there is + any, we should not deallocate _cdfs */ if (_cdfs != NULL) { + for (int i = 0; i < _cdfs_size; i++) + if (_cdfs[i] != NULL) { + fprintf(stderr, "%d is not NULL\n", i); + return -1; + } + /* Release _cdfs and reset its size */ free(_cdfs); _cdfs = NULL; + _cdfs_size = 0; } + return 0; } /* * Allocates _cdfs and returns the allocated size if succeeds; * otherwise return FAIL(-1). + * + * req_max - new max number of files can be opened */ -/* req_max - requested max to allocate */ int NC_reset_maxopenfiles(int req_max) { int sys_limit = MAX_AVAIL_OPENFILES; int alloc_size; - NC **newlist; + NC **newlist = NULL; int i; - int ret_value = SUCCEED; + int old_idx, new_idx; /* indices for the _cdfs list and the new list */ + int ret_value = 0; /* Verify arguments */ if (req_max < 0) { @@ -99,67 +105,81 @@ NC_reset_maxopenfiles(int req_max) HGOTO_DONE(-1); } - /* If requested max is 0, allocate _cdfs with the default, - max_NC_open, if _cdfs is not yet allocated, otherwise, keep - _cdfs as is and return the current max */ - if (req_max == 0) { - if (!_cdfs) { - _cdfs = malloc(sizeof(NC *) * (max_NC_open)); - - /* If allocation fails, return 0 for no allocation */ - if (_cdfs == NULL) { - /* NC_EINVAL is Invalid Argument, but must decide if - we just want to return 0 without error or not */ - NCadvise(NC_EINVAL, "Unable to allocate a cdf list of %d elements", max_NC_open); - HGOTO_DONE(-1); - } - else - HGOTO_DONE(max_NC_open); + /* If _cdfs is not yet allocated, allocate with the default max or the + requested max */ + if (!_cdfs) { + if (req_max == 0) + _cdfs_size = max_NC_open; + else + _cdfs_size = req_max; + + _cdfs = malloc(sizeof(NC *) * (_cdfs_size)); + + /* If allocation fails, return with failure */ + if (_cdfs == NULL) { + /* NC_EINVAL is Invalid Argument, because netCDF 2.x didn't have + error code for bad allocation */ + NCadvise(NC_EINVAL, "Unable to allocate a cdf list of %d elements", _cdfs_size); + HGOTO_DONE(-1); } - else /* return the current limit */ + else { + for (i = 0; i < _cdfs_size; i++) + _cdfs[i] = NULL; + + /* Reset current max files opened allowed in HDF to the new max */ + max_NC_open = _cdfs_size; + HGOTO_DONE(max_NC_open); - } /* if req_max == 0 */ + } + } - /* If the requested max is less than the current max but there are - more than the requested max number of files opened, do not reset - the current max, since this will cause information lost. */ - if (req_max < max_NC_open && req_max <= _ncdf) - HGOTO_DONE(max_NC_open); + /* If the requested max is less than the number of files opened, do + not reset the current max, since this will cause information lost. */ + if (req_max <= _curr_opened) { + NCadvise(NC_EINVAL, "Request max %d must be greater than current number of opened files %d. Keep current size.", req_max, _curr_opened); + HGOTO_DONE(_cdfs_size); + } - /* If the requested max exceeds system limit, only allocate up - to system limit */ + /* If the requested max exceeds system limit, only allocate up to system limit */ if (req_max > sys_limit) alloc_size = sys_limit; else + /* The requested max can be less than the current max */ alloc_size = req_max; /* Allocate a new list */ newlist = malloc(sizeof(NC *) * alloc_size); - /* If allocation fails, return 0 for no allocation */ + /* If allocation fails, return with failure */ if (newlist == NULL) { - /* NC_EINVAL is Invalid Argument, but must decide if - we just want to return 0 without error or not */ - NCadvise(NC_EINVAL, "Unable to allocate a cdf list of %d elements", alloc_size); - HGOTO_DONE(-1); - } + /* NC_EINVAL is Invalid Argument, because netCDF 2.x didn't have + error code for bad allocation */ + NCadvise(NC_EINVAL, "Unable to allocate a cdf list of %d elements", alloc_size); + HGOTO_DONE(-1); + } - /* If _cdfs is already allocated, transfer pointers over to the - new list and deallocate the old list of pointers */ - if (_cdfs != NULL) { - for (i = 0; i < _ncdf; i++) - newlist[i] = _cdfs[i]; - free(_cdfs); - } + /* Ininitialize all NC pointers */ + for (i = 0; i < alloc_size; i++) + newlist[i] = NULL; + + /* Transfer all non-NULL pointers over to the new list and deallocate the + old list of pointers */ + for (old_idx = 0, new_idx = 0; old_idx < _cdfs_size && new_idx < alloc_size; old_idx++) + if (_cdfs[old_idx] != NULL) + newlist[new_idx++] = _cdfs[old_idx]; + free(_cdfs); /* Set _cdfs to the new list */ _cdfs = newlist; newlist = NULL; + /* Update the size of _cdfs list */ + _cdfs_size = alloc_size; + /* Reset current max files opened allowed in HDF to the new max */ max_NC_open = alloc_size; - HGOTO_DONE(max_NC_open); + return max_NC_open; done: return ret_value; @@ -171,7 +191,10 @@ NC_reset_maxopenfiles(int req_max) int NC_get_maxopenfiles(void) { - return max_NC_open; + if (_cdfs_size == 0) + return max_NC_open; + else + return _cdfs_size; } /* NC_get_maxopenfiles */ /* @@ -236,35 +259,40 @@ NC_open(const char *path, int mode) { NC *handle = NULL; int cdfid = -1; - int cdfs_size = -1; - /* Allocate _cdfs, if it is already allocated, nothing will be done */ + /* Allocate _cdfs, if it has not been */ if (_cdfs == NULL) { - if (FAIL == (cdfs_size = NC_reset_maxopenfiles(0))) { + if (FAIL == (_cdfs_size = NC_reset_maxopenfiles(0))) { NCadvise(NC_ENFILE, "Could not reset max open files limit"); return -1; } + cdfid = 0; } - /* find first available id */ - for (cdfid = 0; cdfid < _ncdf; cdfid++) - if (_cdfs[cdfid] == NULL) - break; - - /* if application attempts to open more files than the current max - allows, increase the current max to the system limit, if it's - not at the system limit yet */ - if (cdfid == _ncdf && _ncdf >= max_NC_open) { - /* if the current max already reaches the system limit, fail */ - if (max_NC_open == MAX_AVAIL_OPENFILES) { - NCadvise(NC_ENFILE, "maximum number of open cdfs allowed already reaches system limit %d", - MAX_AVAIL_OPENFILES); - return -1; - } - /* otherwise, increase the current max to the system limit */ - if (FAIL == NC_reset_maxopenfiles(MAX_AVAIL_OPENFILES)) { - NCadvise(NC_ENFILE, "Could not reset max open files limit"); - return -1; + /* _cdfs is already allocated */ + else { + /* find first available id */ + for (cdfid = 0; cdfid < _ncdf; cdfid++) + if (_cdfs[cdfid] == NULL) + break; + + /* if application attempts to open more files than the current max + allows, increase the current max to the system limit, if it's + not at the system limit yet */ + /* if (cdfid == _ncdf && _ncdf >= max_NC_open) { + */ + if (cdfid == _cdfs_size && _ncdf >= max_NC_open) { + /* if the current max already reaches the system limit, fail */ + if (max_NC_open == MAX_AVAIL_OPENFILES) { + NCadvise(NC_ENFILE, "maximum number of open cdfs allowed already reaches system limit %d", + MAX_AVAIL_OPENFILES); + return -1; + } + /* otherwise, increase the current max to the system limit */ + if (FAIL == NC_reset_maxopenfiles(MAX_AVAIL_OPENFILES)) { + NCadvise(NC_ENFILE, "Could not reset max open files limit"); + return -1; + } } } @@ -393,11 +421,16 @@ ncabort(int cdfid) flags = handle->flags; /* need to save past free_cdf */ + /* if (handle->file_type != HDF_FILE) { + */ + /* NC_CREAT implies NC_INDEF, in both cases need to remove handle->path */ if (flags & (NC_INDEF | NC_CREAT)) { (void)strncpy(path, handle->path, FILENAME_MAX); /* stash path */ if (!(flags & NC_CREAT)) /* redef */ { +if (handle->file_type != HDF_FILE) { + NC_free_cdf(STASH(cdfid)); _cdfs[handle->redefid] = NULL; @@ -408,7 +441,11 @@ ncabort(int cdfid) /* if the _cdf list is empty, deallocate and reset it to NULL */ if (_ncdf == 0) - ncreset_cdflist(); + if (ncreset_cdflist() == -1) { + fprintf(stderr, "unable to reset _cdfs list\n"); + return -1; + } + } } } else if (handle->flags & NC_RDWR) { @@ -422,6 +459,7 @@ ncabort(int cdfid) return -1; } } +/* } file type is HDF */ file_type = handle->file_type; NC_free_cdf(handle); /* calls fclose */ @@ -450,7 +488,10 @@ ncabort(int cdfid) /* if the _cdf list is empty, deallocate and reset it to NULL */ if (_ncdf == 0) - ncreset_cdflist(); + if (ncreset_cdflist() == -1) { + fprintf(stderr, "unable to reset _cdfs list\n"); + return -1; + } return 0; } /* ncabort */ @@ -832,7 +873,10 @@ NC_endef(int cdfid, NC *handle) /* if the _cdf list is empty, deallocate and reset it to NULL */ if (_ncdf == 0) - ncreset_cdflist(); + if (ncreset_cdflist() == -1) { + fprintf(stderr, "unable to reset _cdfs list\n"); + return -1; + } return -1; } @@ -916,8 +960,11 @@ ncclose(int cdfid) _curr_opened--; /* one less file currently opened */ /* if the _cdf list is empty, deallocate and reset it to NULL */ - if (_ncdf == 0) - ncreset_cdflist(); + if (_curr_opened == 0) + if (ncreset_cdflist() == -1) { + fprintf(stderr, "unable to reset _cdfs list\n"); + return -1; + } return 0; } diff --git a/mfhdf/test/tfile.c b/mfhdf/test/tfile.c index 77639ca5d8..5abea4fc4c 100644 --- a/mfhdf/test/tfile.c +++ b/mfhdf/test/tfile.c @@ -196,7 +196,7 @@ test_max_open_files() curr_max = SDreset_maxopenfiles(33); VERIFY(curr_max, 33, "test_maxopenfiles: SDreset_maxopenfiles"); - /* Try to create more files than the default max (currently, 32) and + /* Try to create more files than the default max (currently, 33) and all should succeed */ for (index = 0; index < NUM_FILES_LOW; index++) { /* Create a file */ @@ -204,6 +204,10 @@ test_max_open_files() fids[index] = SDstart(filename[index], DFACC_CREATE); CHECK(fids[index], FAIL, "test_maxopenfiles: SDstart"); } + /* Get the current max and system limit, the current max should now be at system limit */ + status = SDget_maxopenfiles(&curr_max, &sys_limit); + CHECK(status, FAIL, "test_maxopenfiles: SDget_maxopenfiles"); + VERIFY(curr_max, sys_limit, "test_maxopenfiles: SDget_maxopenfiles"); /* Verify that NUM_FILES_LOW files are opened */ curr_opened = SDget_numopenfiles(); @@ -219,11 +223,6 @@ test_max_open_files() curr_opened = SDget_numopenfiles(); VERIFY(curr_opened, NUM_FILES_LOW - 3, "test_maxopenfiles: SDget_numopenfiles"); - /* Get the current max and system limit */ - status = SDget_maxopenfiles(&curr_max, &sys_limit); - CHECK(status, FAIL, "test_maxopenfiles: SDget_maxopenfiles"); - VERIFY(curr_max, sys_limit, "test_maxopenfiles: SDreset_maxopenfiles"); - /* Get the current max another way, it should be the system limit */ curr_max = SDreset_maxopenfiles(0); VERIFY(curr_max, sys_limit, "test_maxopenfiles: SDreset_maxopenfiles"); @@ -241,7 +240,7 @@ test_max_open_files() /* Reset current max to a value that is smaller than the current number of opened files; it shouldn't reset */ curr_max_bk = curr_max; - curr_max = SDreset_maxopenfiles(curr_opened - 1); + curr_max = SDreset_maxopenfiles(curr_opened - 4); VERIFY(curr_max, curr_max_bk, "test_maxopenfiles: SDreset_maxopenfiles"); /* Reset current max again to a value that is smaller than the @@ -475,10 +474,15 @@ extern int test_files() { int num_errs = 0; /* number of errors */ + int curr_max; /* current # of open files allowed */ + int sys_limit; /* max # of open files allowed on a system */ /* Output message about test being performed */ TESTING("miscellaneous file related functions (tfile.c)"); + /* Get the current limits */ + SDget_maxopenfiles(&curr_max, &sys_limit); + /* Test that an in-use file is not removed in certain failure cleanup. */ num_errs = num_errs + test_file_inuse();