Skip to content

Commit

Permalink
Handle possible null pointers from malloc/strdup/strndup()
Browse files Browse the repository at this point in the history
GCC 12.1.1_p20220625's static analyzer caught these.

Of the two in the btree test, one had previously been caught by Coverity
and Smatch, but GCC flagged it as a false positive. Upon examining how
other test cases handle this, the solution was changed from
`ASSERT3P(node, !=, NULL);` to using `perror()` to be consistent with
the fixes to the other fixes done to the ZTS code.

That approach was also used in ZED since I did not see a better way of
handling this there. Also, upon inspection, additional unchecked
pointers from malloc()/calloc()/strdup() were found in ZED, so those
were handled too.

In other parts of the code, the existing methods to avoid issues from
memory allocators returning NULL were used, such as using
`umem_alloc(size, UMEM_NOFAIL)` or returning `ENOMEM`.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Closes #13979
  • Loading branch information
ryao authored Oct 7, 2022
1 parent 2ba240f commit 72c99dc
Show file tree
Hide file tree
Showing 10 changed files with 63 additions and 7 deletions.
19 changes: 19 additions & 0 deletions cmd/zed/agents/fmd_serd.c
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,18 @@ fmd_serd_eng_alloc(const char *name, uint64_t n, hrtime_t t)
fmd_serd_eng_t *sgp;

sgp = malloc(sizeof (fmd_serd_eng_t));
if (sgp == NULL) {
perror("malloc");
exit(EXIT_FAILURE);
}
memset(sgp, 0, sizeof (fmd_serd_eng_t));

sgp->sg_name = strdup(name);
if (sgp->sg_name == NULL) {
perror("strdup");
exit(EXIT_FAILURE);
}

sgp->sg_flags = FMD_SERD_DIRTY;
sgp->sg_n = n;
sgp->sg_t = t;
Expand Down Expand Up @@ -123,6 +132,12 @@ fmd_serd_hash_create(fmd_serd_hash_t *shp)
shp->sh_hashlen = FMD_STR_BUCKETS;
shp->sh_hash = calloc(shp->sh_hashlen, sizeof (void *));
shp->sh_count = 0;

if (shp->sh_hash == NULL) {
perror("calloc");
exit(EXIT_FAILURE);
}

}

void
Expand Down Expand Up @@ -241,6 +256,10 @@ fmd_serd_eng_record(fmd_serd_eng_t *sgp, hrtime_t hrt)
fmd_serd_eng_discard(sgp, list_tail(&sgp->sg_list));

sep = malloc(sizeof (fmd_serd_elem_t));
if (sep == NULL) {
perror("malloc");
exit(EXIT_FAILURE);
}
sep->se_hrt = hrt;

list_insert_head(&sgp->sg_list, sep);
Expand Down
10 changes: 10 additions & 0 deletions cmd/zed/agents/zfs_mod.c
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,11 @@ zfs_unavail_pool(zpool_handle_t *zhp, void *data)
if (zfs_toplevel_state(zhp) < VDEV_STATE_DEGRADED) {
unavailpool_t *uap;
uap = malloc(sizeof (unavailpool_t));
if (uap == NULL) {
perror("malloc");
exit(EXIT_FAILURE);
}

uap->uap_zhp = zhp;
list_insert_tail((list_t *)data, uap);
} else {
Expand Down Expand Up @@ -411,6 +416,11 @@ zfs_process_add(zpool_handle_t *zhp, nvlist_t *vdev, boolean_t labeled)
* completed.
*/
device = malloc(sizeof (pendingdev_t));
if (device == NULL) {
perror("malloc");
exit(EXIT_FAILURE);
}

(void) strlcpy(device->pd_physpath, physpath,
sizeof (device->pd_physpath));
list_insert_tail(&g_device_list, device);
Expand Down
4 changes: 4 additions & 0 deletions cmd/zed/zed_exec.c
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,10 @@ _zed_exec_fork_child(uint64_t eid, const char *dir, const char *prog,
node->pid = pid;
node->eid = eid;
node->name = strdup(prog);
if (node->name == NULL) {
perror("strdup");
exit(EXIT_FAILURE);
}

avl_add(&_launched_processes, node);
}
Expand Down
9 changes: 5 additions & 4 deletions cmd/ztest.c
Original file line number Diff line number Diff line change
Expand Up @@ -3512,7 +3512,8 @@ ztest_split_pool(ztest_ds_t *zd, uint64_t id)
VERIFY0(nvlist_lookup_nvlist_array(tree, ZPOOL_CONFIG_CHILDREN,
&child, &children));

schild = malloc(rvd->vdev_children * sizeof (nvlist_t *));
schild = umem_alloc(rvd->vdev_children * sizeof (nvlist_t *),
UMEM_NOFAIL);
for (c = 0; c < children; c++) {
vdev_t *tvd = rvd->vdev_child[c];
nvlist_t **mchild;
Expand Down Expand Up @@ -3546,7 +3547,7 @@ ztest_split_pool(ztest_ds_t *zd, uint64_t id)

for (c = 0; c < schildren; c++)
fnvlist_free(schild[c]);
free(schild);
umem_free(schild, rvd->vdev_children * sizeof (nvlist_t *));
fnvlist_free(split);

spa_config_exit(spa, SCL_VDEV, FTAG);
Expand Down Expand Up @@ -6663,7 +6664,7 @@ join_strings(char **strings, const char *sep)
}

size_t buflen = totallen + 1;
char *o = malloc(buflen); /* trailing 0 byte */
char *o = umem_alloc(buflen, UMEM_NOFAIL); /* trailing 0 byte */
o[0] = '\0';
for (char **sp = strings; *sp != NULL; sp++) {
size_t would;
Expand Down Expand Up @@ -6925,7 +6926,7 @@ ztest_run_zdb(const char *pool)
pool);
ASSERT3U(would, <, len);

free(set_gvars_args_joined);
umem_free(set_gvars_args_joined, strlen(set_gvars_args_joined) + 1);

if (ztest_opts.zo_verbose >= 5)
(void) printf("Executing %s\n", zdb);
Expand Down
5 changes: 3 additions & 2 deletions lib/libzfs/libzfs_sendrecv.c
Original file line number Diff line number Diff line change
Expand Up @@ -4387,7 +4387,7 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap,
* prepend a path separator.
*/
int len = strlen(drrb->drr_toname);
cp = malloc(len + 2);
cp = umem_alloc(len + 2, UMEM_NOFAIL);
cp[0] = '/';
(void) strcpy(&cp[1], drrb->drr_toname);
chopprefix = cp;
Expand Down Expand Up @@ -4440,7 +4440,8 @@ zfs_receive_one(libzfs_handle_t *hdl, int infd, const char *tosnap,
*/
(void) strlcpy(destsnap, tosnap, sizeof (destsnap));
(void) strlcat(destsnap, chopprefix, sizeof (destsnap));
free(cp);
if (cp != NULL)
umem_free(cp, strlen(cp) + 1);
if (!zfs_name_valid(destsnap, ZFS_TYPE_SNAPSHOT)) {
err = zfs_error(hdl, EZFS_INVALIDNAME, errbuf);
goto out;
Expand Down
3 changes: 3 additions & 0 deletions lib/libzutil/zutil_import.c
Original file line number Diff line number Diff line change
Expand Up @@ -1845,6 +1845,9 @@ zpool_find_config(libpc_handle_t *hdl, const char *target, nvlist_t **configp,
int count = 0;
char *targetdup = strdup(target);

if (targetdup == NULL)
return (ENOMEM);

*configp = NULL;

if ((sepp = strpbrk(targetdup, "/@")) != NULL)
Expand Down
4 changes: 4 additions & 0 deletions tests/zfs-tests/cmd/badsend.c
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ main(int argc, char const * const argv[])
tosnap = p + 1;

fsname = strndup(tofull, p - tofull);
if (fsname == NULL) {
perror("strndup");
exit(EXIT_FAILURE);
}
if (strncmp(fsname, fromfull, p - tofull) != 0)
usage(argv[0]);

Expand Down
9 changes: 8 additions & 1 deletion tests/zfs-tests/cmd/btree_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,10 @@ drain_tree(zfs_btree_t *bt, char *why)
zfs_btree_add_idx(bt, &randval, &bt_idx);

node = malloc(sizeof (int_node_t));
ASSERT3P(node, !=, NULL);
if (node == NULL) {
perror("malloc");
exit(EXIT_FAILURE);
}

node->data = randval;
if ((ret = avl_find(&avl, node, &avl_idx)) != NULL) {
Expand Down Expand Up @@ -319,6 +322,10 @@ stress_tree(zfs_btree_t *bt, char *why)

uint64_t randval = random() % tree_limit;
node = malloc(sizeof (*node));
if (node == NULL) {
perror("malloc");
exit(EXIT_FAILURE);
}
node->data = randval;

max = randval > max ? randval : max;
Expand Down
2 changes: 2 additions & 0 deletions tests/zfs-tests/cmd/file/largest_file.c
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ main(int argc, char **argv)
return (errno);

testfile = strdup(argv[1]);
if (testfile == NULL)
return (errno);

fd = open(testfile, O_CREAT | O_RDWR, mode);
if (fd < 0) {
Expand Down
5 changes: 5 additions & 0 deletions tests/zfs-tests/cmd/nvlist_to_lua.c
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,11 @@ run_tests(void)
/* Note: maximum nvlist key length is 32KB */
int len = 1024 * 31;
char *bigstring = malloc(len);
if (bigstring == NULL) {
perror("malloc");
exit(EXIT_FAILURE);
}

for (int i = 0; i < len; i++)
bigstring[i] = 'a' + i % 26;
bigstring[len - 1] = '\0';
Expand Down

0 comments on commit 72c99dc

Please sign in to comment.