Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FreeBSD: Fix leaked strings in libspl mnttab #12961

Merged
merged 1 commit into from
Jan 14, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jan 11, 2022

Motivation and Context

The FreeBSD implementations of various libspl functions for getting
mounted device informantion were found to leak several strings which
were being allocated in statfs2mnttab but never freed.

The Solaris getmntany(3C) and related interfaces are expected to return
strings residing in static buffers that need to be copied rather than
freed by the caller.

Description

Use static thread-local storage to stash the mnttab structure strings
from FreeBSD's statfs info rather than strings allocated on the heap by
strdup(3).

While here, remove some stray commented out lines.

How Has This Been Tested?

Ran full ZTS and passed on FreeBSD.

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

@ghost ghost requested review from amotin and rincebrain January 11, 2022 17:55
@ghost ghost added Component: Userspace user space functionality Status: Code Review Needed Ready for review and testing labels Jan 11, 2022
@ghost ghost force-pushed the libspl-mnttab-leak branch from 9cae5d7 to db9c482 Compare January 11, 2022 18:06
@rkojedzinszky
Copy link
Contributor

rkojedzinszky commented Jan 11, 2022

What if the cache is being used ? Wouldn't the local buffer be clobbered? Also, there are calls to free() those pointers in cache cleanup code.
I mean the code path here: https://github.com/openzfs/zfs/blob/master/lib/libzfs/libzfs_dataset.c#L896

this will populate an internal cache, rewrting the local buffer, and also, upon exit, it will free() the entries in https://github.com/openzfs/zfs/blob/master/lib/libzfs/libzfs_dataset.c#L848

@rkojedzinszky
Copy link
Contributor

What if the cache is being used ? Wouldn't the local buffer be clobbered? Also, there are calls to free() those pointers in cache cleanup code. I mean the code path here: https://github.com/openzfs/zfs/blob/master/lib/libzfs/libzfs_dataset.c#L896

this will populate an internal cache, rewrting the local buffer, and also, upon exit, it will free() the entries in https://github.com/openzfs/zfs/blob/master/lib/libzfs/libzfs_dataset.c#L848

Sorry, it seems that I was too quick reading the code. During cache fill, the strings are strdup()-ed, so in the cache they are safe to use.

lib/libspl/os/freebsd/mnttab.c Outdated Show resolved Hide resolved
lib/libspl/os/freebsd/mnttab.c Outdated Show resolved Hide resolved
The FreeBSD implementations of various libspl functions for getting
mounted device informantion were found to leak several strings which
were being allocated in statfs2mnttab but never freed.

The Solaris getmntany(3C) and related interfaces are expected to return
strings residing in static buffers that need to be copied rather than
freed by the caller.

Use static thread-local storage to stash the mnttab structure strings
from FreeBSD's statfs info rather than strings allocated on the heap by
strdup(3).

While here, remove some stray commented out lines.

Signed-off-by: Ryan Moeller <[email protected]>
@ghost ghost force-pushed the libspl-mnttab-leak branch from db9c482 to 1e6faf6 Compare January 12, 2022 18:52
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jan 14, 2022
@behlendorf behlendorf merged commit 69c5e69 into openzfs:master Jan 14, 2022
@ghost ghost deleted the libspl-mnttab-leak branch January 14, 2022 21:54
ghost pushed a commit to truenas/zfs that referenced this pull request Jan 19, 2022
The FreeBSD implementations of various libspl functions for getting
mounted device information were found to leak several strings which
were being allocated in statfs2mnttab but never freed.

The Solaris getmntany(3C) and related interfaces are expected to return
strings residing in static buffers that need to be copied rather than
freed by the caller.

Use static thread-local storage to stash the mnttab structure strings
from FreeBSD's statfs info rather than strings allocated on the heap by
strdup(3).

While here, remove some stray commented out lines.

Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Rich Ercolani <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#12961
ghost pushed a commit to truenas/zfs that referenced this pull request Jan 20, 2022
The FreeBSD implementations of various libspl functions for getting
mounted device information were found to leak several strings which
were being allocated in statfs2mnttab but never freed.

