zfs

zfs copied to clipboard

Motivation and Context

Coverity found a bug in zfs_secpolicy_create_clone() where it is possible for us to pass an unterminated string when zfs_get_parent() returns an error. Upon inspection, it is clear that using strlcpy() would have avoided this issue.

Description

Looking at the codebase, there are a number of other uses of strncpy() that are unsafe and even when it is used safely, switching to strlcpy() would make the code more readable. Therefore, we switch all instances where we use strncpy() to use strlcpy().

This patch does not tackle the related problem of strcpy(), which is even less safe. Thankfully, a quick inspection found that it is used far more correctly than strncpy() was used. A quick inspection did not find any problems with strcpy() usage outside of zhack, but it should be said that I only checked around 90% of them.

How Has This Been Tested?

A build test on Gentoo Linux was done.

Types of changes

• [x] 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)
• [x] 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:

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

06:03:23.01 cannot create 'testpool/testvol': no such pool 'testpoo'

Well that's strange.

@freqlabs I have revised this to handle the difference in how strlcpy() and strncpy() count. In some cases, strncpy() was passed the field length without a defensive '\0' placed at the end of the destination to handle the case that it stopped copying before the end of the string. Those cases are potential bugs, so I did not increment the field length by 1. I also defensively made the length MIN(dest_length, old_strncpy_len + 1) in a number of places where strncpy() was used to copy substrings unless an explicit length check was done (for the sake of returning ENAMETOOLONG). Whenever possible, I switched to using sizeof(string) when the field length is hard coded (e.g. char string[50]). Lastly, I lengthened some fields in the userland version of kstat_t by 1 to match their in-kernel lengths.

Since these are all (but one!) of the remaining uses of strncpy I'd suggest we add a new deprecation warning in config/Rules.am to make sure new code used strlcpy.

If we are going to go that far, we might as well remove the last remaining usage like I described earlier:

Future cleanup could remove that remaining strncpy() and add a check to cstyle.pl to prevent use of strncpy(), which is a source of subtle bugs.

The reason I did not do it was because I needed to link to libzfs_core to get strlcpy() on Linux systems. The draid test links to libzpool through this:

%C%_draid_LDADD = \
        libzpool.la \
        libnvpair.la

libzpool links to libzfs, which links to libzfs_core and thus strlcpy() is avaliable.

When I try to do the same with zfs_diff-socket, it looks like this:

%C%_zfs_diff-socket_LDADD = \
        libzfs_core.la

Then autotools throws an error because of the -. I was not sure how to rename this, so I left it alone. Input on a good naming convention that doesn't upset autotools would be appreciated.

On second thought, I will give changing - to _ later today a try. There is similar naming used in Gentoo's package manager for git repository variables and it is very confusing when the variable uses _ as well as the thing substituted into it, but perhaps that will work.

If we are going to go that far, we might as well remove the last remaining usage like I described earlier:

Since this last instance is in a test case only used by the test suite I don't think we need to jump through any complicated hoops to fix it. We could just suppress that one warning.

If we are going to go that far, we might as well remove the last remaining usage like I described earlier:

Since this last instance is in a test case only used by the test suite I don't think we need to jump through any complicated hoops to fix it. We could just suppress that one warning.

Alright. In either case, I am probably not going to be able to tackle another revision to this today. I will probably tackle it tomorrow morning before you get into your office.

I added the deprecation warning in config/Rules.am and suppressed it in the test case that uses it. That was easier than I expected. :)

The Linux kernel encourages to use strscpy() only, in favor of n/l and all other flavours specifying string length. So it must be at least some kind of an OS-dependent macro, which would resolve to strscpy() on Linux when present.

The Linux kernel encourages to use strscpy() only, in favor of n/l and all other flavours specifying string length. So it must be at least some kind of an OS-dependent macro, which would resolve to strscpy() on Linux when present.

The reasons to use strscpy() are documented here:

https://www.kernel.org/doc/htmldocs/kernel-api/API-strscpy.html

It is not a drop in replacement for strlcpy() due the the different return value and having different functions between kernels or even the kernel and userspace is undesireable from the perspective of being able to find bugs in ztest and friends.

I am inclined to use strlcpy(). The only reason this is a draft is that I have not yet figured out a sane way to work around the Clang bug on FreeBSD.

I converted the last strncpy() invocation to use strlcpy() via a CPP hack. This fixed the build regression on FreeBSD.