[fuse-devel] segfault due to problem in fuse-3.1.0

Discussion:

Chris Clayton via fuse-devel

2017-07-10 10:02:35 UTC

Hi.

I've built and installed the recent fuse-3.1.0 and sshfs-3.0.0 packages. When I run:

sshfs <user>@<remote-host>:<remote-dir> <mountpoint>

the command fails with a segfault in realloc() in glibc.

Using gdb, I found that when the segfault occurs there are over 32,000 stack frames for fuse_new_30. I confirmed this by
adding a static local variable to add_opt_common() (which calls realloc()) and incrementing it each time the function is
called. At the time of the segfault its value was 65489.

I'm no expert, but I think this is due to the line 'FUSE_SYMVER(".symver fuse_new,fuse_new@@FUSE_3.1");' which
immediately precedes the definition of fuse_new_30() in lib/fuse.c. I think that makes the call to fuse_new() within
fuse_new_30() a recursive call to fuse_new_30(). I've avoided it with this patch:

--- fuse-3.1.0/lib/fuse.c.orig 2017-07-10 10:01:35.359854560 +0100
+++ fuse-3.1.0/lib/fuse.c 2017-07-10 10:01:54.739854165 +0100
@@ -4629,7 +4629,7 @@ void fuse_stop_cleanup_thread(struct fus
/* Emulates 3.0-style fuse_new(), which processes
--help */
FUSE_SYMVER(".symver fuse_new_30,***@FUSE_3.0");
-FUSE_SYMVER(".symver fuse_new,fuse_new@@FUSE_3.1");
+FUSE_SYMVER(".symver fuse_new_31,***@FUSE_3.1");
struct fuse *fuse_new_30(struct fuse_args *args,
const struct fuse_operations *op,
size_t op_size, void *user_data)

Whether it's a correct patch, I'll leave to you experts, but with it applied, the segfault goes away and the mount succeeds.

Hope this helps and thanks for your work on fuse.

Chris

Nikolaus Rath

2017-07-10 10:21:12 UTC

Permalink

stack frames for fuse_new_30. I confirmed this by adding a static
local variable to add_opt_common() (which calls realloc()) and
incrementing it each time the function is called. At the time of the
segfault its value was 65489.
immediately precedes the definition of fuse_new_30() in lib/fuse.c. I think that makes the call to fuse_new() within
--- fuse-3.1.0/lib/fuse.c.orig 2017-07-10 10:01:35.359854560 +0100
+++ fuse-3.1.0/lib/fuse.c 2017-07-10 10:01:54.739854165 +0100
@@ -4629,7 +4629,7 @@ void fuse_stop_cleanup_thread(struct fus
/* Emulates 3.0-style fuse_new(), which processes
--help */
struct fuse *fuse_new_30(struct fuse_args *args,
const struct fuse_operations *op,
size_t op_size, void *user_data)

I assume you also renamed fuse_new() to fuse_new_31()?

I already committed a similar patch to fix a build problem with GCC
4.8. However, that patch seems to make trouble for GCC 7 instead:

https://github.com/libfuse/libfuse/commit/503e32d01e4db00e90d7acfd81ab05386559069f

It's not clear to me at the moment how to best proceed.

Best,
-Nikolaus

--
GPG Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

»Time flies like an arrow, fruit flies like a Banana.«

Chris Clayton via fuse-devel

2017-07-11 05:15:34 UTC

Permalink

Post by Nikolaus Rath

I assume you also renamed fuse_new() to fuse_new_31()?

No, I didn't. I used the phrase "I'm not expert" very deliberately. I don't know why the segfault is avoided by my
patch, but I intend to look at the interaction between the #defines of fuse_new et al in include/fuse.h and the .symver
statements in lib/fuse.c. FUSE_USE_VERSION seems to have the value 31, so I'm not at all sure why fuse_new_30() is being
called. I'll get back when I've figured it out (or admitted defeat).

Cheers,

Chris

Post by Nikolaus Rath
I already committed a similar patch to fix a build problem with GCC
https://github.com/libfuse/libfuse/commit/503e32d01e4db00e90d7acfd81ab05386559069f
It's not clear to me at the moment how to best proceed.
Best,
-Nikolaus

Chris Clayton via fuse-devel

2017-07-12 10:45:26 UTC

Permalink

Post by Chris Clayton via fuse-devel

Post by Nikolaus Rath

I assume you also renamed fuse_new() to fuse_new_31()?

Well, I've waded through the versioned symbols voodoo and I think I've got a legitimate tfix this time. I was right in
my assumption that the segfault was being caused by infinite recursion because the call to fuse_new() from fuse_new_30()
was resolved to fuse_new_30 by ld(1).

The patch below seems to fix the problem:

--- fuse-3.1.0/lib/fuse_versionscript.orig 2017-07-12 10:19:08.807946602 +0100
+++ fuse-3.1.0/lib/fuse_versionscript 2017-07-12 10:19:48.087945800 +0100
@@ -64,7 +64,6 @@ FUSE_3.0 {
fuse_fs_lock;
fuse_fs_mkdir;
fuse_fs_mknod;
- fuse_fs_new;
fuse_fs_open;
fuse_fs_opendir;
fuse_fs_read;
@@ -136,6 +135,7 @@ FUSE_3.1 {
global:
fuse_lib_help;
fuse_new_30;
+ fuse_new_31;
} FUSE_3.0;

# Local Variables:
--- fuse-3.1.0/include/fuse.h.orig 2017-07-11 11:30:04.435821379 +0100
+++ fuse-3.1.0/include/fuse.h 2017-07-11 11:28:40.731823087 +0100
@@ -842,6 +842,8 @@ struct fuse *fuse_new(struct fuse_args *
#endif
struct fuse *fuse_new_30(struct fuse_args *args, const struct fuse_operations *op,
size_t op_size, void *private_data);
+struct fuse *fuse_new_31(struct fuse_args *args, const struct fuse_operations *op,
+ size_t op_size, void *private_data);

/**
* Mount a FUSE file system.
--- fuse-3.1.0/lib/fuse.c.symbols-fix 2017-07-08 11:48:08.000000000 +0100
+++ fuse-3.1.0/lib/fuse.c 2017-07-11 22:41:41.391555268 +0100
@@ -4629,7 +4629,7 @@ void fuse_stop_cleanup_thread(struct fus
/* Emulates 3.0-style fuse_new(), which processes
--help */
FUSE_SYMVER(".symver fuse_new_30,***@FUSE_3.0");
-FUSE_SYMVER(".symver fuse_new,fuse_new@@FUSE_3.1");
+FUSE_SYMVER(".symver fuse_new_31,fuse_new@@FUSE_3.1");
struct fuse *fuse_new_30(struct fuse_args *args,
const struct fuse_operations *op,
size_t op_size, void *user_data)
@@ -4649,10 +4649,10 @@ struct fuse *fuse_new_30(struct fuse_arg
fuse_lib_help(args);
return NULL;
} else
- return fuse_new(args, op, op_size, user_data);
+ return fuse_new_31(args, op, op_size, user_data);
}

-struct fuse *fuse_new(struct fuse_args *args,
+struct fuse *fuse_new_31(struct fuse_args *args,
const struct fuse_operations *op,
size_t op_size, void *user_data)
{

The output from objdump -T on the library shows the effect:

$ objdump -T libfuse3.so.3.1.0 | grep fuse_new
0000000000010ff0 g DF .text 00000000000000c4 FUSE_3.1 fuse_new_30
0000000000010ff0 g DF .text 00000000000000c4 (FUSE_3.0) fuse_new
0000000000010980 g DF .text 0000000000000661 FUSE_3.1 fuse_new_31
0000000000010980 g DF .text 0000000000000661 FUSE_3.1 fuse_new

That seems to be what we want, in particular, ***@FUSE_3.1 is the default used when unversioned references to
fuse_new are resolved by the linker.

One more possible change is to move the prototypes of fuse_new_{30,31} from include/fuse.h to lib/fuse.c because they
are only ever called via their fuse_new() aliases (except, of course) for the call to fuse_new_31() from fuse_new_30()).
In fact, the functions could possibly be made static in lib/fuse.c, because they are only called within that file. I'll
try that out and send a separate patch if it works out.

Comments (on fix or making the functions static)?,

Chris

Post by Chris Clayton via fuse-devel
Cheers,
Chris

Nikolaus Rath

2017-07-12 15:21:33 UTC

Permalink

Post by Chris Clayton via fuse-devel
Well, I've waded through the versioned symbols voodoo and I think I've
got a legitimate tfix this time. I was right in my assumption that the
segfault was being caused by infinite recursion because the call to
fuse_new() from fuse_new_30() was resolved to fuse_new_30 by ld(1).

Thanks for looking into this!

Post by Chris Clayton via fuse-devel
--- fuse-3.1.0/lib/fuse_versionscript.orig 2017-07-12 10:19:08.807946602 +0100
+++ fuse-3.1.0/lib/fuse_versionscript 2017-07-12 10:19:48.087945800 +0100
@@ -64,7 +64,6 @@ FUSE_3.0 {
fuse_fs_lock;
fuse_fs_mkdir;
fuse_fs_mknod;
- fuse_fs_new;
fuse_fs_open;
fuse_fs_opendir;
fuse_fs_read;
@@ -136,6 +135,7 @@ FUSE_3.1 {
fuse_lib_help;
fuse_new_30;
+ fuse_new_31;
} FUSE_3.0;
--- fuse-3.1.0/include/fuse.h.orig 2017-07-11 11:30:04.435821379 +0100
+++ fuse-3.1.0/include/fuse.h 2017-07-11 11:28:40.731823087 +0100
@@ -842,6 +842,8 @@ struct fuse *fuse_new(struct fuse_args *
#endif
struct fuse *fuse_new_30(struct fuse_args *args, const struct fuse_operations *op,
size_t op_size, void *private_data);
+struct fuse *fuse_new_31(struct fuse_args *args, const struct fuse_operations *op,
+ size_t op_size, void *private_data);

The latter is not intended for public use (programs should just use
fuse_new), so this shouldn't be in a public header.

Post by Chris Clayton via fuse-devel
/**
* Mount a FUSE file system.
--- fuse-3.1.0/lib/fuse.c.symbols-fix 2017-07-08 11:48:08.000000000 +0100
+++ fuse-3.1.0/lib/fuse.c 2017-07-11 22:41:41.391555268 +0100
@@ -4629,7 +4629,7 @@ void fuse_stop_cleanup_thread(struct fus
/* Emulates 3.0-style fuse_new(), which processes
--help */
struct fuse *fuse_new_30(struct fuse_args *args,
const struct fuse_operations *op,
size_t op_size, void *user_data)
@@ -4649,10 +4649,10 @@ struct fuse *fuse_new_30(struct fuse_arg
fuse_lib_help(args);
return NULL;
} else
- return fuse_new(args, op, op_size, user_data);
+ return fuse_new_31(args, op, op_size, user_data);
}

I think this is probably the critical part that I overlooked. Would you
like to submit a pull request, so that the fix is properly attributed to
you?

Post by Chris Clayton via fuse-devel
-struct fuse *fuse_new(struct fuse_args *args,
+struct fuse *fuse_new_31(struct fuse_args *args,
const struct fuse_operations *op,
size_t op_size, void *user_data)
{
$ objdump -T libfuse3.so.3.1.0 | grep fuse_new
0000000000010ff0 g DF .text 00000000000000c4 FUSE_3.1 fuse_new_30
0000000000010ff0 g DF .text 00000000000000c4 (FUSE_3.0) fuse_new
0000000000010980 g DF .text 0000000000000661 FUSE_3.1 fuse_new_31
0000000000010980 g DF .text 0000000000000661 FUSE_3.1 fuse_new
fuse_new are resolved by the linker.
One more possible change is to move the prototypes of fuse_new_{30,31}
from include/fuse.h to lib/fuse.c because they are only ever called
via their fuse_new() aliases (except, of course) for the call to
fuse_new_31() from fuse_new_30()). In fact, the functions could
possibly be made static in lib/fuse.c, because they are only called
within that file. I'll try that out and send a separate patch if it
works out.

No. In that case programs written for FUSE 3.0 could no longer be
compiled against FUSE 3.1. Note that if a program defines
FUSE_USE_VERSION 30, then fuse.h turns fuse_new() into a macro that
actually calls fuse_new_30(), so this function needs to be public.

fuse_new_31, however, should indeed become private/static.

Best,
-Nikolaus

--
GPG Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

»Time flies like an arrow, fruit flies like a Banana.«

Nikolaus Rath

2017-07-12 15:24:25 UTC

Permalink

Post by Nikolaus Rath

Post by Chris Clayton via fuse-devel
One more possible change is to move the prototypes of fuse_new_{30,31}
from include/fuse.h to lib/fuse.c because they are only ever called
via their fuse_new() aliases (except, of course) for the call to
fuse_new_31() from fuse_new_30()). In fact, the functions could
possibly be made static in lib/fuse.c, because they are only called
within that file. I'll try that out and send a separate patch if it
works out.

No. In that case programs written for FUSE 3.0 could no longer be
compiled against FUSE 3.1. Note that if a program defines
FUSE_USE_VERSION 30, then fuse.h turns fuse_new() into a macro that
actually calls fuse_new_30(), so this function needs to be public.
fuse_new_31, however, should indeed become private/static.

Actually, it should become *private* (i.e, not declared in public
headers), but not static. If you declare it as static, the linker will
no longer see it an the .symver directive will probably either do
nothing or give an error.

Best,
-Nikolaus

--
GPG Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

»Time flies like an arrow, fruit flies like a Banana.«

Chris Clayton via fuse-devel

2017-07-13 05:59:54 UTC

Permalink

Post by Nikolaus Rath

Thanks for looking into this!

The latter is not intended for public use (programs should just use
fuse_new), so this shouldn't be in a public header.

OK, I'll fix that.

Post by Nikolaus Rath

I think this is probably the critical part that I overlooked. Would you
like to submit a pull request, so that the fix is properly attributed to
you?

Sorry, it will have to be a traditional patch. I can use git for pulling, cloning and bisecting, but beyond that it's
more voodoo! New patch to follow when I've tested it. BTW, do you know of a fuse-based FS that builds against fuse-3.0.x?

Post by Nikolaus Rath

No. In that case programs written for FUSE 3.0 could no longer be
compiled against FUSE 3.1. Note that if a program defines
FUSE_USE_VERSION 30, then fuse.h turns fuse_new() into a macro that
actually calls fuse_new_30(), so this function needs to be public.
fuse_new_31, however, should indeed become private/static.

I'll make it private - I've noted your point from your later message about not making it static.

Thanks.

Chris

Post by Nikolaus Rath
Best,
-Nikolaus

Chris Clayton via fuse-devel

2017-07-13 07:08:18 UTC

Permalink

Post by Chris Clayton via fuse-devel

Post by Nikolaus Rath

Thanks for looking into this!

The latter is not intended for public use (programs should just use
fuse_new), so this shouldn't be in a public header.

OK, I'll fix that.

Post by Nikolaus Rath

I think this is probably the critical part that I overlooked. Would you
like to submit a pull request, so that the fix is properly attributed to
you?

I've now got a fuse-based FS built against fuse-3.0.2. Simple, one-line changes to Makefile.am, configure.ac and sshfs,c
allow sshfs-3.0.0 to build against fuse-3.0.2. Testing continues...

Post by Chris Clayton via fuse-devel

Post by Nikolaus Rath

No. In that case programs written for FUSE 3.0 could no longer be
compiled against FUSE 3.1. Note that if a program defines
FUSE_USE_VERSION 30, then fuse.h turns fuse_new() into a macro that
actually calls fuse_new_30(), so this function needs to be public.
fuse_new_31, however, should indeed become private/static.

I'll make it private - I've noted your point from your later message about not making it static.
Thanks.
Chris

Post by Nikolaus Rath
Best,
-Nikolaus