Skip to content

Commit

Permalink
Fix security issue from malformed data
Browse files Browse the repository at this point in the history
This PR
- fixes an abort of the user's program (issue HDFGroupGH-4435) because the
  malformed data caused a very large size of memory to be allocated.
- removes memory leaks causing the free-list code to abort in addition
  to the reported issue.

NF provided helpful code to the fix.
  • Loading branch information
bmribler committed Sep 18, 2024
1 parent f54abf7 commit b3329e3
Show file tree
Hide file tree
Showing 7 changed files with 20 additions and 13 deletions.
5 changes: 4 additions & 1 deletion src/H5Centry.c
Original file line number Diff line number Diff line change
Expand Up @@ -594,8 +594,11 @@ H5C__flush_single_entry(H5F_t *f, H5C_cache_entry_t *entry_ptr, unsigned flags)
else
mem_type = entry_ptr->type->mem_type;

if (H5F_block_write(f, mem_type, entry_ptr->addr, entry_ptr->size, entry_ptr->image_ptr) < 0)
if (H5F_block_write(f, mem_type, entry_ptr->addr, entry_ptr->size, entry_ptr->image_ptr) < 0) {
H5C__REMOVE_ENTRY_FROM_SLIST(cache_ptr, entry_ptr, during_flush, FAIL);

HGOTO_ERROR(H5E_CACHE, H5E_CANTFLUSH, FAIL, "Can't write image to file");
}
#ifdef H5_HAVE_PARALLEL
}
#endif /* H5_HAVE_PARALLEL */
Expand Down
2 changes: 1 addition & 1 deletion src/H5Epublic.h
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ typedef herr_t (*H5E_auto2_t)(hid_t estack, void *client_data);
* \param[in] lib_name Name of the client library or application to which the error class belongs
* \param[in] version Version of the client library or application to which the
error class belongs. It can be \c NULL.
* \return Returns a class identifier on success; otherwise returns H5I_INVALID_ID.
* \return Returns a class identifier on success; otherwise returns H5I_INVALID_HID.
*
* \details H5Eregister_class() registers a client library or application
* program to the HDF5 error API so that the client library or
Expand Down
13 changes: 8 additions & 5 deletions src/H5Faccum.c
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,7 @@ H5F__accum_write(H5F_shared_t *f_sh, H5FD_mem_t map_type, haddr_t addr, size_t s
/* Make certain that data in accumulator is visible before new write */
if ((H5F_SHARED_INTENT(f_sh) & H5F_ACC_SWMR_WRITE) > 0)
/* Flush if dirty and reset accumulator */
if (H5F__accum_reset(f_sh, true) < 0)
if (H5F__accum_reset(f_sh, true, false) < 0)
HGOTO_ERROR(H5E_IO, H5E_CANTRESET, FAIL, "can't reset accumulator");

/* Write the data */
Expand Down Expand Up @@ -776,7 +776,7 @@ H5F__accum_write(H5F_shared_t *f_sh, H5FD_mem_t map_type, haddr_t addr, size_t s
} /* end if */
else { /* Access covers whole accumulator */
/* Reset accumulator, but don't flush */
if (H5F__accum_reset(f_sh, false) < 0)
if (H5F__accum_reset(f_sh, false, false) < 0)
HGOTO_ERROR(H5E_IO, H5E_CANTRESET, FAIL, "can't reset accumulator");
} /* end else */
} /* end if */
Expand Down Expand Up @@ -1039,7 +1039,7 @@ H5F__accum_flush(H5F_shared_t *f_sh)
*-------------------------------------------------------------------------
*/
herr_t
H5F__accum_reset(H5F_shared_t *f_sh, bool flush)
H5F__accum_reset(H5F_shared_t *f_sh, bool flush, bool force)
{
herr_t ret_value = SUCCEED; /* Return value */

Expand All @@ -1050,8 +1050,11 @@ H5F__accum_reset(H5F_shared_t *f_sh, bool flush)

/* Flush any dirty data in accumulator, if requested */
if (flush)
if (H5F__accum_flush(f_sh) < 0)
HGOTO_ERROR(H5E_FILE, H5E_CANTFLUSH, FAIL, "can't flush metadata accumulator");
if (H5F__accum_flush(f_sh) < 0) {
HDONE_ERROR(H5E_FILE, H5E_CANTFLUSH, FAIL, "can't flush metadata accumulator");
if (!force)
HGOTO_DONE(FAIL);
}

/* Check if we need to reset the metadata accumulator information */
if (f_sh->feature_flags & H5FD_FEAT_ACCUMULATE_METADATA) {
Expand Down
7 changes: 4 additions & 3 deletions src/H5Fint.c
Original file line number Diff line number Diff line change
Expand Up @@ -1152,7 +1152,8 @@ H5F__new(H5F_shared_t *shared, unsigned flags, hid_t fcpl_id, hid_t fapl_id, H5F
*/
if (NULL == (plist = (H5P_genplist_t *)H5I_object(fcpl_id)))
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, NULL, "not property list");
f->shared->fcpl_id = H5P_copy_plist(plist, false);
if ((f->shared->fcpl_id = H5P_copy_plist(plist, false)) < 0)
HGOTO_ERROR(H5E_PLIST, H5E_CANTCOPY, NULL, "can't copy property list");

/* Get the FCPL values to cache */
if (H5P_get(plist, H5F_CRT_ADDR_BYTE_NUM_NAME, &f->shared->sizeof_addr) < 0)
Expand Down Expand Up @@ -1559,7 +1560,7 @@ H5F__dest(H5F_t *f, bool flush, bool free_on_failure)
} /* end if */

/* Destroy other components of the file */
if (H5F__accum_reset(f->shared, true) < 0)
if (H5F__accum_reset(f->shared, true, true) < 0)
/* Push error, but keep going*/
HDONE_ERROR(H5E_FILE, H5E_CANTRELEASE, FAIL, "problems closing file");
if (H5FO_dest(f) < 0)
Expand Down Expand Up @@ -3893,7 +3894,7 @@ H5F__start_swmr_write(H5F_t *f)
} /* end if */

