Skip to content
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

asprintf(3) error handling, mostly #11993

Closed
wants to merge 7 commits into from
Closed

asprintf(3) error handling, mostly #11993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7bf9ead
libzfs: zpool_load_compat(): open feature file cloexec
nabijaczleweli Apr 29, 2021
e1bc568
libzfs: zpool_load_compat(): don't free undefined pointers
nabijaczleweli Apr 30, 2021
50f83ec
zpool: vdev_run_cmd(): don't free undefined pointers
nabijaczleweli May 3, 2021
d4e5303
zutil_device_path/linux: zfs_get_enclosure_sysfs_path(): don't leak d…
nabijaczleweli May 3, 2021
a0ca497
zutil_device_path/linux: zfs_get_enclosure_sysfs_path(): don't free u…
nabijaczleweli May 3, 2021
431a021
libzfsbootenv: lzbe_set_boot_device(): don't free undefined pointer
nabijaczleweli May 3, 2021
a44f405
libzfs: zfs_asprintf(): don't return undefined pointer
nabijaczleweli May 3, 2021
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
15 changes: 10 additions & 5 deletions cmd/zpool/zpool_iter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,19 +494,25 @@ vdev_run_cmd(vdev_cmd_data_t *data, char *cmd)
/* Setup our custom environment variables */
rc = asprintf(&env[1], "VDEV_PATH=%s",
data->path ? data->path : "");
if (rc == -1)
if (rc == -1) {
env[1] = NULL;
goto out;
}

rc = asprintf(&env[2], "VDEV_UPATH=%s",
data->upath ? data->upath : "");
if (rc == -1)
if (rc == -1) {
env[2] = NULL;
goto out;
}

rc = asprintf(&env[3], "VDEV_ENC_SYSFS_PATH=%s",
data->vdev_enc_sysfs_path ?
data->vdev_enc_sysfs_path : "");
if (rc == -1)
if (rc == -1) {
env[3] = NULL;
goto out;
}

/* Run the command */
rc = libzfs_run_process_get_stdout_nopath(cmd, argv, env, &lines,
Expand All @@ -525,8 +531,7 @@ vdev_run_cmd(vdev_cmd_data_t *data, char *cmd)

/* Start with i = 1 since env[0] was statically allocated */
for (i = 1; i < ARRAY_SIZE(env); i++)
if (env[i] != NULL)
free(env[i]);
free(env[i]);
}

/*
Expand Down
18 changes: 10 additions & 8 deletions lib/libzfs/libzfs_pool.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4812,14 +4812,14 @@ zpool_load_compat(const char *compat, boolean_t *features, char *report,
file != NULL;
file = strtok_r(NULL, ",", &ps)) {

boolean_t l_features[SPA_FEATURES];
boolean_t l_features[SPA_FEATURES] = {B_FALSE};

enum { Z_SYSCONF, Z_DATA } source;

/* try sysconfdir first, then datadir */
source = Z_SYSCONF;
if ((featfd = openat(sdirfd, file, 0, O_RDONLY)) < 0) {
featfd = openat(ddirfd, file, 0, O_RDONLY);
if ((featfd = openat(sdirfd, file, O_RDONLY | O_CLOEXEC)) < 0) {
featfd = openat(ddirfd, file, O_RDONLY | O_CLOEXEC);
source = Z_DATA;
}

Expand All @@ -4835,13 +4835,18 @@ zpool_load_compat(const char *compat, boolean_t *features, char *report,
continue;
}

#if !defined(MAP_POPULATE) && defined(MAP_PREFAULT_READ)
#define MAP_POPULATE MAP_PREFAULT_READ
#elif !defined(MAP_POPULATE)
#define MAP_POPULATE 0
#endif
/* private mmap() so we can strtok safely */
fc = (char *)mmap(NULL, fs.st_size,
PROT_READ|PROT_WRITE, MAP_PRIVATE, featfd, 0);
PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_POPULATE, featfd, 0);
(void) close(featfd);

