libspl's getextmntent() fd leak on Linux, libzfs's obsession with the mtab #11868

nabijaczleweli · 2021-04-08T23:09:25Z

Motivation and Context

Well, one's a leak, and the other's Weird?

Description

See individual commit messages.

This conflicts with #11866 and both will need a rebase if the other is merged.

How Has This Been Tested?

I ran zfs mount and it closed /proc/self/mounts and zfs list and it didn't even touch it.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv) – libzfs but only a private opaque member; also technically changes the ABI of libspl too to no longer leak the FILE*s, but, y'know
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes. – none apply
I have run the ZFS Test Suite with this change applied. – CI take my hand
All commit messages are properly formatted and contain Signed-off-by.

nabijaczleweli · 2021-04-11T19:26:05Z

checkstyle CI failed again, which is fair, that is UB, but I don't get why nothing else triggers this

nabijaczleweli · 2021-04-12T08:16:28Z

Rebased

behlendorf

I'm not sure what's going on with the lint warning either. That's quite strange, but something which will have to be sorted out.

include/libzfs_impl.h

Update the test images link to reference the openzfs github repository. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Issue #11868

behlendorf · 2021-04-12T16:42:03Z

.gitmodules: link to openzfs github user

I've manually cherry picked this unrelated change in to master so you can drop it from this PR.

nabijaczleweli · 2021-04-12T18:06:28Z

Updated to globally inline fopen()/fclose().

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

nabijaczleweli · 2021-04-12T20:54:02Z

I make storeabid and make checkabi passes on my machine now, but CI has a problem with libzutil.h (which last changed in june of last year) for some reason?

behlendorf · 2021-04-12T23:04:18Z

but CI has a problem with libzutil.h

It'd odd this results in an error even after you updated the libzfs.abi file. The ABI change itself is caused by the removal of libzfs_mnttab from the libzfs_handle. It order to avoid the otherwise unnecessary ABI change it might be prudent simply rename .libzfs_mnttab something like .libzfs_pad1; /* previously libzfs_mnttab */.

nabijaczleweli · 2021-04-13T08:51:12Z

I mean, there is no ABI change, really, since all libzfs handles are opaque pointers (alternatively, if you consider that (FILE*)((char*)libzfs_init() + 8) is a valid use of libzfs, then your ABI is already epically owned if you make it padding, since it's all zero now, so freopen()s on it already segfault (musl) or return NULL (glibc), where they'd previously succeed); I'll try regenerating under fresher abigail, maybe.

nabijaczleweli · 2021-04-13T10:00:09Z

Regenerated ABI on sid abigail and it's even more broken, which, lol

All users did a freopen() on it. Even some non-users did! This is point-less ‒ just open the mtab when needed If I understand Solaris' getextmntent(3C) correctly, the non-user freopen()s are very likely an odd, twisted vestigial tail of that ‒ but it's got a completely different calling convention and caching semantics than any platform we support Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

nabijaczleweli · 2021-04-13T10:19:48Z

Did a full clean/regen/rebuild cycle and now the ABI is Okay apparently, abigail never ceases to amaze.

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

All users did a freopen() on it. Even some non-users did! This is point-less ‒ just open the mtab when needed If I understand Solaris' getextmntent(3C) correctly, the non-user freopen()s are very likely an odd, twisted vestigial tail of that ‒ but it's got a completely different calling convention and caching semantics than any platform we support Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes #11868

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes #11868

Update the test images link to reference the openzfs github repository. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Issue openzfs#11868

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes openzfs#11868

Update the test images link to reference the openzfs github repository. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Issue openzfs#11868

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes openzfs#11868

All users did a freopen() on it. Even some non-users did! This is point-less ‒ just open the mtab when needed If I understand Solaris' getextmntent(3C) correctly, the non-user freopen()s are very likely an odd, twisted vestigial tail of that ‒ but it's got a completely different calling convention and caching semantics than any platform we support Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes openzfs#11868

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes openzfs#11868

