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.
We should not use global variables, and we should minimize the state
that we pass around. Instead of requiring the full NmCli struct in
nm_cli_spawn_pager(), pass only the necessary data.
This reduces our use of global variables.
Of course, we later pass the point on, where we need to cast the constness away
again. This is more a reminder that we aren't suppost to change the variable.
We should try to avoid access to global variables. For libreadline
callbacks we still need a global variable.
Introduce a global variable nm_cli_global_readline, specially for this
use. It makes the places clear where we use it, and discourages
the use at other places, where we better avoid global variables.
In the next commit, GString will be replaced by NMStrBuf. Then, we will
pre-allocate a string buffer with 16 bytes, and measure the performance
difference. To have it comparable, adjust the pre-allocation size also
with GString.
nm_utils_buf_utf8safe_unescape() is almost the same as g_strcompress(),
with the only difference is that if the string contains NUL escapes "\000",
it will be handled correctly.
In other words, g_strcompress() and nm_utils_str_utf8safe_unescape() can only
unescape values, that contain no NUL escapes. That's why we added our
own binary unescape function.
As we already have our g_strcompress() variant, use it. It just gives it more
testing and usage. Also, we have full control over it's behavior. For example,
g_strcompress() issues a g_warning() when encountering a trailing '\\'. I
think this makes it unsuitable to unescape untrusted data. Either the function
should fail, or just make the best of it. Currently, our implementation
does the latter.
Our own implementation of a string buffer like GString.
Advantages (in decreasing relevance):
- Since we are in control, we can easily let it nm_explicit_bzero()
the memory. The regular GString API cannot be used in such a case.
While nm_explicit_bzero() may or may not be of questionable benefit,
the problem is that if the underlying API counteracts the aim of
clearing memory, it gets impossible. As API like NMStrBuf supports
it, clearing memory is a easy as enable the right flag.
This would for example be useful for example when we read passwords
from a file or file descriptor (e.g. try_spawn_vpn_auth_helper()).
- We have API like
nmp_object_to_string (const NMPObject *obj,
NMPObjectToStringMode to_string_mode,
char *buf,
gsize buf_size);
which accept a fixed size output buffer. This has the problem of
how choosing the right sized buffer. With NMStrBuf such API could
be instead
nmp_object_to_string (const NMPObject *obj,
NMPObjectToStringMode to_string_mode,
NMStrBuf *buf);
which can automatically grow (using heap allocation). It would be
easy to extend NMStrBuf to use a fixed buffer or limiting the
maximum string length. The point is, that the to-string API wouldn't
have to change. Depending on the NMStrBuf passed in, you can fill
an unbounded heap allocated string, a heap allocated string up to
a fixed length, or a static string of fixed length. NMStrBuf currently
only implements the unbounded heap allocate string case, but it would
be simple to extend.
Note that we already have API like nm_utils_strbuf_*() to fill a buffer
of fixed size. GString is not useable for that (efficiently), hence
this API exists. NMStrBuf could be easily extended to replace this API
without usability or performance penalty. So, while this adds one new
API, it could replace other APIs.
- GString always requires a heap allocation for the container. In by far
most of the cases where we use GString, we use it to simply construct
a string dynamically. There is zero use for this overhead. If one
really needs a heap allocated buffer, NMStrBuf can easily embedded
in a malloc'ed memory and boxed that way.
- GString API supports inserting and removing range. We almost never
make use of that. We only require append-only, which is simple to
implement.
- GString needs to NUL terminate the buffer on every append. It
has unnecessary overhead for allowing a usage of where intermediate
buffer contents are valid strings too. That is not the case with
NMStrBuf: the API requires the user to call nm_str_buf_get_str() or
nm_str_buf_finalize(). In most cases, you would only access the string
once at the end, and not while constructing it.
- GString always grows the buffer size by doubling it. I don't think
that is optimal. I don't think there is one optimal approach for how
to grow the buffer, it depends on the usage patterns. However, trying
to make an optimal choice here makes a difference. QT also thinks so,
and I adopted their approach in nm_utils_get_next_realloc_size().
When growing a buffer by appending a previously unknown number
of elements, the often preferable strategy is growing it exponentially,
so that the amortized runtime and re-allocation costs scale linearly.
GString just always increases the buffer length to the next power of
two. That works.
I think there is value in trying to find an optimal next size. Because
while it doesn't matter in terms of asymptotic behavior, in practice
a better choice should make a difference. This is inspired by what QT
does ([1]), to take more care when growing the buffers:
- QString allocates 4 characters at a time until it reaches size 20.
- From 20 to 4084, it advances by doubling the size each time. More
precisely, it advances to the next power of two, minus 12. (Some memory
allocators perform worst when requested exact powers of two, because
they use a few bytes per block for book-keeping.)
- From 4084 on, it advances by blocks of 2048 characters (4096 bytes).
This makes sense because modern operating systems don't copy the entire
data when reallocating a buffer; the physical memory pages are simply
reordered, and only the data on the first and last pages actually needs
to be copied.
Note that a QT is talking about 12 characters, so we use 24 bytes
head room.
[1] https://doc.qt.io/qt-5/containers.html#growth-strategies
This was previously tracked via a signal "scanning-prohibited".
However, I think it was buggy, because the signal didn't specify
a GSignalAccumulator, so when a NMDeviceOlpcMesh registered a handler,
NMDeviceWifi.scanning_prohibited() was ignored.
In theory, a GObject signal decouples the target and source of the
signal and is more abstract. But more abstraction is worse, if there
is exactly one target who cares about this signal: the OLPC mesh.
And that target is well known at compile time. So, don't pretend that
NMDeviceWifi or NMDeviceOlpcMesh aren't aware that they are together in
this.
Another downside of the signal is that you don't know when scanning gets
unblocked. You can only poll and asked whether it is blocked, but there
was no mechanism how NMDeviceWifi would be notified when scanning is
no longer blocked.
Rework this. Instead, the OLPC mesh explicitly registers and unregisters
its blocking state with nm_device_wifi_scanning_prohibited_track().
It feels better to first parse input arguments before authenticating.
One argument for otherwise would be that we shouldn't reveal any
information about the request before authenticating it. Meaning: every
request (even with invalid arguments) should fail with
permission-denied.
However, I prefer this for minor reasons:
- what makes a valid request is no secret. And if somebody makes an
invalid request, it should fail with invalid-arguments first.
- we possibly can short cut the expensive authentication process, where
we ask PolicyKit.
- by extracting the options variant early and only pass on the SSIDs
array, we handle the encoding of the options array earlier and where
it belongs: closer to the D-Bus request that defines the meaning of
the argument.
Also, change the failure reason to return invalid-argument.
This was first introduced by commit 4ed4b491fa ('2005-12-31 Dan
Williams <dcbw@redhat.com>'), a very long time ago.
It got reworked several times, but I don't think this code makes sense
anymore. So, if nm_platform_wifi_get_quality() returns an error, we
would ignore it for three times, until we would set the strength to the
error code (presumably -1). Why? If we cannot read the strength via
nl80211/WEXT, then we should just keep whatever we got from supplicant.
Drop this.
Also, only accept the percentage if it is in a valid range from 0 to
100%. If the driver (or platform code) gives us numbers out of that
range, we have no idea what their meaning is. In that case, the value
must be fixed in the lower layers, that knows how to convert the value
from the actual meaning to the requested percentage.
In NMSupplicantInterface, we determine whether we currently are scanning
both on the "scanning" supplicant state and the "Scanning" property.
Extend that. If we currently are scanning and are about to clear the
scanning state, then pretend to still scan as long as we are still
initializing BSS instances. What otherwise happens is that we declare
that we finished scanning, but the NMWifiAP instances are not yet ready.
The result is, that `nmcli device wifi` will already start printing the
scan list, when we didn't yet fully process all access points.
Now, _notify_maybe_scanning() will delay switching the scanning state to
disabled, as long as we have BSS initializing (bss_initializing_lst_head).
Also, ignore the "ScanDone" signal. It's redundant to the "Scanning"
property anyway.
Also, only set priv->last_scan_msec when we switch the scanning state
off. That is the right (and only) place where the last-scan timestamp
needs updating.
Certain properties (for example "scanning") are combined from multiple
other properties. So, we want to notify a changed signal, exactly when
something relevant changes. We also may not want to emit a signal while
we are still in the middle of changing multiple properties together.
Only at certain places we want to check and emit the signal.
Simplify the implementation for that by tracking the property value that
we currently expose, and keeping state about when it changes.
- add more code comments
- refactor the code flow in _get_hash_key_init() to follow a simpler
code path.
- use c_siphash_hash() instead of 3 separate steps.
- Drop "?: static_seed" from nm_hash_static(). It's not useful, because
the only _get_hash_key() for which _get_hash_key()^static_seed is zero
is ~static_seed. That means, only one value of all the static seeds
can result in zero here. At that point, we can just coerce that value
to 3679500967u directly.
