Skip to content

Fix memory safety vulnerabilities in high-level and VFD code #6140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
brtnfld wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

Fix memory safety vulnerabilities in high-level and VFD code #6140

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7b22833
Fix memory safety vulnerabilities in high-level and VFD code
brtnfld Jan 2, 2026
e450bd6
made HLTB_MAX_FIELD_LEN public
brtnfld Jan 2, 2026
2b42b81
Merge branch 'develop' into snyk
brtnfld Feb 13, 2026
d3f9058
Fix backward compatibility issue in H5TBget_field_info
brtnfld Feb 13, 2026
187b77a
Clarify API contract and defensive checks in realloc_and_append
brtnfld Feb 13, 2026
a5298fd
Remove unnecessary historical comment from H5TBprivate.h
brtnfld Feb 13, 2026
31edfe0
Refactor defensive checks and add boundary tests for high-level table…
brtnfld Feb 13, 2026
14b3a69
Add CHANGELOG entries for security fixes and improvements
brtnfld Feb 13, 2026
1ce73d4
Committing clang-format changes
github-actions[bot] Feb 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion hl/src/H5DS.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2292,6 +2292,7 @@
is_ds = 1;

free(buf);
buf = NULL;

if (H5Tclose(tid) < 0)
goto out;
Expand All @@ -2301,7 +2302,8 @@
}
out:
if (is_ds < 0) {
free(buf);
if (buf)
free(buf);
H5E_BEGIN_TRY
{
H5Aclose(aid);
Expand Down
130 changes: 64 additions & 66 deletions hl/src/H5LT.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1889,13 +1889,17 @@ H5LTtext_to_dtype(const char *text, H5LT_lang_t lang_type)

input_len = strlen(text);
myinput = strdup(text);
if (!myinput)
goto out;

if ((type_id = H5LTyyparse()) < 0) {
free(myinput);
myinput = NULL;
goto out;
}

free(myinput);
myinput = NULL;
input_len = 0;

return type_id;
Expand Down Expand Up @@ -1942,6 +1946,9 @@ realloc_and_append(bool _no_user_buf, size_t *len, char *buf, const char *str_to
buf = tmp_realloc;
}

if (!buf)
Copy link
Collaborator

@jhendersonHDF jhendersonHDF Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of the _no_user_buf parameter isn't really obvious, but it seems like this check overlaps with the same check inside that block, which seems like it would imply buf being allowed to be passed in as NULL in the false case. But I'm guessing this check was added due to the strlen(buf) below. This seems like we should determine whether it was ever intended for buf to be allowed as NULL.

Copy link
Collaborator Author

@brtnfld brtnfld Feb 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added comments, side-stepped the question if we should assert/error more clearly.

goto out;

if (str_to_add) {
/* find the size of the buffer to add */
size_str_to_add = strlen(str_to_add);
Expand Down Expand Up @@ -2171,6 +2178,50 @@ H5LTdtype_to_text(hid_t dtype, char *str, H5LT_lang_t lang_type, size_t *len)
return FAIL;
}

/*-------------------------------------------------------------------------
* Function: H5LT_append_dtype_super_text
*
* Purpose: Helper function to get super type text and append it to dt_str.
* This encapsulates the common pattern of: allocate buffer,
* convert dtype to text, append to string, free buffer.
*
* Return: Success: updated dt_str pointer, Failure: NULL
*
*-------------------------------------------------------------------------
*/
static char *
H5LT_append_dtype_super_text(hid_t super, char *dt_str, H5LT_lang_t lang, size_t *slen, bool no_user_buf)
{
size_t super_len;
char *stmp = NULL;

/* Get required buffer size for super type text */
if (H5LTdtype_to_text(super, NULL, lang, &super_len) < 0)
return NULL;

/* Allocate buffer for super type text */
stmp = (char *)calloc(super_len, sizeof(char));
if (!stmp)
return NULL;

/* Convert super type to text */
if (H5LTdtype_to_text(super, stmp, lang, &super_len) < 0) {
free(stmp);
return NULL;
}

/* Append super type text to dt_str */
if (!(dt_str = realloc_and_append(no_user_buf, slen, dt_str, stmp))) {
free(stmp);
return NULL;
}

/* Clean up */
free(stmp);

return dt_str;
}

/*-------------------------------------------------------------------------
* Function: H5LT_dtype_to_text
*
Expand Down Expand Up @@ -2533,8 +2584,7 @@ H5LT_dtype_to_text(hid_t dtype, char *dt_str, H5LT_lang_t lang, size_t *slen, bo
tag = H5Tget_tag(dtype);
if (tag) {
snprintf(tmp_str, TMP_LEN, "OPQ_TAG \"%s\";\n", tag);
if (tag)
H5free_memory(tag);
H5free_memory(tag);
tag = NULL;
}
else
Expand All @@ -2553,33 +2603,19 @@ H5LT_dtype_to_text(hid_t dtype, char *dt_str, H5LT_lang_t lang, size_t *slen, bo
break;
}
case H5T_ENUM: {
hid_t super;
size_t super_len;
char *stmp = NULL;
hid_t super;

/* Print lead-in */
snprintf(dt_str, *slen, "H5T_ENUM {\n");
indent += COL;
if (!(dt_str = indentation(indent + COL, dt_str, no_user_buf, slen)))
goto out;

/* Get super type and append its text representation */
if ((super = H5Tget_super(dtype)) < 0)
goto out;
if (H5LTdtype_to_text(super, NULL, lang, &super_len) < 0)
goto out;
stmp = (char *)calloc(super_len, sizeof(char));
if (H5LTdtype_to_text(super, stmp, lang, &super_len) < 0) {
free(stmp);
goto out;
}
if (!(dt_str = realloc_and_append(no_user_buf, slen, dt_str, stmp))) {
free(stmp);
if (!(dt_str = H5LT_append_dtype_super_text(super, dt_str, lang, slen, no_user_buf)))
goto out;
}

if (stmp)
free(stmp);
stmp = NULL;

snprintf(tmp_str, TMP_LEN, ";\n");
if (!(dt_str = realloc_and_append(no_user_buf, slen, dt_str, tmp_str)))
Expand All @@ -2600,33 +2636,20 @@ H5LT_dtype_to_text(hid_t dtype, char *dt_str, H5LT_lang_t lang, size_t *slen, bo
break;
}
case H5T_VLEN: {
hid_t super;
size_t super_len;
char *stmp = NULL;
hid_t super;

/* Print lead-in */
snprintf(dt_str, *slen, "H5T_VLEN {\n");
indent += COL;
if (!(dt_str = indentation(indent + COL, dt_str, no_user_buf, slen)))
goto out;

/* Get super type and append its text representation */
if ((super = H5Tget_super(dtype)) < 0)
goto out;
if (H5LTdtype_to_text(super, NULL, lang, &super_len) < 0)
goto out;
stmp = (char *)calloc(super_len, sizeof(char));
if (H5LTdtype_to_text(super, stmp, lang, &super_len) < 0) {
free(stmp);
goto out;
}
if (!(dt_str = realloc_and_append(no_user_buf, slen, dt_str, stmp))) {
free(stmp);
if (!(dt_str = H5LT_append_dtype_super_text(super, dt_str, lang, slen, no_user_buf)))
goto out;
}

if (stmp)
free(stmp);
stmp = NULL;
snprintf(tmp_str, TMP_LEN, "\n");
if (!(dt_str = realloc_and_append(no_user_buf, slen, dt_str, tmp_str)))
goto out;
Expand All @@ -2644,8 +2667,6 @@ H5LT_dtype_to_text(hid_t dtype, char *dt_str, H5LT_lang_t lang, size_t *slen, bo
}
case H5T_ARRAY: {
hid_t super;
size_t super_len;
char *stmp = NULL;
hsize_t dims[H5S_MAX_RANK];
int ndims;

Expand All @@ -2671,22 +2692,12 @@ H5LT_dtype_to_text(hid_t dtype, char *dt_str, H5LT_lang_t lang, size_t *slen, bo
if (!(dt_str = realloc_and_append(no_user_buf, slen, dt_str, tmp_str)))
goto out;

/* Get super type and append its text representation */
if ((super = H5Tget_super(dtype)) < 0)
goto out;
if (H5LTdtype_to_text(super, NULL, lang, &super_len) < 0)
goto out;
stmp = (char *)calloc(super_len, sizeof(char));
if (H5LTdtype_to_text(super, stmp, lang, &super_len) < 0) {
free(stmp);
if (!(dt_str = H5LT_append_dtype_super_text(super, dt_str, lang, slen, no_user_buf)))
goto out;
}
if (!(dt_str = realloc_and_append(no_user_buf, slen, dt_str, stmp))) {
free(stmp);
goto out;
}
if (stmp)
free(stmp);
stmp = NULL;

snprintf(tmp_str, TMP_LEN, "\n");
if (!(dt_str = realloc_and_append(no_user_buf, slen, dt_str, tmp_str)))
goto out;
Expand Down Expand Up @@ -2772,33 +2783,20 @@ H5LT_dtype_to_text(hid_t dtype, char *dt_str, H5LT_lang_t lang, size_t *slen, bo
break;
}
case H5T_COMPLEX: {
hid_t super;
size_t super_len;
char *stmp = NULL;
hid_t super;

/* Print lead-in */
snprintf(dt_str, *slen, "H5T_COMPLEX {\n");
indent += COL;
if (!(dt_str = indentation(indent + COL, dt_str, no_user_buf, slen)))
goto out;

/* Get super type and append its text representation */
if ((super = H5Tget_super(dtype)) < 0)
goto out;
if (H5LTdtype_to_text(super, NULL, lang, &super_len) < 0)
goto out;
stmp = (char *)calloc(super_len, sizeof(char));
if (H5LTdtype_to_text(super, stmp, lang, &super_len) < 0) {
free(stmp);
if (!(dt_str = H5LT_append_dtype_super_text(super, dt_str, lang, slen, no_user_buf)))
goto out;
}
if (!(dt_str = realloc_and_append(no_user_buf, slen, dt_str, stmp))) {
free(stmp);
goto out;
}

if (stmp)
free(stmp);
stmp = NULL;
snprintf(tmp_str, TMP_LEN, "\n");
if (!(dt_str = realloc_and_append(no_user_buf, slen, dt_str, tmp_str)))
goto out;
Expand Down
6 changes: 5 additions & 1 deletion hl/src/H5TB.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3025,7 +3025,11 @@ H5TBget_field_info(hid_t loc_id, const char *dset_name, char *field_names[], siz

if (NULL == (member_name = H5Tget_member_name(tid, (unsigned)i)))
goto out;
strcpy(field_names[i], member_name);
/* Use strncpy with size limit to prevent buffer overflow
* HLTB_MAX_FIELD_LEN is the expected buffer size (defined in H5TBprivate.h as 255)
* Callers must allocate at least HLTB_MAX_FIELD_LEN bytes per field name */
strncpy(field_names[i], member_name, HLTB_MAX_FIELD_LEN - 1);
field_names[i][HLTB_MAX_FIELD_LEN - 1] = '\0'; /* Ensure null termination */
H5free_memory(member_name);
} /* end if */

Expand Down
9 changes: 8 additions & 1 deletion hl/src/H5TBpublic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,10 @@ H5HL_DLL herr_t H5TBget_table_info(hid_t loc_id, const char *dset_name, hsize_t
*
* \fg_loc_id
* \param[in] dset_name The name of the dataset to read
* \param[out] field_names An array containing the names of the fields
* \param[out] field_names An array containing the names of the fields.
* Each element must be allocated by the caller to at least
* 255 bytes. Field names longer than 254 characters will be
* truncated. Pass NULL to skip retrieving field names.
* \param[out] field_sizes An array containing the size of the fields
* \param[out] field_offsets An array containing the offsets of the fields
* \param[out] type_size The size of the HDF5 datatype associated
Expand All @@ -472,6 +475,10 @@ H5HL_DLL herr_t H5TBget_table_info(hid_t loc_id, const char *dset_name, hsize_t
* named \p dset_name attached to the object specified
* by the identifier \p loc_id.
*
* \note When retrieving field names, callers must allocate each field_names[i]
* buffer to at least 255 bytes. Names exceeding 254 characters will be
* silently truncated to fit.
*
*/
H5HL_DLL herr_t H5TBget_field_info(hid_t loc_id, const char *dset_name, char *field_names[],
size_t *field_sizes, size_t *field_offsets, size_t *type_size);
Expand Down
10 changes: 5 additions & 5 deletions src/H5FDstdio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,8 +393,8 @@ H5FD_stdio_open(const char *name, unsigned flags, hid_t fapl_id, haddr_t maxaddr

/* Use the value in the property list */
if (H5Pget_file_locking(fapl_id, &unused, &file->ignore_disabled_file_locks) < 0) {
fclose(file->fp);
Copy link
Collaborator

@jhendersonHDF jhendersonHDF Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the issue with these close calls? file->fp should be the same as f at this point

Copy link
Collaborator Author

@brtnfld brtnfld Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Snyk flags it for clarity on resource ownership. The concern is that it's not explicit which pointer "owns" the resource after the assignment. Once you've assigned file->fp = f, the FILE* is conceptually owned by the file structure, and using file->fp in cleanup makes this ownership clear.

free(file);
fclose(f);
H5Epush_ret(__func__, H5E_ERR_CLS, H5E_FILE, H5E_CANTGET,
"unable to get use disabled file locks property", NULL);
}
Expand All @@ -407,23 +407,23 @@ H5FD_stdio_open(const char *name, unsigned flags, hid_t fapl_id, haddr_t maxaddr
file->fd = fileno(file->fp);
#endif /* H5_HAVE_WIN32_API */
if (file->fd < 0) {
fclose(file->fp);
free(file);
fclose(f);
H5Epush_ret(__func__, H5E_ERR_CLS, H5E_FILE, H5E_CANTOPENFILE, "unable to get file descriptor", NULL);
} /* end if */

#ifdef H5_HAVE_WIN32_API
file->hFile = (HANDLE)_get_osfhandle(file->fd);
if (INVALID_HANDLE_VALUE == file->hFile) {
fclose(file->fp);
free(file);
fclose(f);
H5Epush_ret(__func__, H5E_ERR_CLS, H5E_FILE, H5E_CANTOPENFILE, "unable to get Windows file handle",
NULL);
} /* end if */

if (!GetFileInformationByHandle((HANDLE)file->hFile, &fileinfo)) {
fclose(file->fp);
free(file);
fclose(f);
H5Epush_ret(__func__, H5E_ERR_CLS, H5E_FILE, H5E_CANTOPENFILE,
"unable to get Windows file descriptor information", NULL);
} /* end if */
Expand All @@ -433,8 +433,8 @@ H5FD_stdio_open(const char *name, unsigned flags, hid_t fapl_id, haddr_t maxaddr
file->dwVolumeSerialNumber = fileinfo.dwVolumeSerialNumber;
#else /* H5_HAVE_WIN32_API */
if (fstat(file->fd, &sb) < 0) {
fclose(file->fp);
free(file);
fclose(f);
H5Epush_ret(__func__, H5E_ERR_CLS, H5E_FILE, H5E_BADFILE, "unable to fstat file", NULL);
} /* end if */
file->device = sb.st_dev;
Expand Down
4 changes: 4 additions & 0 deletions src/H5VLnative.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,10 @@ H5VL_native_get_file_struct(void *obj, H5I_type_t type, H5F_t **file)

FUNC_ENTER_NOAPI(FAIL)

/* Check arguments */
assert(obj);
assert(file);

*file = NULL;

switch (type) {
Expand Down
Loading