-
-
Notifications
You must be signed in to change notification settings - Fork 265
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.
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
Fix several warnings #720
Fix several warnings #720
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -129,7 +129,7 @@ H5E__get_msg(const H5E_msg_t *msg, H5E_type_t *type, char *msg_str, size_t size) | |
|
||
/* Copy the message into the user's buffer, if given */ | ||
if (msg_str) { | ||
HDstrncpy(msg_str, msg->msg, MIN((size_t)(len + 1), size)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above |
||
HDstrncpy(msg_str, msg->msg, size); | ||
if ((size_t)len >= size) | ||
msg_str[size - 1] = '\0'; | ||
} /* end if */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -646,7 +646,7 @@ H5FD_multi_sb_encode(H5FD_t *_file, char *name /*out*/, unsigned char *buf /*out | |
H5Eclear2(H5E_DEFAULT); | ||
|
||
/* Name and version number */ | ||
strncpy(name, "NCSAmulti", (size_t)8); | ||
strncpy(name, "NCSAmult", (size_t)9); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously was trying to copy 8 bytes out of a 10 byte (including NUL terminator) string? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope - there's only room for 8 bytes in the "Driver ID" field for the file format: https://portal.hdfgroup.org/display/HDF5/File+Format+Specification There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand that. However,
|
||
name[8] = '\0'; | ||
|
||
assert(7 == H5FD_MEM_NTYPES); | ||
|
@@ -682,7 +682,7 @@ H5FD_multi_sb_encode(H5FD_t *_file, char *name /*out*/, unsigned char *buf /*out | |
p = buf + 8 + nseen * 2 * 8; | ||
UNIQUE_MEMBERS (file->fa.memb_map, mt) { | ||
size_t n = strlen(file->fa.memb_name[mt]) + 1; | ||
strncpy((char *)p, file->fa.memb_name[mt], n); | ||
strcpy((char *)p, file->fa.memb_name[mt]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using strcpy here is no less safe than strncpy. We don't know the total size of the output buffer (the API doesn't give us that info), and using strncpy with a size based on the source length will throw a warning anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regardless which one, shouldn't it be HD...? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, sure, that's a good point. Let me fix all the usages that were never converted over. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It turns out that the multi driver uses only public HDF5 headers, so it doesn't use the HD-prefixed versions of standard library/system calls. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The stdio VFD is the same way. They were intended as "demo" VFDs to show people how to write them using the public API (multi = multi-file VFD example, stdio = single-file VFD example). |
||
p += n; | ||
for (i = n; i % 8; i++) | ||
*p++ = '\0'; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -340,10 +340,12 @@ H5Pset_fapl_splitter(hid_t fapl_id, H5FD_splitter_vfd_config_t *vfd_config) | |
HGOTO_ERROR(H5E_VFL, H5E_CANTALLOC, FAIL, "unable to allocate file access property list struct") | ||
|
||
info->ignore_wo_errs = vfd_config->ignore_wo_errs; | ||
HDstrncpy(info->wo_path, vfd_config->wo_path, H5FD_SPLITTER_PATH_MAX); | ||
HDstrncpy(info->log_file_path, vfd_config->log_file_path, H5FD_SPLITTER_PATH_MAX); | ||
info->rw_fapl_id = H5P_FILE_ACCESS_DEFAULT; /* pre-set value */ | ||
info->wo_fapl_id = H5P_FILE_ACCESS_DEFAULT; /* pre-set value */ | ||
HDstrncpy(info->wo_path, vfd_config->wo_path, H5FD_SPLITTER_PATH_MAX + 1); | ||
info->wo_path[H5FD_SPLITTER_PATH_MAX] = '\0'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sure to copy the whole path and ensure it's NUL-terminated, otherwise GCC complains about truncated output. |
||
HDstrncpy(info->log_file_path, vfd_config->log_file_path, H5FD_SPLITTER_PATH_MAX + 1); | ||
info->log_file_path[H5FD_SPLITTER_PATH_MAX] = '\0'; | ||
info->rw_fapl_id = H5P_FILE_ACCESS_DEFAULT; /* pre-set value */ | ||
info->wo_fapl_id = H5P_FILE_ACCESS_DEFAULT; /* pre-set value */ | ||
|
||
/* Set non-default channel FAPL IDs in splitter configuration info */ | ||
if (H5P_DEFAULT != vfd_config->rw_fapl_id) { | ||
|
@@ -412,8 +414,8 @@ H5Pget_fapl_splitter(hid_t fapl_id, H5FD_splitter_vfd_config_t *config /*out*/) | |
if (NULL == (fapl_ptr = (const H5FD_splitter_fapl_t *)H5P_peek_driver_info(plist_ptr))) | ||
HGOTO_ERROR(H5E_PLIST, H5E_BADVALUE, FAIL, "unable to get specific-driver info") | ||
|
||
HDstrncpy(config->wo_path, fapl_ptr->wo_path, H5FD_SPLITTER_PATH_MAX); | ||
HDstrncpy(config->log_file_path, fapl_ptr->log_file_path, H5FD_SPLITTER_PATH_MAX); | ||
HDstrncpy(config->wo_path, fapl_ptr->wo_path, H5FD_SPLITTER_PATH_MAX + 1); | ||
HDstrncpy(config->log_file_path, fapl_ptr->log_file_path, H5FD_SPLITTER_PATH_MAX + 1); | ||
config->ignore_wo_errs = fapl_ptr->ignore_wo_errs; | ||
|
||
/* Copy R/W and W/O FAPLs */ | ||
|
@@ -587,8 +589,8 @@ H5FD__splitter_fapl_copy(const void *_old_fa) | |
HGOTO_ERROR(H5E_VFL, H5E_CANTALLOC, NULL, "unable to allocate log file FAPL") | ||
|
||
H5MM_memcpy(new_fa_ptr, old_fa_ptr, sizeof(H5FD_splitter_fapl_t)); | ||
HDstrncpy(new_fa_ptr->wo_path, old_fa_ptr->wo_path, H5FD_SPLITTER_PATH_MAX); | ||
HDstrncpy(new_fa_ptr->log_file_path, old_fa_ptr->log_file_path, H5FD_SPLITTER_PATH_MAX); | ||
HDstrncpy(new_fa_ptr->wo_path, old_fa_ptr->wo_path, H5FD_SPLITTER_PATH_MAX + 1); | ||
HDstrncpy(new_fa_ptr->log_file_path, old_fa_ptr->log_file_path, H5FD_SPLITTER_PATH_MAX + 1); | ||
|
||
/* Copy R/W and W/O FAPLs */ | ||
if (H5FD__copy_plist(old_fa_ptr->rw_fapl_id, &(new_fa_ptr->rw_fapl_id)) < 0) | ||
|
@@ -688,8 +690,8 @@ H5FD__splitter_open(const char *name, unsigned flags, hid_t splitter_fapl_id, ha | |
HGOTO_ERROR(H5E_PLIST, H5E_CANTGET, NULL, "unable to get VFL driver info") | ||
|
||
/* Copy simpler info */ | ||
HDstrncpy(file_ptr->fa.wo_path, fapl_ptr->wo_path, H5FD_SPLITTER_PATH_MAX); | ||
HDstrncpy(file_ptr->fa.log_file_path, fapl_ptr->log_file_path, H5FD_SPLITTER_PATH_MAX); | ||
HDstrncpy(file_ptr->fa.wo_path, fapl_ptr->wo_path, H5FD_SPLITTER_PATH_MAX + 1); | ||
HDstrncpy(file_ptr->fa.log_file_path, fapl_ptr->log_file_path, H5FD_SPLITTER_PATH_MAX + 1); | ||
file_ptr->fa.ignore_wo_errs = fapl_ptr->ignore_wo_errs; | ||
|
||
/* Copy R/W and W/O channel FAPLs. */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1043,7 +1043,7 @@ H5F__super_read(H5F_t *f, H5P_genplist_t *fa_plist, hbool_t initial_read) | |
HDONE_ERROR(H5E_FILE, H5E_CANTUNPIN, FAIL, "unable to unpin driver info") | ||
|
||
/* Evict the driver info block from the cache */ | ||
if (H5AC_expunge_entry(f, H5AC_DRVRINFO, sblock->driver_addr, H5AC__NO_FLAGS_SET) < 0) | ||
if (sblock && H5AC_expunge_entry(f, H5AC_DRVRINFO, sblock->driver_addr, H5AC__NO_FLAGS_SET) < 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GCC complains about a potential dereference of a NULL sblock here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is trickier than that - the sblock should be set to NULL after the H5AC_unprotect, about 15 lines earlier. But, if that's the case, how's the expunge supposed to work? There's a bigger algorithmic problem here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, but the algorithm is beyond me and adding an sblock check here theoretically won't do more harm than good. If sblock actually were NULL here, we'd immediately crash rather than H5AC_expunge_entry doing something "bad". Now, we just skip the expunge anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I can tell, the H5AC_unprotect call flushes the entry, but I don't see anywhere that the entry is freed/set to NULL, especially since H5AC_unprotect only accepts a single level of pointer indirection. Since the code following the expunge also checks to make sure sblock is valid before using it, this seems like a reasonable change, but maybe @jrmainzer would have a better idea? |
||
HDONE_ERROR(H5E_FILE, H5E_CANTEXPUNGE, FAIL, "unable to expunge driver info block") | ||
} /* end if */ | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -359,7 +359,7 @@ H5PLget(unsigned int idx, char *path_buf, size_t buf_size) | |
|
||
/* If the path buffer is not NULL, copy the path to the buffer */ | ||
if (path_buf) { | ||
HDstrncpy(path_buf, path, MIN((size_t)(path_len + 1), buf_size)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment about GCC being confused about the usage of MIN here. |
||
HDstrncpy(path_buf, path, buf_size); | ||
if ((size_t)path_len >= buf_size) | ||
path_buf[buf_size - 1] = '\0'; | ||
} /* end if */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1453,7 +1453,7 @@ H5Pget_efile_prefix(hid_t plist_id, char *prefix /*out*/, size_t size) | |
/* Copy to user's buffer, if given */ | ||
len = HDstrlen(my_prefix); | ||
if (prefix) { | ||
HDstrncpy(prefix, my_prefix, MIN(len + 1, size)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above |
||
HDstrncpy(prefix, my_prefix, size); | ||
if (len >= size) | ||
prefix[size - 1] = '\0'; | ||
} /* end if */ | ||
|
@@ -1543,7 +1543,7 @@ H5Pget_virtual_prefix(hid_t plist_id, char *prefix /*out*/, size_t size) | |
/* Copy to user's buffer, if given */ | ||
len = HDstrlen(my_prefix); | ||
if (prefix) { | ||
HDstrncpy(prefix, my_prefix, MIN(len + 1, size)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above |
||
HDstrncpy(prefix, my_prefix, size); | ||
if (len >= size) | ||
prefix[size - 1] = '\0'; | ||
} /* end if */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -333,7 +333,7 @@ H5P__encode_cb(H5P_genprop_t *prop, void *_udata) | |
/* Encode (or not, if the 'encode' flag is off) the property's name */ | ||
prop_name_len = HDstrlen(prop->name) + 1; | ||
if (udata->encode) { | ||
HDstrncpy((char *)*(udata->pp), prop->name, prop_name_len); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, we don't know the size of the output buffer, so using strcpy here is no less safe than strncpy and strncpy gives a warning since we're just giving it the length of the source being copied. |
||
HDstrcpy((char *)*(udata->pp), prop->name); | ||
*(uint8_t **)(udata->pp) += prop_name_len; | ||
} /* end if */ | ||
*(udata->enc_size_ptr) += prop_name_len; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1343,13 +1343,21 @@ attr_search(hid_t oid, const char *attr_name, const H5A_info_t H5_ATTR_UNUSED *a | |
ret = FAIL; | ||
} | ||
else { | ||
size_t buffer_space = w - 1; | ||
|
||
HDmemset(obj_name, '\0', w); | ||
if (op_name[0] != '/') { | ||
HDstrncat(obj_name, buf, u + 1); | ||
if (buf[u - 1] != '/') | ||
HDstrncat(obj_name, "/", (size_t)2); | ||
HDstrncat(obj_name, buf, buffer_space); | ||
buffer_space -= MIN(buffer_space, u); | ||
|
||
if (buf[u - 1] != '/') { | ||
HDstrncat(obj_name, "/", buffer_space); | ||
buffer_space -= MIN(buffer_space, 2); | ||
} | ||
} | ||
HDstrncat(obj_name, op_name, v + 1); | ||
|
||
HDstrncat(obj_name, op_name, buffer_space); | ||
buffer_space -= MIN(buffer_space, v); | ||
|
||
handle_attributes(oid, obj_name, NULL, 0, NULL); | ||
HDfree(obj_name); | ||
|
@@ -1421,10 +1429,10 @@ lnk_search(const char *path, const H5L_info2_t *li, void *_op_data) | |
else { | ||
if (k == 2) { | ||
HDstrcpy(search_name, "/"); | ||
HDstrncat(search_name, op_name, search_len + 1); | ||
HDstrcat(search_name, op_name); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Already know the buffer size, use strcat to not generate a warning. |
||
} | ||
else | ||
HDstrncpy(search_name, op_name, search_len + 1); | ||
HDstrcpy(search_name, op_name); | ||
search_name[search_len + k - 1] = '\0'; | ||
|
||
if (HDstrcmp(path, search_name) == 0) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While using MIN here can make strncpy do less work in the case that len + 1 is smaller than size, GCC isn't intelligent enough to figure out what we're doing here, so stick to something sensible.