On Sat, 28 Oct 2017 09:24:55 +0200
Martin Pieuchot <[email protected]> wrote:
> On 25/10/17(Wed) 13:27, Helg Bredow wrote:
> > I've included different minor patches below as one patch. I haven't split
> > into separate patches since the changes are not complex and easy to audit.
> >
> > Here's what it does:
> >
> > Almost all functions in fuse.c do not check if the arguments are null. This
> > patch adds null checks where appropriate.
> >
> > Some arguments are incorrectly flagged as unused. Delete "unused" if the
> > argument is used in the function.
> >
> > The wrong version macro is used in dump_version() and is inconsistent with
> > that used in fuse_version(). FUSE_USE_VERSION is intended to be defined by
> > file system to specify which backward compatible version of libfuse they
> > require.
> >
> > fuse_loop_mt() is not implemented so return -1 rather than success. If a
> > file system tries to call it then it should be obvious that it's not going
> > to work.
> >
> > fuse_main() declared redundant variables due to the lack of NULL checks
> > before assignment. We now check for NULL so can safely pass NULL instead.
> >
> > Tested with a regression test that passes all NULL arguments to exported
> > functions.
> >
> > Something to consider is that fuse_is_lib_option() is deprecated. Should we
> > remove it from libfuse to stay strictly at version 26?
>
> Testing for NULL is good. However returning -1 in fuse_loop_mt() and
> changing the version number might break ports. So if these changes go
> in, you should watch for regression on ports@ at least.
I've reverted fuse_loop_mt(). However, I don't feel that this is correct. If a
file system expects it to work then it should fail to make it clear that the
functionality is not implemented. sshfs calls it but because
fuse_parse_cmdline() always returns 0 for multithreaded it never gets called
(you normally need to specify -s on the command line for multithreaded fuse
file systems to run in a single thread. I've tested to see what sshfs does when
fuse_loop_mt() returns -1 and it simply exits with no message, just as it does
when it returns 0.
I also reverted the version macro change. The value for both macros is the same
(26) and they will likely stay in step if the version is incremented. It's not
using the correct macro though.
>
> We can remove fuse_is_lib_option() if nothing in ports use it. A good
> start to search for it is codesearch.debian.net. You can also ask
> porters to do a grep on unpacked port tree.
>
I didn't know about this site, it will be useful. I was going to search the
ports source tree but this saves me the trouble and also expands the search.
sshfs calls this fuse_is_lib_option() so it will have to stay. However, it also
needs work as it doesn't behave the same as the Linux version.
> Could you provide a diff including only the NULL check and the removal
> of "unused" markers?
>
Updated patch is below. I had missed a NULL check in fuse_set_signal_handlers()
so have added it. The signal handling only works if the file system is not
busy, otherwise it becomes very difficult to kill. fuse_loop() also doesn't set
the signal handlers but it should. More things to fix later.
Index: fuse.c
===================================================================
RCS file: /cvs/src/lib/libfuse/fuse.c,v
retrieving revision 1.31
diff -u -p -r1.31 fuse.c
--- fuse.c 25 Oct 2017 09:29:46 -0000 1.31
+++ fuse.c 28 Oct 2017 13:39:26 -0000
@@ -71,6 +71,9 @@ fuse_loop(struct fuse *fuse)
ssize_t n;
int ret;
+ if (fuse == NULL)
+ return (-1);
+
fuse->fc->kq = kqueue();
if (fuse->fc->kq == -1)
return (-1);
@@ -156,6 +159,9 @@ fuse_mount(const char *dir, unused struc
struct fuse_chan *fc;
const char *errcause;
+ if (dir == NULL)
+ return (NULL);
+
fc = calloc(1, sizeof(*fc));
if (fc == NULL)
return (NULL);
@@ -197,9 +203,9 @@ bad:
}
void
-fuse_unmount(const char *dir, unused struct fuse_chan *ch)
+fuse_unmount(const char *dir, struct fuse_chan *ch)
{
- if (ch->dead)
+ if (ch == NULL || ch->dead)
return;
if (unmount(dir, MNT_UPDATE) == -1)
@@ -207,7 +213,7 @@ fuse_unmount(const char *dir, unused str
}
int
-fuse_is_lib_option(unused const char *opt)
+fuse_is_lib_option(const char *opt)
{
return (fuse_opt_match(fuse_core_opts, opt));
}
@@ -215,6 +221,9 @@ fuse_is_lib_option(unused const char *op
int
fuse_chan_fd(struct fuse_chan *ch)
{
+ if (ch == NULL)
+ return (-1);
+
return (ch->fd);
}
@@ -233,11 +242,14 @@ fuse_loop_mt(unused struct fuse *fuse)
struct fuse *
fuse_new(struct fuse_chan *fc, unused struct fuse_args *args,
const struct fuse_operations *ops, unused size_t size,
- unused void *userdata)
+ void *userdata)
{
struct fuse *fuse;
struct fuse_vnode *root;
+ if (fc == NULL || ops == NULL)
+ return (NULL);
+
if ((fuse = calloc(1, sizeof(*fuse))) == NULL)
return (NULL);
@@ -275,8 +287,11 @@ fuse_daemonize(unused int foreground)
}
void
-fuse_destroy(unused struct fuse *f)
+fuse_destroy(struct fuse *f)
{
+ if (f == NULL)
+ return;
+
close(f->fc->fd);
free(f->fc->dir);
free(f->fc);
@@ -322,8 +337,11 @@ fuse_remove_signal_handlers(unused struc
}
int
-fuse_set_signal_handlers(unused struct fuse_session *se)
+fuse_set_signal_handlers(struct fuse_session *se)
{
+ if (se == NULL)
+ return -1;
+
sigse = se;
signal(SIGHUP, ifuse_get_signal);
signal(SIGINT, ifuse_get_signal);
@@ -453,6 +471,9 @@ fuse_version(void)
void
fuse_teardown(struct fuse *fuse, char *mp)
{
+ if (fuse == NULL || mp == NULL)
+ return;
+
fuse_unmount(mp, fuse->fc);
fuse_destroy(fuse);
}
@@ -500,10 +521,8 @@ int
fuse_main(int argc, char **argv, const struct fuse_operations *ops, void *data)
{
struct fuse *fuse;
- char *mp = NULL;
- int mt;
- fuse = fuse_setup(argc, argv, ops, sizeof(*ops), &mp, &mt, data);
+ fuse = fuse_setup(argc, argv, ops, sizeof(*ops), NULL, NULL, data);
if (!fuse)
return (-1);
