Address static analyzer complaints in nvpair code and constify string functions #14612

ryao · 2023-03-11T19:03:01Z

Motivation and Context

Both Coverity and Clang's static analyzer each had 3 complaints in the nvpair code. They were mostly false positives, with the exception of a possible NULL pointer dereference reported by Clang's static analyzer. The coverity reports had been marked as false positives, but upon reflection, I have decided to address them by using a C99 variable array member, much like we currently use elsewhere.

Description

The individual commits have descriptions.

A patch constifying nvpair string functions is included to silence compiler complaints about dropping const that the compiler correctly began to make after the adoption of flexible array members. It touches far more than just nvpair_name() mostly because making certain things const required either making others const or adopting typecasts. With the exception of three instances in the ioctl code, we have chosen not to use typecasts.

How Has This Been Tested?

A local build test has been done. The buildbot will do further testing.

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:

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.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

Clang's static analyzer complains that nvs_xdr() and nvs_native() functions return pointers to stack memory. That is technically true, but the pointers are stored in stack memory from the caller's stack frame, are not read by the caller and are deallocated when the caller returns, so this is harmless. We set the pointers to NULL to silence the warnings. Signed-off-by: Richard Yao <[email protected]>

Clang's static analyzer complains about a possible NULL pointer dereference in nvlist_lookup_nvpair_ei_sep() because it unconditionally dereferences a pointer initialized by `nvpair_value_nvlist_array()` under the assumption that `nvpair_value_nvlist_array()` will always initialize the pointer without checking to see if an error was returned to indicate otherwise. This itself is improper error handling, so we fix it. However, fixing it to properly respond to errors is not enough to avoid a NULL pointer dereference, since we can receive NULL when the array is empty, so we also add a NULL check. Signed-off-by: Richard Yao <[email protected]>

The strings returned from parsing nvlists should be immutable, but to simplify the code when we want a substring from it, we sometimes will write a NULL into it and then restore the value afterward. Provided there is no concurrent access, this is okay, unless we forget to restore the value afterward. This was caught when constifying string functions related to nvlists. Signed-off-by: Richard Yao <[email protected]>

discover_cached_paths() will write a NULL into a string from a nvlist to use it as a substring, but does not restore it before return. This corrupts the nvlist. It should be harmless unless the string is needed again later, but we should not do this, so let us fix it. Signed-off-by: Richard Yao <[email protected]>

After addressing coverity complaints involving `nvpair_name()`, the compiler started complaining about dropping const. This lead to a rabbit hole where not only `nvpair_name()` needed to be constified, but also `nvpair_value_string()`, `fnvpair_value_string()` and a few other static functions, plus variable pointers throughout the code. The result became a fairly big change, so it has been split out into its own patch. Signed-off-by: Richard Yao <[email protected]>

Coverity reported possible out-of-bounds reads from doing `((char *)(nvp) + sizeof (nvpair_t))` to get the nvpair name string. These were initially marked as false positives, but since we are now using C99 flexible array members elsewhere, we could use them here too as cleanup to make the code easier to understand. Reported-by: Coverity (CID-977165) Reported-by: Coverity (CID-1524109) Reported-by: Coverity (CID-1524642) Signed-off-by: Richard Yao <[email protected]>

ryao · 2023-03-12T22:57:39Z

The FreeBSD, Fedora 35, CentOS 7 and CentOS 8 buildbots seem to have hit pre-existing issues. I received a suggestion to tackle the array functions alongside this, but I do not have time for those right now, so they will be left as future work.

mcmilk

LGTM - and we get rid of a lot (char *) casts 👍
I will try to test some docker based linux images for further testings within our CI

behlendorf · 2023-03-14T22:23:54Z

include/sys/nvpair.h

@@ -232,7 +232,8 @@ _SYS_NVPAIR_H int nvlist_lookup_int64(const nvlist_t *, const char *,
    int64_t *);
 _SYS_NVPAIR_H int nvlist_lookup_uint64(const nvlist_t *, const char *,
    uint64_t *);
-_SYS_NVPAIR_H int nvlist_lookup_string(nvlist_t *, const char *, char **);
+_SYS_NVPAIR_H int nvlist_lookup_string(const nvlist_t *, const char *,
+    const char **);


These API changes would mean we're diverging from the long standing documented interfaces. Historically we've tried to avoid that kind of thing. However, now that we're converging on a single code base it's a bit less problematic. Given the upside of this change I'm okay with it.

I generally shy away from divergence, but I make an exception for const correctness. Despite libabigail’s claims to the contrary, the binary ABI is the same and programs will still compile when not using -Werror, although they could have warnings.

