Skip to content

Commit

Permalink
Forbid basename(3) and dirname(3)
Browse files Browse the repository at this point in the history
There are at least two interpretations of basename(3),
in addition to both functions being allowed to /both/ return a static
buffer (unsuitable in multi-threaded environments) /and/ raze the input
(which encourages overallocations, at best)

Reviewed-by: John Kennedy <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12105
  • Loading branch information
nabijaczleweli authored and behlendorf committed Jun 11, 2021
1 parent 64dfdab commit feb04e6
Showing 7 changed files with 45 additions and 33 deletions.
3 changes: 2 additions & 1 deletion cmd/zed/agents/zfs_retire.c
Original file line number Diff line number Diff line change
@@ -38,6 +38,7 @@
#include <sys/fs/zfs.h>
#include <sys/fm/protocol.h>
#include <sys/fm/fs/zfs.h>
#include <libzutil.h>
#include <libzfs.h>
#include <string.h>

@@ -240,7 +241,7 @@ replace_with_spare(fmd_hdl_t *hdl, zpool_handle_t *zhp, nvlist_t *vdev)
ZPOOL_CONFIG_CHILDREN, &spares[s], 1);

fmd_hdl_debug(hdl, "zpool_vdev_replace '%s' with spare '%s'",
dev_name, basename(spare_name));
dev_name, zfs_basename(spare_name));

