nettools plugin represents the way how to do it, and other plugins
should mimic that behavior. The nettools implementation adds private
DHCP options as hex, except the options
- 249 (Microsoft Classless Static Route)
- 252 (Web Proxy Auto Discovery Protocol)
Adjust systemd plugin to do the same.
For 252, we now parse the "wpad" option differently. The change in
behavior is that the property is now no longer exposed as hexstring,
but as backslash escaped plain text.
For 249, the option is not implemented. But stop adding the option as
hex-string too.
A NMRefString tracks the length seprately, it thus may not be a NUL terminated
string (although, there is always a NUL character at the end of the buffer).
As such, the previous implementation did not work correctly in when comparing
for example NMRefString("a\0b") with "a". There was even a comment hinting
to that fact. Instead of making obscure comments, fix the implementation to
behave always correctly.
This helper is useful to get a dummy GSource instance that can be
refed, unrefed and destroyed. It can act as a replacement for
a timeout source with infinite timeout.
However, don't also use the NM_DEPRECATED_IN_1_32 macro, because that
causes annoying compiler warnings.
There is no replacement for the function in libnm, nor is it planned
to add one. So users may still call it, but they are now warned by
documentation that it may not be a good idea.
In the past, the UUID was only loosely validate and would accept
forms that are not valid. This was fixed by commit 207cf3d5d4 ('libnm:
normalize "connection.uuid"'). Now the UUID is always strictly valid
and lower case.
Thus, don't use the fuzzy nm_utils_is_uuid() from libnm but the exact
check nm_uuid_is_valid_full().
Note that this is only used for assertions in the header file. We thus
don't want to drag in "libnm-glib-aux/nm-uuid.h". Instead, we forward
declare the function.
lgtm.com warns about declarations are block scope, so fix that too by
moving the declaration at file scope.
lgtm.com warns about function declarations inside blocks.
*sigh*. I think it's well understood what this code means, and it is not
done by accident. Still, let's make the tool happy in this case.
lgtm.com warns about these uses. They are correct though. Maybe the code should
not use alloca() simply to suppress the warning. Instead, add a comment pointing
out that this is in fact correct.
For one, "src_n_map" must always be greater than zero at this point.
lgtm.com warns about that, and the point of this patch is to avoid
that warning.
Still, the check really isn't needed, also because nm_memdup() explicitly
handles buffers sizes of zero.
Since the [main].iwd-config-path functionality, where NM watches for
NMSettingsConnection changes and update IWD network config files with
new settings, has proven to work without issues so far, enable it by
default. Instead of hardcoding /var/lib/iwd as the value, and since the
value can't be probed at NM compile time, query it from IWD's recently-
added D-Bus interface for settings when [main].iwd-config-path is either
missing or set to the new value "auto".
Coverity has issues with functions that handle ownership like
g_strstrip(). Thus the scan is full of false positives like:
Error: RESOURCE_LEAK (CWE-772): [#def45] [important]
NetworkManager-1.31.5/src/core/devices/wwan/nm-service-providers.c:134: alloc_fn: Storage is returned from allocation function "g_strdup".
NetworkManager-1.31.5/src/core/devices/wwan/nm-service-providers.c:134: noescape: Resource "g_strdup(attribute_values[i])" is not freed or pointed-to in "g_strchug".
NetworkManager-1.31.5/src/core/devices/wwan/nm-service-providers.c:134: leaked_storage: Failing to save or free storage allocated by "g_strdup(attribute_values[i])" leaks it.
# 132| if (strcmp(attribute_names[i], "value") == 0) {
# 133| parse_context->state = PARSER_METHOD_GSM_APN;
# 134|-> parse_context->apn = g_strstrip(g_strdup(attribute_values[i]));
# 135| break;
# 136| }
Add a workaround for that.
There are other functions that have the same problem, but the usage
g_strstrip(g_strdup(...)) is common to warrant a special workaround.
It's a false positive. Still avoid it.
Error: FORWARD_NULL (CWE-476): [#def479]
NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5558: var_compare_op: Comparing "strbuf->_priv_str" to null implies that "strbuf->_priv_str" might be null.
NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5575: var_deref_model: Passing "strbuf" to "nm_str_buf_maybe_expand", which dereferences null "strbuf->_priv_str".
# 5573| l2 = ((gsize) l) + 1u;
# 5574|
# 5575|-> nm_str_buf_maybe_expand(strbuf, l2, FALSE);
# 5576|
# 5577| va_start(args, format);
Error: FORWARD_NULL (CWE-476): [#def480]
NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5558: var_compare_op: Comparing "strbuf->_priv_str" to null implies that "strbuf->_priv_str" might be null.
NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5575: no_write_call: Although "nm_str_buf_maybe_expand" does overwrite "strbuf->_priv_str" on some paths, it also contains at least one feasible path which does not overwrite it.
NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5578: var_deref_op: Dereferencing null pointer "strbuf->_priv_str".
# 5576|
# 5577| va_start(args, format);
# 5578|-> l = g_vsnprintf(&strbuf->_priv_str[strbuf->_priv_len], l2, format, args);
# 5579| va_end(args);
# 5580|
Also Coverity found that something is wrong here:
Error: FORWARD_NULL (CWE-476): [#def361]
NetworkManager-1.31.5/src/libnm-client-impl/nm-vpn-plugin-old.c:441: var_compare_op: Comparing "connection" to null implies that "connection" might be null.
NetworkManager-1.31.5/src/libnm-client-impl/nm-vpn-plugin-old.c:489: var_deref_model: Passing null pointer "connection" to "g_object_unref", which dereferences it.
# 487| }
# 488|
# 489|-> g_object_unref(connection);
# 490| }
# 491|
Fixes: 6793a32a8c ('libnm: port to GDBus')
Previously, we would allocate a buffer of the worst case, that is,
4 times the number of bytes, in case all of them require octal escaping.
Coverity doesn't like _escape_ansic() for another reason:
Error: NULL_RETURNS (CWE-476): [#def298]
NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:161: returned_null: "g_malloc" returns "NULL".
NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:161: alias: Assigning: "q" = "dest = g_malloc(strlen(source) * 4UL + 1UL + 3UL)". Both pointers are now "NULL".
NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:163: dereference: Incrementing a pointer which might be null: "q".
# 161| q = dest = g_malloc(strlen(source) * 4 + 1 + 3);
# 162|
# 163|-> *q++ = '$';
# 164| *q++ = '\'';
# 165|
It doesn't recognize that g_malloc() shouldn't return NULL (because
we never request zero bytes).
I am not sure how to avoid that, but let's rework the code to first count
how many characters we exactly need. It think that should also help with
the coverity warning.
Doing exact allocation requires first to count the number of required
bytes. It still might be worth it, because we might keep the allocated
strings a bit longer around.
It seems very ugly to read one byte at a time. Use a naive buffered
reader, so that we can read multiple bytes at a time, and return them
one by one.
Also, this now keeps state of any error/EOF. Once we reach EOF, we won't
read again. The previous code did that too, but I think this code is
easier to read.
g_warning() and printing to stdout/stderr are not suitable actions
for a library. If there is something important, find a way to report the
condition to the caller. If it's not important, stay quiet.
"val" and "key" are now marked as nm_auto. They are freed at the end,
and we should not free them before breaking the loop (at least not,
without also clearing the variables).
Fixes: 02dbba49d6 ('libnm: fix leak in nm_vpn_service_plugin_read_vpn_details()')
Since g_string_free() takes an additional argument,
it's not direclty usable with nm_clear_pointer(ptr, g_string_free);
As workaround, add nm_clear_g_string() helper.
Coverity doesn't like the previous code:
Error: RESOURCE_LEAK (CWE-772): [#def34] [important]
NetworkManager-1.31.5/src/core/devices/team/nm-device-team.c:835: alloc_fn: Storage is returned from allocation function "g_strdup".
NetworkManager-1.31.5/src/core/devices/team/nm-device-team.c:835: noescape: Resource "g_strdup(config)" is not freed or pointed-to in "g_strdelimit".
NetworkManager-1.31.5/src/core/devices/team/nm-device-team.c:835: leaked_storage: Failing to save or free storage allocated by "g_strdup(config)" leaks it.
# 833| char *sanitized_config;
# 834|
# 835|-> sanitized_config = g_strdelimit(g_strdup(config), "\r\n", ' ');
# 836| err = teamdctl_port_config_update_raw(priv->tdc, slave_iface, sanitized_config);
# 837| g_free(sanitized_config);
Maybe this works better.
When the ACCEPT_ALL_MAC_ADDRESSES key is found by the wired reader, the
wired setting was not being created.
Fixes: d946aa0c50 ('wired-setting: add support to accept-all-mac-addresses')
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