Update the test images link to reference the openzfs github repository. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Issue openzfs#11868

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes openzfs#11868

nabijaczleweli changed the title ~~libspl's getextmntent() fd leak on Linux, libzfs obsession with the mtab~~ libspl's getextmntent() fd leak on Linux, libzfs's obsession with the mtab Apr 8, 2021

nabijaczleweli force-pushed the nomntent branch from 94a10f3 to c7f0b25 Compare April 11, 2021 19:12

behlendorf added the Status: Code Review Needed Ready for review and testing label Apr 11, 2021

nabijaczleweli force-pushed the nomntent branch from c7f0b25 to e881610 Compare April 12, 2021 08:12

behlendorf reviewed Apr 12, 2021

View reviewed changes

include/libzfs_impl.h Outdated Show resolved Hide resolved

nabijaczleweli force-pushed the nomntent branch from e881610 to dbdd943 Compare April 12, 2021 17:59

nabijaczleweli force-pushed the nomntent branch from dbdd943 to e7287a5 Compare April 12, 2021 18:34

linux/libspl: getextmntent(): don't leak mnttab FILE*

65bdf33

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

behlendorf approved these changes Apr 12, 2021

View reviewed changes

nabijaczleweli force-pushed the nomntent branch from e7287a5 to dfd1469 Compare April 13, 2021 09:46

nabijaczleweli force-pushed the nomntent branch from b971c73 to 45ae60c Compare April 13, 2021 10:19

libzfs: get rid of unused libzfs_handle::libzfs_{storeerr,chassis_id}

02651fb

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Apr 13, 2021

behlendorf closed this in 74e48f4 Apr 13, 2021

behlendorf pushed a commit that referenced this pull request Apr 13, 2021

libzfs: get rid of unused libzfs_handle::libzfs_{storeerr,chassis_id}

d197a15

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes #11868

mcmilk pushed a commit to mcmilk/zfs that referenced this pull request Apr 15, 2021

linux/libspl: getextmntent(): don't leak mnttab FILE*

421503a

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Closes openzfs#11868

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libspl's getextmntent() fd leak on Linux, libzfs's obsession with the mtab #11868

libspl's getextmntent() fd leak on Linux, libzfs's obsession with the mtab #11868

nabijaczleweli commented Apr 8, 2021 •

edited

Loading

nabijaczleweli commented Apr 11, 2021

nabijaczleweli commented Apr 12, 2021

behlendorf left a comment

behlendorf commented Apr 12, 2021

nabijaczleweli commented Apr 12, 2021

nabijaczleweli commented Apr 12, 2021 •

edited

Loading

behlendorf commented Apr 12, 2021

nabijaczleweli commented Apr 13, 2021 •

edited

Loading

nabijaczleweli commented Apr 13, 2021

nabijaczleweli commented Apr 13, 2021 •

edited

Loading

libspl's getextmntent() fd leak on Linux, libzfs's obsession with the mtab #11868

libspl's getextmntent() fd leak on Linux, libzfs's obsession with the mtab #11868

Conversation

nabijaczleweli commented Apr 8, 2021 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

nabijaczleweli commented Apr 11, 2021

nabijaczleweli commented Apr 12, 2021

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf commented Apr 12, 2021

nabijaczleweli commented Apr 12, 2021

nabijaczleweli commented Apr 12, 2021 • edited Loading

behlendorf commented Apr 12, 2021

nabijaczleweli commented Apr 13, 2021 • edited Loading

nabijaczleweli commented Apr 13, 2021

nabijaczleweli commented Apr 13, 2021 • edited Loading

nabijaczleweli commented Apr 8, 2021 •

edited

Loading

nabijaczleweli commented Apr 12, 2021 •

edited

Loading

nabijaczleweli commented Apr 13, 2021 •

edited

Loading

nabijaczleweli commented Apr 13, 2021 •

edited

Loading