if (zpool_vdev_attach(zhp, dev_name, spare_name,
replacement, B_TRUE, rebuild) == 0) {
3 changes: 3 additions & 0 deletions config/Rules.am
Original file line number Diff line number Diff line change
@@ -54,6 +54,9 @@ if BUILD_FREEBSD
AM_CPPFLAGS += -DTEXT_DOMAIN=\"zfs-freebsd-user\"
endif
AM_CPPFLAGS += -D"strtok(...)=strtok(__VA_ARGS__) __attribute__((deprecated(\"Use strtok_r(3) instead!\")))"
AM_CPPFLAGS += -D"__xpg_basename(...)=__xpg_basename(__VA_ARGS__) __attribute__((deprecated(\"basename(3) is underspecified. Use zfs_basename() instead!\")))"
AM_CPPFLAGS += -D"basename(...)=basename(__VA_ARGS__) __attribute__((deprecated(\"basename(3) is underspecified. Use zfs_basename() instead!\")))"
AM_CPPFLAGS += -D"dirname(...)=dirname(__VA_ARGS__) __attribute__((deprecated(\"dirname(3) is underspecified. Use zfs_dirnamelen() instead!\")))"

AM_LDFLAGS = $(DEBUG_LDFLAGS)
AM_LDFLAGS += $(ASAN_LDFLAGS)
2 changes: 1 addition & 1 deletion lib/libzfs/libzfs_pool.c
Original file line number Diff line number Diff line change
@@ -4309,7 +4309,7 @@ zfs_save_arguments(int argc, char **argv, char *string, int len)
{
int i;

(void) strlcpy(string, basename(argv[0]), len);
(void) strlcpy(string, zfs_basename(argv[0]), len);
for (i = 1; i < argc; i++) {
(void) strlcat(string, " ", len);
(void) strlcat(string, argv[i], len);
18 changes: 5 additions & 13 deletions lib/libzpool/kernel.c
Original file line number Diff line number Diff line change
@@ -31,6 +31,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <libzutil.h>
#include <sys/crypto/icp.h>
#include <sys/processor.h>
#include <sys/rrwlock.h>
@@ -541,19 +542,10 @@ void
__dprintf(boolean_t dprint, const char *file, const char *func,
int line, const char *fmt, ...)
{
const char *newfile;
va_list adx;

/*
* Get rid of annoying "../common/" prefix to filename.
*/
newfile = strrchr(file, '/');
if (newfile != NULL) {
newfile = newfile + 1; /* Get rid of leading / */
} else {
newfile = file;
}
/* Get rid of annoying "../common/" prefix to filename. */
const char *newfile = zfs_basename(file);

va_list adx;
if (dprint) {
/* dprintf messages are printed immediately */

@@ -1040,7 +1032,7 @@ zfs_file_open(const char *path, int flags, int mode, zfs_file_t **fpp)

if (vn_dumpdir != NULL) {
char *dumppath = umem_zalloc(MAXPATHLEN, UMEM_NOFAIL);
char *inpath = basename((char *)(uintptr_t)path);
const char *inpath = zfs_basename(path);

(void) snprintf(dumppath, MAXPATHLEN,
"%s/%s", vn_dumpdir, inpath);
37 changes: 27 additions & 10 deletions lib/libzutil/zutil_import.c
Original file line number Diff line number Diff line change
@@ -154,6 +154,17 @@ zutil_strdup(libpc_handle_t *hdl, const char *str)
return (ret);
}

static char *
zutil_strndup(libpc_handle_t *hdl, const char *str, size_t n)
{
char *ret;

if ((ret = strndup(str, n)) == NULL)
(void) zutil_no_memory(hdl);

return (ret);
}

/*
* Intermediate structures used to gather configuration information.
*/
@@ -1272,20 +1283,22 @@ zpool_find_import_scan_path(libpc_handle_t *hdl, pthread_mutex_t *lock,
{
int error = 0;
char path[MAXPATHLEN];
char *d, *b;
char *dpath, *name;
char *d = NULL;
ssize_t dl;
const char *dpath, *name;

/*
* Separate the directory part and last part of the
* path. We do this so that we can get the realpath of
* Separate the directory and the basename.
* We do this so that we can get the realpath of
* the directory. We don't get the realpath on the
* whole path because if it's a symlink, we want the
* path of the symlink not where it points to.
*/
d = zutil_strdup(hdl, dir);
b = zutil_strdup(hdl, dir);
dpath = dirname(d);
name = basename(b);
name = zfs_basename(dir);
if ((dl = zfs_dirnamelen(dir)) == -1)
dpath = ".";
else
dpath = d = zutil_strndup(hdl, dir, dl);

if (realpath(dpath, path) == NULL) {
error = errno;
@@ -1303,7 +1316,6 @@ zpool_find_import_scan_path(libpc_handle_t *hdl, pthread_mutex_t *lock,
zpool_find_import_scan_add_slice(hdl, lock, cache, path, name, order);

out:
free(b);
free(d);
return (error);
}
@@ -1506,6 +1518,7 @@ discover_cached_paths(libpc_handle_t *hdl, nvlist_t *nv,
avl_tree_t *cache, pthread_mutex_t *lock)
{
char *path = NULL;
ssize_t dl;
uint_t children;
nvlist_t **child;

@@ -1521,8 +1534,12 @@ discover_cached_paths(libpc_handle_t *hdl, nvlist_t *nv,
* our directory cache.
*/
if (nvlist_lookup_string(nv, ZPOOL_CONFIG_PATH, &path) == 0) {
if ((dl = zfs_dirnamelen(path)) == -1)
path = ".";
else
path[dl] = '\0';
return (zpool_find_import_scan_dir(hdl, lock, cache,
dirname(path), 0));
path, 0));
}
return (0);
}
2 changes: 2 additions & 0 deletions tests/zfs-tests/tests/functional/ctime/Makefile.am
Original file line number Diff line number Diff line change
@@ -11,3 +11,5 @@ pkgexecdir = $(datadir)/@PACKAGE@/zfs-tests/tests/functional/ctime

pkgexec_PROGRAMS = ctime
ctime_SOURCES = ctime.c

ctime_LDADD = $(abs_top_builddir)/lib/libzfs_core/libzfs_core.la
13 changes: 5 additions & 8 deletions tests/zfs-tests/tests/functional/ctime/ctime.c
Original file line number Diff line number Diff line change
@@ -37,6 +37,7 @@
#include <utime.h>
#include <stdio.h>
#include <stdlib.h>
#include <libzutil.h>
#include <unistd.h>
#include <strings.h>
#include <errno.h>
@@ -149,22 +150,18 @@ static int
do_link(const char *pfile)
{
int ret = 0;
char link_file[BUFSIZ] = { 0 };
char pfile_copy[BUFSIZ] = { 0 };
char *dname;
char link_file[BUFSIZ + 16] = { 0 };

if (pfile == NULL) {
return (-1);
}

strncpy(pfile_copy, pfile, sizeof (pfile_copy)-1);
pfile_copy[sizeof (pfile_copy) - 1] = '\0';
/*
* Figure out source file directory name, and create
* the link file in the same directory.
*/
dname = dirname((char *)pfile_copy);
(void) snprintf(link_file, BUFSIZ, "%s/%s", dname, "link_file");
(void) snprintf(link_file, sizeof (link_file),
"%.*s/%s", (int)zfs_dirnamelen(pfile), pfile, "link_file");

if (link(pfile, link_file) == -1) {
(void) fprintf(stderr, "link(%s, %s) failed with errno %d\n",
@@ -321,7 +318,7 @@ main(int argc, char *argv[])
(void) snprintf(tfile, sizeof (tfile), "%s/%s", penv[0], penv[1]);

/*
* If the test file is exists, remove it first.
* If the test file exists, remove it first.
*/
if (access(tfile, F_OK) == 0) {
(void) unlink(tfile);

0 comments on commit feb04e6

Please sign in to comment.