/* Flush and reset the accumulator */
if (H5F__accum_reset(f->shared, true) < 0)
if (H5F__accum_reset(f->shared, true, false) < 0)
HGOTO_ERROR(H5E_IO, H5E_CANTRESET, FAIL, "can't reset accumulator");

/* Turn on SWMR write in shared file open flags */
Expand Down
2 changes: 1 addition & 1 deletion src/H5Fio.c
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ H5F_flush_tagged_metadata(H5F_t *f, haddr_t tag)
HGOTO_ERROR(H5E_CACHE, H5E_CANTFLUSH, FAIL, "unable to flush tagged metadata");

/* Flush and reset the accumulator */
if (H5F__accum_reset(f->shared, true) < 0)
if (H5F__accum_reset(f->shared, true, false) < 0)
HGOTO_ERROR(H5E_IO, H5E_CANTRESET, FAIL, "can't reset accumulator");

/* Flush file buffers to disk. */
Expand Down
2 changes: 1 addition & 1 deletion src/H5Fpkg.h
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ H5_DLL herr_t H5F__accum_write(H5F_shared_t *f_sh, H5FD_mem_t type, haddr_t addr
const void *buf);
H5_DLL herr_t H5F__accum_free(H5F_shared_t *f, H5FD_mem_t type, haddr_t addr, hsize_t size);
H5_DLL herr_t H5F__accum_flush(H5F_shared_t *f_sh);
H5_DLL herr_t H5F__accum_reset(H5F_shared_t *f_sh, bool flush);
H5_DLL herr_t H5F__accum_reset(H5F_shared_t *f_sh, bool flush, bool force);

/* Shared file list related routines */
H5_DLL herr_t H5F__sfile_add(H5F_shared_t *shared);
Expand Down
2 changes: 1 addition & 1 deletion test/accum.c
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ void accum_printf(const H5F_t *f);
#define accum_read(a, s, b) H5F_block_read(f, H5FD_MEM_DEFAULT, (haddr_t)(a), (size_t)(s), (b))
#define accum_free(f, a, s) H5F__accum_free(f->shared, H5FD_MEM_DEFAULT, (haddr_t)(a), (hsize_t)(s))
#define accum_flush(f) H5F__accum_flush(f->shared)
#define accum_reset(f) H5F__accum_reset(f->shared, true)
#define accum_reset(f) H5F__accum_reset(f->shared, true, false)

/* ================= */
/* Main Test Routine */
Expand Down

0 comments on commit b3329e3

Please sign in to comment.