If you have a LIST with 7 elements, and you lookup a value that
is not in the (sorted) list and would lie before the first element,
the binary search will dig down to imin=0, imid=0, imax=0 and
strcmp will give positive cmp value (indicating that the searched
value is sorted before).
Then, we would do "imax = imid - 1;", which wrapped to G_MAXUINT,
and the following "if (G_UNLIKELY (imin > imax))" would not hit,
resulting in an out of bound access next.
The easy fix is to not used unsigned integers.
The binary search was adapted from nm_utils_array_find_binary_search()
and nm_utils_ptrarray_find_binary_search(), which already used signed
integers to avoid this problem.
Fixes: 17d9b852c8 ('shared: explicitly implement binary search in NM_UTILS_STRING_TABLE_LOOKUP_DEFINE*()')
When the server is restarted the write to unix socket fails with
EPIPE. In such case, don't fail all the calls in queue; instead, after
a sync of the ovsdb state (through a monitor call), start processing
the queue again, including the call that previously failed.
Add a retry counter to avoid that calls are stuck in the queue forever
in a hypothetical scenario in which the write always fails.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/459
This would only be necessary when editing the test themselves,
to not only remove individual tests, but entire test runs.
It's not necessary to describe that in the howto.
When the users configure a DNS server on the interface, they likely
want to use it, regardless whether there is a default route on the
device.
For that to work, add an explicit "~" search domain.
Otherwise, by default NetworkManager only adds the special search domain
only on devices that have a "best default route" (nm_ip_config_best_default_route_is).
But that only considers a best default route in the main table, and
WireGuard (with ipx-auto-default-route) adds the default route to a
separate table. The heuristic to determine best devices works not well
with policy routing, so explicitly add this search domain during import.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/405
I, Frédéric Danis, agree to relicense my contributions to NetworkManager
as LGPL-2.1+ as proposed by Thomas Haller.
Some of my work may be held under copyright by Sigfox or Collabora Ltd.
I do not speak for those entities.
Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/462
Hide the object and class structures from public API.
This is an API and ABI break, but of something that is very likely
unused.
This is mainly done to embed the private structure in the object itself.
This has benefits for performance and debugability.
This is obviously a change in behavior, as we now honor backslash
escape sequences. With this change, all string values can be expressed,
both as option keys and values.
Previously, you could for example not set vpn.secrets to have a ','
and you could not set vpn.data to
nmcli connection modify "$PROFILE" +vpn.data 'ipsec-ike = aes256-sha2_256-modp2048,aes256-sha2_256-modp1536'
Use a relatively simple backslash escaping scheme. The main goal of
the scheme is that it doesn't change behavior for the majority of cases.
It only changes behavior for setting an option if:
- the string contains a backslash
- and if the backslash proceeds one of the few characters that support
escaping now (white space, ',', '\\', and '=').
The only downside here is that backslash is only treated special, if it
preceeds a character that requires escaping. That makes the behavior
non intuitive. However, it allows to write most backslashes without
escaping them as "\\\\" and thus keep previous behavior.
The nmcli getters now also escape the options accordingly. That means,
the string printed by the getter is also a valid input for the setter.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/390
The two modes VALUE_STRSPLIT_MODE_OBJLIST and VALUE_STRSPLIT_MODE_MULTILIST
basically do regular split and afterwards g_strstrip() all values and
remove empty tokens.
That is what the NM_UTILS_STRSPLIT_SET_FLAGS_STRSTRIP flag already does.
Use it.
There should be no change in behavior.
Add flags to explicitly escape leading or trailing spaces. Note
that we were already escaping trailing spaces.
This will be used later when supporting backslash escapes for
option parameters for nmcli (vpn.data).
We should generate the GVariant in a stable manner. That implies
to sort the keys first.
Also, don't use the NM_SETTING_VPN_SECRETS getter, which first needs
to clone all secrets.
When we use _nm_utils_strdict_from_dbus(), we clone the secrets, but don't use
nm_free_secret() for freeing them.
And in fact, we clone the setings twice. It't really not too nice. Implement
this without the property setter.
Don't use _nm_utils_copy_strdict().
On a minor note, that function will always allocate a GHashTable, even if
it only contains "" as only key. Later we would throw that out again,
so it was unnecessary.
Worse, using _nm_utils_copy_strdict() does not use nm_free_secrets as
destroy function. While it's in general difficult to clear all places
in memory where we copy the secrets around, we can easily avoid that.
Also skip over %NULL keys and values. It probably would be a bug passing
such arguments to the property. Better ignore them and not crash.
Like for data, now also allow empty secrets to be added to the VPN
setting.
For one, this avoids an assertion failure, where keyfile reader wouldn't
check whether a secret key is set to the empty word.
For data, it's more clear that we want to allow setting empty data
values. VPN settings are only interpreted by the VPN plugin, so libnm
and the daemon shouldn't prevent empty settings. It can be useful to
distinguish between unset (NULL) and empty values.
For secrets, it's less clear that empty secrets shall be allowed. I
think it should. Of course, the empty secret likely isn't a correct
nor valid secret. But libnm cannot validate the secrets anyway. It's
up to the VPN plugin to handle this in any way they see fit.
Also, already before, the user could set NM_SETTING_VPN_SECRETS to
a string dictionary with empty passwords. So, the API didn't fully
prevent that. Only certain API wouldn't play along.
Until now, nm_setting_vpn_add_data_item() would reject empty data values.
This leads for example to an assertion failure, if you write a keyfile
that assigns an empty value to a key. Keyfile reader would not check that
the value is non-empty before calling nm_setting_vpn_add_data_item().
Anyway, I think we should not require having non-empty data elements. It's
an unnecessary and sometimes harmful restriction. NetworkManager doesn't understand
not care about the content of the vpn data. That is up the VPN plugins. Sometimes
and empty value may be desirable.
Also, the NM_SETTING_VPN_DATA property setter wouldn't filter out empty
values either. So it was always possible to use some libnm API to set data
with empty values. The restriction in nm_setting_vpn_add_data_item() was
inconsistent.
Also drop the g_warn_if_fail() from update_secret_dict(). We
may get the variant from D-Bus, so avoiding this assertion (g_warn*() is
an assertion!) would require us to prevalidate the variant.
That would be very cumbersome, and we would probably not want to
handle that as an error and silently ignore them anyway. Just shut
up.
Ensure that the data hash doesn't contain keys with empty key-name.
It just doesn't make sense, and will lead to potential problems later,
if we would allow this to happen.
For example, keyfile writer may naively try to set all keys, without
checking for empty keys. That may lead to assertion failures or worse,
later on. Don't allow that.
Also, assert against non-empty keys in nm_setting_vpn_get_data_item()
and nm_setting_vpn_remove_data_item(). This stricter handling is a
change in behavior, however it would have always been a bug trying to
refer to empty key names. So, this assertion will only help to find
those bugs.
We always initialized priv->data in nm_setting_vpn_init(), but usually
soon after this hash would be replaced by NM_SETTING_VPN_DATA property
setter.
Avoid that. Allow for priv->data to be %NULL, which of course has the
meaning that no keys are set.
When we invoke the user's callback, be more robust about the user
modifying or unreferencing the NMSettingVpn, while iterating.
While it would be odd to modify the NMSettingVpn from inside the foreach
callback, we should behave consistently and sensibly.
That means, to ensure that the NMSettingVpn instance stays alive all
the time, that we don't crash, and that we always iterate over the
previously determined, planned list of keys.
Also, avoid some unnecessary string copies, like the clone of the first key.
For one, this code was unreachable, because we checked these conditions
already before.
That aside, g_warn_*() is for all intended purposes an assertion.
The caller probably gets the GVariant from an untrusted source (e.g. via
D-Bus). Asserting here is not helpful.
It's up to the caller to validate the argument. Or, in case the caller
doesn't care, update_secret_dict() should just do the sensible thing.
But be silent about it!
Yes, the compiler probably can optimize strlen() in these cases. That
still doesn't make strlen() the right API to check whether a NUL
terminated string is empty.