That said, the only places where we would have warnings are in the f versions of these functions that is not part of the OpenSolaris DDI and the functions who had handles constified. The benefits of constification outweighs the downsides and an earlier commit already constified some of the libnvpair functions.

This could pose a problem when we reunify with illumos, but we can always reverse course then, assuming a compromise where the constification is only done in the headers when a CPP flag is defined cannot be reached. I actually asked them about this while the PR was open and received two responses. A younger illumos developer loved it while an older one was open to it, but was more guarded. He suggested tackling the array functions alongside this and said that they needed to give more thought to this in illumos because it touches a committed interface. Unfortunately, I do not have time in my schedule to tackle the array functions any time soon.

One additional thing to note for future reference is that compilers are always adding new warnings about correctness that similarly break programs built with -Werror. The effect of constification is not dissimilar to them.

Clang's static analyzer complains about a possible NULL pointer dereference in nvlist_lookup_nvpair_ei_sep() because it unconditionally dereferences a pointer initialized by `nvpair_value_nvlist_array()` under the assumption that `nvpair_value_nvlist_array()` will always initialize the pointer without checking to see if an error was returned to indicate otherwise. This itself is improper error handling, so we fix it. However, fixing it to properly respond to errors is not enough to avoid a NULL pointer dereference, since we can receive NULL when the array is empty, so we also add a NULL check. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14612

The strings returned from parsing nvlists should be immutable, but to simplify the code when we want a substring from it, we sometimes will write a NULL into it and then restore the value afterward. Provided there is no concurrent access, this is okay, unless we forget to restore the value afterward. This was caught when constifying string functions related to nvlists. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14612

discover_cached_paths() will write a NULL into a string from a nvlist to use it as a substring, but does not restore it before return. This corrupts the nvlist. It should be harmless unless the string is needed again later, but we should not do this, so let us fix it. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14612

After addressing coverity complaints involving `nvpair_name()`, the compiler started complaining about dropping const. This lead to a rabbit hole where not only `nvpair_name()` needed to be constified, but also `nvpair_value_string()`, `fnvpair_value_string()` and a few other static functions, plus variable pointers throughout the code. The result became a fairly big change, so it has been split out into its own patch. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes #14612

Coverity reported possible out-of-bounds reads from doing `((char *)(nvp) + sizeof (nvpair_t))` to get the nvpair name string. These were initially marked as false positives, but since we are now using C99 flexible array members elsewhere, we could use them here too as cleanup to make the code easier to understand. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Reported-by: Coverity (CID-977165) Reported-by: Coverity (CID-1524109) Reported-by: Coverity (CID-1524642) Closes #14612

Clang's static analyzer complains that nvs_xdr() and nvs_native() functions return pointers to stack memory. That is technically true, but the pointers are stored in stack memory from the caller's stack frame, are not read by the caller and are deallocated when the caller returns, so this is harmless. We set the pointers to NULL to silence the warnings. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14612

Clang's static analyzer complains about a possible NULL pointer dereference in nvlist_lookup_nvpair_ei_sep() because it unconditionally dereferences a pointer initialized by `nvpair_value_nvlist_array()` under the assumption that `nvpair_value_nvlist_array()` will always initialize the pointer without checking to see if an error was returned to indicate otherwise. This itself is improper error handling, so we fix it. However, fixing it to properly respond to errors is not enough to avoid a NULL pointer dereference, since we can receive NULL when the array is empty, so we also add a NULL check. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14612

The strings returned from parsing nvlists should be immutable, but to simplify the code when we want a substring from it, we sometimes will write a NULL into it and then restore the value afterward. Provided there is no concurrent access, this is okay, unless we forget to restore the value afterward. This was caught when constifying string functions related to nvlists. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14612

discover_cached_paths() will write a NULL into a string from a nvlist to use it as a substring, but does not restore it before return. This corrupts the nvlist. It should be harmless unless the string is needed again later, but we should not do this, so let us fix it. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14612

After addressing coverity complaints involving `nvpair_name()`, the compiler started complaining about dropping const. This lead to a rabbit hole where not only `nvpair_name()` needed to be constified, but also `nvpair_value_string()`, `fnvpair_value_string()` and a few other static functions, plus variable pointers throughout the code. The result became a fairly big change, so it has been split out into its own patch. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Closes openzfs#14612

Coverity reported possible out-of-bounds reads from doing `((char *)(nvp) + sizeof (nvpair_t))` to get the nvpair name string. These were initially marked as false positives, but since we are now using C99 flexible array members elsewhere, we could use them here too as cleanup to make the code easier to understand. Reviewed-by: Tino Reichardt <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Richard Yao <[email protected]> Reported-by: Coverity (CID-977165) Reported-by: Coverity (CID-1524109) Reported-by: Coverity (CID-1524642) Closes openzfs#14612