Skip to content

Fix memory leak in high level API#781

Merged
Nikratio merged 1 commit intolibfuse:masterfrom
matthiasgoergens:matthias/fix-leak
Apr 14, 2023
Merged

Fix memory leak in high level API#781
Nikratio merged 1 commit intolibfuse:masterfrom
matthiasgoergens:matthias/fix-leak

Conversation

@matthiasgoergens
Copy link
Copy Markdown
Contributor

See #780 for context.

Previously, in the high level API if we received a signal between setting up signal handlers and processing INIT, we would leak

$ ./example/hello -s -d -f mountpoint/
[9/9] Linking target example/hello_ll
FUSE library version: 3.14.1
nullpath_ok: 0

=================================================================
==178330==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 352 byte(s) in 1 object(s) allocated from:
    #0 0x7fbb19abf411 in __interceptor_calloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:77
    #1 0x7fbb1a0efd3b in fuse_fs_new ../lib/fuse.c:4814
    #2 0x7fbb1a0f02b5 in fuse_new_31 ../lib/fuse.c:4913
    #3 0x7fbb1a10ec5e in fuse_main_real ../lib/helper.c:345
    #4 0x5625db8ab418 in main ../example/hello.c:176
    #5 0x7fbb1983c78f  (/usr/lib/libc.so.6+0x2378f)

SUMMARY: AddressSanitizer: 352 byte(s) leaked in 1 allocation(s).

That's because fuse_lowlevel.cs fuse_session_destroy would only call the user supplied op.destroy, if INIT had been processed, but the high level API relied on op.destroy to free f->fs.

This patch moves the freeing into fuse_destroy that will always be called by our high-level API.

Previously, in the high level API if we received a signal between
setting up signal handlers and processing INIT, we would leak

```
$ ./example/hello -s -d -f mountpoint/
[9/9] Linking target example/hello_ll
FUSE library version: 3.14.1
nullpath_ok: 0

=================================================================
==178330==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 352 byte(s) in 1 object(s) allocated from:
    #0 0x7fbb19abf411 in __interceptor_calloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:77
    #1 0x7fbb1a0efd3b in fuse_fs_new ../lib/fuse.c:4814
    libfuse#2 0x7fbb1a0f02b5 in fuse_new_31 ../lib/fuse.c:4913
    libfuse#3 0x7fbb1a10ec5e in fuse_main_real ../lib/helper.c:345
    libfuse#4 0x5625db8ab418 in main ../example/hello.c:176
    libfuse#5 0x7fbb1983c78f  (/usr/lib/libc.so.6+0x2378f)

SUMMARY: AddressSanitizer: 352 byte(s) leaked in 1 allocation(s).
```

That's because `fuse_lowlevel.c`s `fuse_session_destroy` would only call
the user supplied `op.destroy`, if INIT had been processed, but the high
level API relied on `op.destroy` to free `f->fs`.

This patch moves the freeing into `fuse_destroy` that will always be
called by our high-level API.
@bsbernd
Copy link
Copy Markdown
Collaborator

bsbernd commented Apr 14, 2023

Hmm, ok, so if the file system allocated something at startup (and not in fs->op.init) and that would still leak. I wonder what is the reason to call se->op.destroy() only when se->got_init() was run.

@matthiasgoergens
Copy link
Copy Markdown
Contributor Author

I wonder what is the reason to call se->op.destroy() only when se->got_init() was run.

I also wondered about this one. But since that is part of the already established, publicly observable behaviour of the low level API, I did not want to change it for this PR.

(I don't know whether it's documented, but it's definitely observable and clients likely rely on this.)

@Nikratio Nikratio merged commit fcd293f into libfuse:master Apr 14, 2023
@bsbernd
Copy link
Copy Markdown
Collaborator

bsbernd commented Apr 14, 2023

Yeah I know, I wonder if we should add a session flag. For sure that is something that could turned around to be the default if there ever should be another incompatible libfuse version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants