Message ID | e64e27d5cb103b7764f1a05b6eda7e7fedd517c5.1645114783.git.qemu_oss@crudebyte.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [PULL,v2,1/5] tests/9pfs: use g_autofree where possible | expand |
On Thu, 17 Feb 2022 at 16:43, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index d1660d67fa..ce12f64853 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -805,6 +805,19 @@ static inline int platform_does_not_support_system(const char *command) > } > #endif /* !HAVE_SYSTEM_FUNCTION */ > > +/** > + * Duplicate directory entry @dent. > + * > + * It is highly recommended to use this function instead of open coding > + * duplication of @c dirent objects, because the actual @c struct @c dirent > + * size may be bigger or shorter than @c sizeof(struct dirent) and correct > + * handling is platform specific (see gitlab issue #841). > + * > + * @dent - original directory entry to be duplicated > + * @returns duplicated directory entry which should be freed with g_free() > + */ > +struct dirent *qemu_dirent_dup(struct dirent *dent); Hi; I just noticed this has landed in git recently. Please don't add new prototypes to osdep.h -- it is a header included by every single C file in the tree, so making it bigger slows down compilation. osdep.h is supposed to contain only: * things which everybody needs * things without which code would work on most platforms but fail to compile or misbehave on a minority of host OSes (ie system incompatibility handling) This prototype is neither of those -- please find or create a more appropriate header file for it, that can be included only by the source files that actually need it. thanks -- PMM
On Dienstag, 22. Februar 2022 14:21:52 CET Peter Maydell wrote: > On Thu, 17 Feb 2022 at 16:43, Christian Schoenebeck > > <qemu_oss@crudebyte.com> wrote: > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > > index d1660d67fa..ce12f64853 100644 > > --- a/include/qemu/osdep.h > > +++ b/include/qemu/osdep.h > > @@ -805,6 +805,19 @@ static inline int > > platform_does_not_support_system(const char *command)> > > } > > #endif /* !HAVE_SYSTEM_FUNCTION */ > > > > +/** > > + * Duplicate directory entry @dent. > > + * > > + * It is highly recommended to use this function instead of open coding > > + * duplication of @c dirent objects, because the actual @c struct @c > > dirent + * size may be bigger or shorter than @c sizeof(struct dirent) > > and correct + * handling is platform specific (see gitlab issue #841). > > + * > > + * @dent - original directory entry to be duplicated > > + * @returns duplicated directory entry which should be freed with > > g_free() > > + */ > > +struct dirent *qemu_dirent_dup(struct dirent *dent); > > Hi; I just noticed this has landed in git recently. > Please don't add new prototypes to osdep.h -- it is > a header included by every single C file in the tree, so > making it bigger slows down compilation. osdep.h is supposed > to contain only: > * things which everybody needs > * things without which code would work on most platforms but > fail to compile or misbehave on a minority of host OSes > (ie system incompatibility handling) > > This prototype is neither of those -- please find or create a more > appropriate header file for it, that can be included only by the > source files that actually need it. > > thanks > -- PMM Good to know, because the pending Darwin series would have added stuff to osdep.h as well: https://lore.kernel.org/qemu-devel/20220220165056.72289-10-wwcohen@gmail.com/ We'll find a different place. Thanks! Best regards, Christian Schoenebeck
On Tue, 22 Feb 2022 14:54:17 +0100 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Dienstag, 22. Februar 2022 14:21:52 CET Peter Maydell wrote: > > On Thu, 17 Feb 2022 at 16:43, Christian Schoenebeck > > > > <qemu_oss@crudebyte.com> wrote: > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > > > index d1660d67fa..ce12f64853 100644 > > > --- a/include/qemu/osdep.h > > > +++ b/include/qemu/osdep.h > > > @@ -805,6 +805,19 @@ static inline int > > > platform_does_not_support_system(const char *command)> > > > } > > > #endif /* !HAVE_SYSTEM_FUNCTION */ > > > > > > +/** > > > + * Duplicate directory entry @dent. > > > + * > > > + * It is highly recommended to use this function instead of open coding > > > + * duplication of @c dirent objects, because the actual @c struct @c > > > dirent + * size may be bigger or shorter than @c sizeof(struct dirent) > > > and correct + * handling is platform specific (see gitlab issue #841). > > > + * > > > + * @dent - original directory entry to be duplicated > > > + * @returns duplicated directory entry which should be freed with > > > g_free() > > > + */ > > > +struct dirent *qemu_dirent_dup(struct dirent *dent); > > > > Hi; I just noticed this has landed in git recently. > > Please don't add new prototypes to osdep.h -- it is > > a header included by every single C file in the tree, so > > making it bigger slows down compilation. osdep.h is supposed > > to contain only: > > * things which everybody needs > > * things without which code would work on most platforms but > > fail to compile or misbehave on a minority of host OSes > > (ie system incompatibility handling) > > > > This prototype is neither of those -- please find or create a more > > appropriate header file for it, that can be included only by the > > source files that actually need it. > > > > thanks > > -- PMM > > Good to know, because the pending Darwin series would have added stuff to > osdep.h as well: > https://lore.kernel.org/qemu-devel/20220220165056.72289-10-wwcohen@gmail.com/ > > We'll find a different place. > I suggest you move all that under hw/9pfs for now since it is the only user and we don't really know if there will ever be a new one (especially now that development on C virtiofsd is slowing down). Cheers, -- Greg > Thanks! > > Best regards, > Christian Schoenebeck > >
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c index b38088e066..7a7cd5c5ba 100644 --- a/hw/9pfs/9p-synth.c +++ b/hw/9pfs/9p-synth.c @@ -182,7 +182,12 @@ static int synth_opendir(FsContext *ctx, V9fsSynthOpenState *synth_open; V9fsSynthNode *node = *(V9fsSynthNode **)fs_path->data; - synth_open = g_malloc(sizeof(*synth_open)); + /* + * V9fsSynthOpenState contains 'struct dirent' which have OS-specific + * properties, thus it's zero cleared on allocation here and below + * in synth_open. + */ + synth_open = g_new0(V9fsSynthOpenState, 1); synth_open->node = node; node->open_count++; fs->private = synth_open; @@ -220,7 +225,14 @@ static void synth_rewinddir(FsContext *ctx, V9fsFidOpenState *fs) static void synth_direntry(V9fsSynthNode *node, struct dirent *entry, off_t off) { - strcpy(entry->d_name, node->name); + size_t sz = strlen(node->name) + 1; + /* + * 'entry' is always inside of V9fsSynthOpenState which have NAME_MAX + * back padding. Ensure we do not overflow it. + */ + g_assert(sizeof(struct dirent) + NAME_MAX >= + offsetof(struct dirent, d_name) + sz); + memcpy(entry->d_name, node->name, sz); entry->d_ino = node->attr->inode; entry->d_off = off + 1; } @@ -266,7 +278,7 @@ static int synth_open(FsContext *ctx, V9fsPath *fs_path, V9fsSynthOpenState *synth_open; V9fsSynthNode *node = *(V9fsSynthNode **)fs_path->data; - synth_open = g_malloc(sizeof(*synth_open)); + synth_open = g_new0(V9fsSynthOpenState, 1); synth_open->node = node; node->open_count++; fs->private = synth_open; diff --git a/hw/9pfs/9p-synth.h b/hw/9pfs/9p-synth.h index 036d7e4a5b..eeb246f377 100644 --- a/hw/9pfs/9p-synth.h +++ b/hw/9pfs/9p-synth.h @@ -41,6 +41,11 @@ typedef struct V9fsSynthOpenState { off_t offset; V9fsSynthNode *node; struct dirent dent; + /* + * Ensure there is enough space for 'dent' above, some systems have a + * d_name size of just 1, which would cause a buffer overrun. + */ + char dent_trailing_space[NAME_MAX]; } V9fsSynthOpenState; int qemu_v9fs_synth_mkdir(V9fsSynthNode *parent, int mode, diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c index 032cce04c4..c0873bde16 100644 --- a/hw/9pfs/codir.c +++ b/hw/9pfs/codir.c @@ -143,8 +143,7 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState *fidp, } else { e = e->next = g_malloc0(sizeof(V9fsDirEnt)); } - e->dent = g_malloc0(sizeof(struct dirent)); - memcpy(e->dent, dent, sizeof(struct dirent)); + e->dent = qemu_dirent_dup(dent); /* perform a full stat() for directory entry if requested by caller */ if (dostat) { diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index d1660d67fa..ce12f64853 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -805,6 +805,19 @@ static inline int platform_does_not_support_system(const char *command) } #endif /* !HAVE_SYSTEM_FUNCTION */ +/** + * Duplicate directory entry @dent. + * + * It is highly recommended to use this function instead of open coding + * duplication of @c dirent objects, because the actual @c struct @c dirent + * size may be bigger or shorter than @c sizeof(struct dirent) and correct + * handling is platform specific (see gitlab issue #841). + * + * @dent - original directory entry to be duplicated + * @returns duplicated directory entry which should be freed with g_free() + */ +struct dirent *qemu_dirent_dup(struct dirent *dent); + #ifdef __cplusplus } #endif diff --git a/util/osdep.c b/util/osdep.c index 42a0a4986a..67fbf22778 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -33,6 +33,7 @@ extern int madvise(char *, size_t, int); #endif +#include <dirent.h> #include "qemu-common.h" #include "qemu/cutils.h" #include "qemu/sockets.h" @@ -615,3 +616,23 @@ writev(int fd, const struct iovec *iov, int iov_cnt) return readv_writev(fd, iov, iov_cnt, true); } #endif + +struct dirent * +qemu_dirent_dup(struct dirent *dent) +{ + size_t sz = 0; +#if defined _DIRENT_HAVE_D_RECLEN + /* Avoid use of strlen() if platform supports d_reclen. */ + sz = dent->d_reclen; +#endif + /* + * Test sz for zero even if d_reclen is available + * because some drivers may set d_reclen to zero. + */ + if (sz == 0) { + /* Fallback to the most portable way. */ + sz = offsetof(struct dirent, d_name) + + strlen(dent->d_name) + 1; + } + return g_memdup(dent, sz); +}