The Solaris getmntany(3C) and related interfaces are expected to return
strings residing in static buffers that need to be copied rather than
freed by the caller.

Use static thread-local storage to stash the mnttab structure strings
from FreeBSD's statfs info rather than strings allocated on the heap by
strdup(3).

While here, remove some stray commented out lines.

Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Rich Ercolani <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#12961
ghost pushed a commit to truenas/zfs that referenced this pull request Feb 2, 2022
The FreeBSD implementations of various libspl functions for getting
mounted device information were found to leak several strings which
were being allocated in statfs2mnttab but never freed.

The Solaris getmntany(3C) and related interfaces are expected to return
strings residing in static buffers that need to be copied rather than
freed by the caller.

Use static thread-local storage to stash the mnttab structure strings
from FreeBSD's statfs info rather than strings allocated on the heap by
strdup(3).

While here, remove some stray commented out lines.

Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Rich Ercolani <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#12961
tonyhutter pushed a commit that referenced this pull request Feb 3, 2022
The FreeBSD implementations of various libspl functions for getting
mounted device information were found to leak several strings which
were being allocated in statfs2mnttab but never freed.

The Solaris getmntany(3C) and related interfaces are expected to return
strings residing in static buffers that need to be copied rather than
freed by the caller.

Use static thread-local storage to stash the mnttab structure strings
from FreeBSD's statfs info rather than strings allocated on the heap by
strdup(3).

While here, remove some stray commented out lines.

Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Rich Ercolani <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes #12961
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
The FreeBSD implementations of various libspl functions for getting
mounted device information were found to leak several strings which
were being allocated in statfs2mnttab but never freed.

The Solaris getmntany(3C) and related interfaces are expected to return
strings residing in static buffers that need to be copied rather than
freed by the caller.

Use static thread-local storage to stash the mnttab structure strings
from FreeBSD's statfs info rather than strings allocated on the heap by
strdup(3).

While here, remove some stray commented out lines.

Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Rich Ercolani <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#12961
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
The FreeBSD implementations of various libspl functions for getting
mounted device information were found to leak several strings which
were being allocated in statfs2mnttab but never freed.

The Solaris getmntany(3C) and related interfaces are expected to return
strings residing in static buffers that need to be copied rather than
freed by the caller.

Use static thread-local storage to stash the mnttab structure strings
from FreeBSD's statfs info rather than strings allocated on the heap by
strdup(3).

While here, remove some stray commented out lines.

Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Rich Ercolani <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#12961
bsdjhb pushed a commit to CTSRD-CHERI/zfs that referenced this pull request Jul 13, 2023
This is not associated with a specific upstream commit but apparently
a local diff applied as part of:

commit e92ffd9b626833ebdbf2742c8ffddc6cd94b963e
Merge: 3c3df3660072 17b2ae0
Author: Martin Matuska <[email protected]>
Date:   Sat Jan 22 23:05:15 2022 +0100

    zfs: merge openzfs/zfs@17b2ae0b2 (master) into main

    Notable upstream pull request merges:
      openzfs#12766 Fix error propagation from lzc_send_redacted
      openzfs#12805 Updated the lz4 decompressor
      openzfs#12851 FreeBSD: Provide correct file generation number
      openzfs#12857 Verify dRAID empty sectors
      openzfs#12874 FreeBSD: Update argument types for VOP_READDIR
      openzfs#12896 Reduce number of arc_prune threads
      openzfs#12934 FreeBSD: Fix zvol_*_open() locking
      openzfs#12947 lz4: Cherrypick fix for CVE-2021-3520
      openzfs#12961 FreeBSD: Fix leaked strings in libspl mnttab
      openzfs#12964 Fix handling of errors from dmu_write_uio_dbuf() on FreeBSD
      openzfs#12981 Introduce a flag to skip comparing the local mac when raw sending
      openzfs#12985 Avoid memory allocations in the ARC eviction thread

    Obtained from:  OpenZFS
    OpenZFS commit: 17b2ae0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Userspace user space functionality Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants