and add nm_utils_secret_key_get() to cache the secret-key, to only
obtain it once.
nm_utils_secret_key_read() is not expected to fail. However, in case
of an unexpected error, don't propagate the error to the caller,
but instead handle it internally.
That means, in case of error:
- log a warning within nm_utils_secret_key_read() itself.
- always return a generated secret-key. In case of error, the
key won't be persisted (obviously). But the caller can ignore
the error and just proceed with an in-memory key.
Hence, also add nm_utils_secret_key_get() to cache the key. This way,
we only try to actually generate/read the secret-key once. Since that
might fail and return an in-memory key, we must for future invocations
return the same key, without generating a new one.
The secret_key is binary random data, so one shouldn't ever use it as a
NUL terminated string directly.
Still, just ensure that the entire buffer is always NUL terminated.
Coccinelle:
@@
expression a, b;
@@
-a ? a : b
+a ?: b
Applied with:
spatch --sp-file ternary.cocci --in-place --smpl-spacing --dir .
With some manual adjustments on spots that Cocci didn't catch for
reasons unknown.
Thanks to the marvelous effort of the GNU compiler developer we can now
spare a couple of bits that could be used for more important things,
like this commit message. Standards commitees yet have to catch up.
No longer rely on nm_connection_get_path() being meaningful in server.
It also was wrong. During update, nm_settings_connection_update()
would call
nm_utils_log_connection_diff (replace_connection, NM_CONNECTION (self), ...
where replace_connection has no path set, and nothing was logged.
Fix it, by explicitly passing the D-Bus path. Also, because
nm-core-utils.c should be independent of nm-dbus-object.h.
nm_utils_lifetime_get() already has so many arguments.
Essentially, the function returned %TRUE if and only if the
lifetime was greater then zero.
Combine the return value and the output argument for the lifetime.
It also matches better the function name: to get the lifetime.
Similarly to what systemd-resolved does, introduce the concept of
"routing" domain, which is a domain in the search list that is used
only to decide the interface over which a query must be forwarded, but
is not used to complete unqualified host names. Routing domains are
those starting with a tilde ('~') before the actual domain name.
Domains without the initial tilde are used both for completing
unqualified names and for the routing decision.
31. NetworkManager-1.9.2/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c:974:
uninit_use_in_call: Using uninitialized value "contents_rest" when
calling "__strtok_r_1c".
33. NetworkManager-1.9.2/src/nm-core-utils.c:1957:
uninit_use: Using uninitialized value "s".
148. NetworkManager-1.9.2/src/nm-core-utils.c:1924:
uninit_use_in_call: Using uninitialized value "s" when calling
"nm_strstrip_avoid_copy".
30. NetworkManager-1.9.2/src/settings/plugins/keyfile/nms-keyfile-writer.c:218:
check_return: Calling "g_mkdir_with_parents" without checking return
value (as is done elsewhere 4 out of 5
times).
25. NetworkManager-1.9.2/src/platform/nm-linux-platform.c:3969:
check_return: Calling "_nl_send_nlmsg" without checking return value (as
is done elsewhere 4 out of 5 times).
34. NetworkManager-1.9.2/src/nm-core-utils.c:2843:
negative_returns: "fd2" is passed to a parameter that cannot be negative.
26. NetworkManager-1.9.2/src/devices/wwan/nm-modem-broadband.c:897:
check_return: Calling "nm_utils_parse_inaddr_bin" without checking
return value (as is done elsewhere 4 out of 5 times).
3. NetworkManager-1.9.2/src/devices/bluetooth/nm-bluez5-manager.c:386:
check_return: Calling "g_variant_lookup" without checking return value
(as is done elsewhere 79 out of 83 times).
16. NetworkManager-1.9.2/libnm-util/nm-setting.c:405:
check_return: Calling "nm_g_object_set_property" without checking return
value (as is done elsewhere 4 out of 5 times).
and nm_utils_ip6_property_path(). The API with static buffers
looks a bit nicer. But I think they are dangerous, because
we tend to pass the buffer down several layers of the stack, and
it's not immediately clear, that we don't overwrite the static
buffer again (which we probably did not, but it's hard to verify
that there is no bug there).
In many scenarios, we have no use for the file descriptor
after nm_utils_fd_get_contents(). We just want to read it
and close it.
API wise, it would be nice that the get_contents() function never
closes the passed in fd and it's always responsibility of the caller.
However, that costs an additional dup() syscall that could
be avoided, if we allow the function to (optionally) close
the file descriptor.
The function should not close the input file descriptor; however
fdopen() associates the fd to the new stream so that when the stream
is closed, the fd is too. The result is a double close() and the
second call can in certain cases affect a wrong fd.
Use a duplicate fd for the stream.
Fixes: 1d9bdad1dfhttps://bugzilla.redhat.com/show_bug.cgi?id=1451236
By using a macro, we don't cast all the types to guint. Instead,
we use their native types directly. Hence, we don't need
nm_hash_update_uint64() nor nm_hash_update_ptr().
Also, for types smaller then guint like char, we save hashing
the all zero bytes.
The privious NM_HASH_* macros directly operated on a guint value
and were thus close to the actual implementation.
Replace them by adding a NMHashState struct and accessors to
update the hash state. This hides the implementation better
and would allow us to carry more state. For example, we could
switch to siphash24() transparently.
For now, we still do a form basically djb2 hashing, albeit with
differing start seed.
Also add nm_hash_str() and nm_str_hash():
- nm_hash_str() is our own string hashing implementation
- nm_str_hash() is our own string implementation, but with a
GHashFunc signature, suitable to pass it to g_hash_table_new().
Also, it has this name in order to remind you of g_str_hash(),
which it is replacing.
"nm-utils/nm-shared-utils.h" shall contain utility function without other
dependencies. It is intended to be used by other projects as-is.
nm_utils_random_bytes() requires getrandom() and a HAVE_GETRANDOM configure
check. That makes it more cumbersome to re-use "nm-shared-utils.h", in
cases where you don't care about nm_utils_random_bytes().
Split nm_utils_random_bytes() out to a separate file.
Same for hash utils, which depend on nm_utils_random_bytes(). Also, hash
utils will eventually be extended to use siphash24.
Introduce a NM_HASH_INIT() function. It makes the places
where we initialize a hash with a certain seed visually clear.
Also, move them from "shared/nm-utils/nm-shared-utils.h" to
"shared/nm-utils/nm-macros-internal.h". We might want to
have NM_HASH_INIT() non-inline (hence, define it in the
source file).
Add a new function nm_utils_random_bytes().
This function now preferably uses getrandom() syscall if it is
available.
As fallback, it always tries to fill the buffer from /dev/urandom.
If it cannot, as last fallback it uses GRand, which cannot fail.
Hence, the function always sets some (pseudo) random bytes.
It also returns FALSE if the obtained bytes are possibly not good
randomness.
We encounter the same enum in 3 forms:
- NMNDiscPreference in NetworkManager
- "enum ndp_route_preference" in <ndp.h>
- ICMPV6_ROUTER_PREF_* in <linux/icmpv6.h>
Move our enum to nm-core-utils.h, so that it can be used
by platform code as well (platform code should not include
ndisc/nm-ndisc.h).
Also, NMNDiscPreference was not numerically identical to their
native values (meaning: it shuffled the names and numbers).
Make them all numerically equal, so that they can be used in
the same context.
This means, while previously we could compare NMNDiscPreference
directly according to their priority, we now need _preference_to_priority().
On the other hand, we could omit translate_preference() -- but actually,
we still have _route_preference_coerce() because pref comes from libndp
and is thus untrusted. We still have to range check it.
- merge the IPv4 and IPv6 implementations. They are for the most
part identical. Also, they are independent of NMIP4Config/NMIP6Config.
- parse the entire file at once. Don't parse it twice, once for the
name servers and once for the options. This also avoids loading
/etc/resolv.conf twice, as it would be done before.
Allow passing a pretty name for the zero flag 0, like "none".
Also, don't require flags to be power-of-two. Instead, allow names for
multiple flags. For example an "all" name. By specifying multi-value
flags first, their nick will be supersede the more specific flags.
Probably it doesn't make sense in usual cases, but nm_utils_flags2str()
should prevent such use.
This will make us stop worry how relevant are chunks of compat code with
older kernels when deciding whether it's worth supporting/testing them.
As if we actually were testing old kernels.
For convenience, to clear the address inplace, allow to leave @src NULL,
instead of requiring to set @src to @dst.
The only problem is, if you make use of this extended behavior and later backport
the use to an older branch, ensure that you cherry-pick this commit too.
That is easy to miss, but you are testing the backport, right?
Using plain numbers make it cumbersome to grep for
setting types by priority.
The only downside is, that with the enum values it
is no longer obvious which value has higher or lower
priority.
Also, introduce NM_SETTING_PRIORITY_INVALID. This is what
_nm_setting_type_get_base_type_priority() returns. For the moment
it still has the same numerical value 0 as before. Later, that
shall be distinct from NM_SETTING_PRIORITY_CONNECTION.
The dad_counter is hashed into the resulting address. Since we
want the hashing to be independent of the architecture, we always
hash 32 bit of dad_counter. Make the dad_counter argument of
type guint32 for consistency.
In practice this has no effect because:
- for all our (current!) architectues, guint is the same as
guint32.
- all callers of nm_utils_ipv6_addr_set_stable_privacy() keep
their dad-counter argument as guint8, so they never even pass
numbers larger then 255.
- nm_utils_ipv6_addr_set_stable_privacy() limits dad_counter
further against RFC7217_IDGEN_RETRIES.
(cherry picked from commit 951e5f5bf8)
https://tools.ietf.org/html/rfc7217 says:
The resulting Interface Identifier SHOULD be compared against the
reserved IPv6 Interface Identifiers [RFC5453] [IANA-RESERVED-IID]
and against those Interface Identifiers already employed in an
address of the same network interface and the same network
prefix. In the event that an unacceptable identifier has been
generated, this situation SHOULD be handled in the same way as
the case of duplicate addresses (see Section 6).
In case of conflict, this suggests to create a new address incrementing
the DAD counter, etc. Don't do that. If we generate an address of the
reserved region, just rehash it right away. Note that the actual address
anyway appears random, so this re-hashing is just as good as incrementing
the DAD counter and going through the entire process again.
Note that now we no longer generate certain addresses like we did
previously. But realize that we now merely reject (1 + 16777216 + 128)
addresses out of 2^64. So, the likelyhood of of a user accidentally
generating an address that is suddenly rejected is in the order of
10e-13 (1 / 1,099,503,173,697). Which is not astronomically, but still
extreeeemely unlikely.
Also, the whole process is anyway build on the idea that somebody else
might generate conflicting addresses (DAD). It means, there was always
the extremely tiny chance that the address you generated last time is
suddenly taken by somebody else. So, this change appears to a user
like these reserved addresses are now claimed by another (non existing)
host and a different address gets generated -- business as usual, as
far as SLAAC is concerned.
(cherry picked from commit f15c4961ad)
