Re: libfuse: fuse.c null checks and others

[Martin Pieuchot]

On 28/10/17(Sat) 13:57, Helg Bredow wrote:
> [...] 
> 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 agree it's incorrect.  But sometimes correct changes introduce
regressions.  So it's simpler to not mix such change with a bigger
one like the NULL checks in case we need to revert them.

> 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.

I'd be happy to see you commit these changes now that the NULL checks
are in :)

> > 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.

The nice thing is that we can do test-driven development using the linux
version as reference (8

> > Could you provide a diff including only the NULL check and the removal
> > of "unused" markers?
> > 
> 
> Updated patch is below.

That's in now.

Re: libfuse: fuse.c null checks and others

[Helg Bredow]

On Sat, 28 Oct 2017 09:24:55 +0200
Martin Pieuchot  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 -  1.31
+++ fuse.c  28 Oct 2017 13:39:26 -
@@ -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;

Re: libfuse: fuse.c null checks and others

[Martin Pieuchot]

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.

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.

Could you provide a diff including only the NULL check and the removal
of "unused" markers?

> Index: fuse.c
> ===
> RCS file: /cvs/src/lib/libfuse/fuse.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 fuse.c
> --- fuse.c25 Oct 2017 09:29:46 -  1.31
> +++ fuse.c25 Oct 2017 12:54:48 -
> @@ -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);
>  }
>  
> @@ -227,17 +236,20 @@ fuse_get_session(struct fuse *f)
>  int
>  fuse_loop_mt(unused struct fuse *fuse)
>  {
> - return (0);
> + return (-1);
>  }
>  
>  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,7 +337,7 @@ fuse_remove_signal_handlers(unused struc
>  }
>  
>  int
> -fuse_set_signal_handlers(unused struct fuse_session *se)
> +fuse_set_signal_handlers(struct fuse_session *se)
>  {
>   sigse = se;
>   signal(SIGHUP, ifuse_get_signal);
> @@ -344,7 +359,7 @@ dump_help(void)
>  static void
>  dump_version(void)
>  {
> - fprintf(stderr, "FUSE library version %i\n", FUSE_USE_VERSION);
> + fprintf(stderr, "FUSE library version %i\n", FUSE_VERSION);
>  }
>  
>  static int
> @@ -453,6 +468,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 +518,8 @@ int
>  fuse_main(int argc, char **argv, const struct fuse_operations *ops, void 
> *data)
>  {
>   

libfuse: fuse.c null checks and others

[Helg Bredow]

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?


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 -  1.31
+++ fuse.c  25 Oct 2017 12:54:48 -
@@ -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);
 }
 
@@ -227,17 +236,20 @@ fuse_get_session(struct fuse *f)
 int
 fuse_loop_mt(unused struct fuse *fuse)
 {
-   return (0);
+   return (-1);
 }
 
 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,7 +337,7 @@ fuse_remove_signal_handlers(unused struc
 }
 
 int
-fuse_set_signal_handlers(unused struct fuse_session *se)
+fuse_set_signal_handlers(struct fuse_session *se)
 {
sigse = se;
signal(SIGHUP, ifuse_get_signal);
@@ -344,7 +359,7 @@ dump_help(void)
 static void
 dump_version(void)
 {
-   fprintf(stderr, "FUSE library version %i\n", FUSE_USE_VERSION);
+   fprintf(stderr, "FUSE library version %i\n", FUSE_VERSION);
 }
 
 static int
@@ -453,6 +468,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 +518,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), , , data);
+   fuse = fuse_setup(argc, argv, ops, sizeof(*ops), NULL, NULL, data);
if (!fuse)
return (-1);