/* map ok, and last character == newline? */
if (fc < 0 || fc[fs.st_size - 1] != '\n') {
if (fc == MAP_FAILED || fc[fs.st_size - 1] != '\n') {
(void) munmap((void *) fc, fs.st_size);
strlcat(err_badfile, file, ZFS_MAXPROPLEN);
strlcat(err_badfile, " ", ZFS_MAXPROPLEN);
Expand All @@ -4851,9 +4856,6 @@ zpool_load_compat(const char *compat, boolean_t *features, char *report,

ret_nofiles = B_FALSE;

for (uint_t i = 0; i < SPA_FEATURES; i++)
l_features[i] = B_FALSE;

/* replace last char with NUL to ensure we have a delimiter */
fc[fs.st_size - 1] = '\0';

Expand Down
4 changes: 3 additions & 1 deletion lib/libzfs/libzfs_util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,8 +782,10 @@ zfs_asprintf(libzfs_handle_t *hdl, const char *fmt, ...)

va_end(ap);

if (err < 0)
if (err < 0) {
(void) no_memory(hdl);
ret = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, zfs_asprintf() and zfs_alloc() say "which will die if the allocation fails", which is strictly false. no_memory() in the same file says "Display an out of memory error message and abort the current program.", which is also wrong, on both counts.

If you trace the call path you'll see this is in fact true when hdl->libzfs_printerr is set. But clearly we can't count on that so I agree setting this to NULL is the right thing to do here.

no_memory()
    zfs_error(, EZFS_NOMEM, )
        zfs_error_fmt()
            zfs_verror()
                exit()

}

return (ret);
}
Expand Down
7 changes: 3 additions & 4 deletions lib/libzfsbootenv/lzbe_device.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,11 @@ lzbe_set_boot_device(const char *pool, lzbe_flags_t flag, const char *device)
if (strncmp(device, "zfs:", 4) == 0) {
fnvlist_add_string(nv, OS_BOOTONCE, device);
} else {
descriptor = NULL;
if (asprintf(&descriptor, "zfs:%s:", device) > 0)
if (asprintf(&descriptor, "zfs:%s:", device) > 0) {
fnvlist_add_string(nv, OS_BOOTONCE, descriptor);
else
free(descriptor);
} else
rv = ENOMEM;
free(descriptor);
}
}

Expand Down
26 changes: 13 additions & 13 deletions lib/libzutil/os/linux/zutil_device_path_os.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,10 +195,8 @@ zfs_get_enclosure_sysfs_path(const char *dev_name)
}

dp = opendir(tmp1);
if (dp == NULL) {
tmp1 = NULL; /* To make free() at the end a NOP */
if (dp == NULL)
goto end;
}

/*
* Look though all sysfs entries in /sys/block/<dev>/device for
Expand All @@ -209,18 +207,16 @@ zfs_get_enclosure_sysfs_path(const char *dev_name)
if (strstr(ep->d_name, "enclosure_device") == NULL)
continue;

if (asprintf(&tmp2, "%s/%s", tmp1, ep->d_name) == -1 ||
tmp2 == NULL)
if (asprintf(&tmp2, "%s/%s", tmp1, ep->d_name) == -1) {
tmp2 = NULL;
break;
}

size = readlink(tmp2, buf, sizeof (buf));

/* Did readlink fail or crop the link name? */
if (size == -1 || size >= sizeof (buf)) {
free(tmp2);
tmp2 = NULL; /* To make free() at the end a NOP */
if (size == -1 || size >= sizeof (buf))
break;
}

/*
* We got a valid link. readlink() doesn't terminate strings
Expand Down Expand Up @@ -306,9 +302,10 @@ dm_get_underlying_path(const char *dm_name)
else
dev_str = tmp;

size = asprintf(&tmp, "/sys/block/%s/slaves/", dev_str);
if (size == -1 || !tmp)
if ((size = asprintf(&tmp, "/sys/block/%s/slaves/", dev_str)) == -1) {
tmp = NULL;
goto end;
}

dp = opendir(tmp);
if (dp == NULL)
Expand All @@ -334,7 +331,9 @@ dm_get_underlying_path(const char *dm_name)
if (!enclosure_path)
continue;

size = asprintf(&path, "/dev/%s", ep->d_name);
if ((size = asprintf(
&path, "/dev/%s", ep->d_name)) == -1)
path = NULL;
free(enclosure_path);
break;
}
Expand All @@ -352,7 +351,8 @@ dm_get_underlying_path(const char *dm_name)
* enclosure devices. Throw up out hands and return the first
* underlying path.
*/
size = asprintf(&path, "/dev/%s", first_path);
if ((size = asprintf(&path, "/dev/%s", first_path)) == -1)
path = NULL;
}

free(first_path);
Expand Down