First of all, all file names in our source-tree should be unique. We should
not have stuff like "libnm-core/tests/test-general.c" and "src/tests/test-general.c".
The problem here are the C source files, and consequently also the test
binaries have duplicate names. We should avoid that in general. However,
our binaries should have a matching name with the C source. If
"test-general.c" is not good enough, that needs renaming. Not building
"platform-test-general" out of it.
On the other hand, all our tests should have a filename "*/tests/test-*", like
they do for autotools.
Rename the meson platform tests.
It's also important because "tools/run-nm-test.sh" relies on the test
name to workaround valgrind warnings.
There should be no difference in pratice. But we should not mention "/var/run"
as /var might not be mounted in early boot or rescue environment. Of course,
in those cases ConsoleKit is not running either. But still...
This change is a bit annoying, because we package "NetworkManager.conf" file in
our RPM. So, upon package upgrade, rpm will note that a new config file should
be installed and thus will leave ".rpmnew" files.
Also, don't mention "/var/run". It should really be just "/run" because
"/var" might not be mounted in early boot/initrd or in rescue environment.
"RUNDIR" is set to "$runstatedir/NetworkManager". That is not correct,
we must use "$runstatedir".
I don't understand how this could have ever worked. Commit e2ecf5b808
('dhcp: dhcpcd uses a fixed path for PID files') seems to address this issue,
but already then "RUNDIR" was set to "$(localstatedir)/run/NetworkManager".
NM_LOG_DOMAINS is a comma-separated list of the selected logging domains.
As the number of all logging domains is fixed at compile-time, the maximum
length of the buffer is known.
$ git grep $'^\t{ LOGD_' | sed 's/.*"\(.*\)" .*/\1/' | xargs | sed 's/ */,/g' | sed 's/^/NM_LOG_DOMAINS=/'
NM_LOG_DOMAINS=PLATFORM,RFKILL,ETHER,WIFI,BT,MB,DHCP4,DHCP6,PPP,WIFI_SCAN,IP4,IP6,AUTOIP4,DNS,VPN,SHARING,SUPPLICANT,AGENTS,SETTINGS,SUSPEND,CORE,DEVICE,OLPC,INFINIBAND,FIREWALL,ADSL,BOND,VLAN,BRIDGE,DBUS_PROPS,TEAM,CONCHECK,DCB,DISPATCH,AUDIT,SYSTEMD,VPN_PLUGIN,PROXY
Note that the static buffer "_all_logging_domains_to_str" is known
to be large enough to contain these domain names (it's even longer,
as it also contains "ALL", "IP", and "DHCP" alises). We can use that
to define the array of suitable size.
Don't create a heap allocated GString to hold the static
result of nm_logging_all_domains_to_string().
Instead, use a static buffer of the exactly required size.
The main reason to do this, is to get the exact size of
"_all_logging_domains_to_str" buffer. This is the upper
boundary for the size of a string buffer to hold all domain
names.
We will need that boundary in the next commit. The attractive
thing here is that we will have a unit-test failure if this
boundery no longer matches (--with-more-asserts). That means,
this boundary is guarded by unit tests and we don't accidentally
get it wrong when the domains change.
Also, take care to initialize the buffer in a thread-safe manner.
It's easy enough to get right, so there is no excuse for having
non-thread-safe code in logging.
Syslog's "facility" is a well defined thing and must be
one of a few well-known numbers. Don't re-use it for our
own purposes.
Fixes: 1b808d3b25 ('logging: add native systemd-journald support to nm-logging')
https://bugzilla.redhat.com/show_bug.cgi?id=1709741
This overflow could only happen when we would try to log a message
with "NM_DEVICE=", "NM_CONNECTION=", and more than 8 logging domains
(_NUM_MAX_FIELDS_SYSLOG_FACILITY - 2).
The latter is never the case. While we sometimes log messages with
more than one logging domain, there are no logging statements that
use most as 8 different logging domains. So, this overflow is not
actually reachable from current code (I think).
Fixes: ed552c732c ('logging: log device and connection along with the message'):
The type of the "data" pointer may not be compatible with the type of
the "to_free" / output pointer. This is due to constness, and that we
are unable in C to remove constness from a type.
For example,
{
const char *const *data = ...;
gs_free const char **cpy_to_free = NULL;
const char **cpy;
cpy = nm_memdup_maybe_a (300, data, NM_PTRARRAY_LEN (data) + 1, &cpy_to_free);
}
is prefectly valid , but would not have compiled.
It shows that "data" is not of type "*(&cpy_to_free)", but rather
it might be a non-const pointer of the same type.
Fixes: d0e1d0e626 ('shared: propagate types in nm_malloc_maybe_a(), nm_malloc0_maybe_a(), nm_memdup_maybe_a()')
Always ensure that the entire buffer is initialized with padding NULs.
For example, valgrind checks whether we access uninitalized memory,
so leaving this uninitalized can be unexpected and cause valgrind
failures. In general, one might be tempted to copy the ifname buffer (of
well known size IFNAMSIZ) with memcpy(). In that case, we should not
have trailing garbage there.
We could use strncpy() for that (which guarantees NUL padding), but
then we still would have to ensure NUL termination. But strncpy() is
frowned upon, so let's not use it here.
Note that g_strlcpy() does not guarantee NUL padding, so it's
unsuitable.
We could also implement this with a combination of memcpy() and
memset(). But in this case, it just seems simpler to iterate over the
16 bytes and do it manually.
==6207== Syscall param ioctl(SIOCETHTOOL) points to uninitialised byte(s)
==6207== at 0x514603B: ioctl (syscall-template.S:78)
==6207== by 0x19FC2F: _ioctl_call (nm-platform-utils.c:183)
==6207== by 0x1A026B: _ethtool_call_handle (nm-platform-utils.c:319)
==6207== by 0x1A031F: ethtool_get_stringset (nm-platform-utils.c:378)
==6207== by 0x1A03BC: ethtool_get_stringset_index (nm-platform-utils.c:414)
==6207== by 0x1A181E: nmp_utils_ethtool_supports_vlans (nm-platform-utils.c:912)
==6207== by 0x1756D7: link_supports_vlans (nm-linux-platform.c:6508)
==6207== by 0x1A81D8: nm_platform_link_supports_vlans (nm-platform.c:1536)
==6207== by 0x14B96B: test_internal (test-link.c:602)
==6207== by 0x4F5C18D: test_case_run (gtestutils.c:2597)
==6207== by 0x4F5C18D: g_test_run_suite_internal (gtestutils.c:2685)
==6207== by 0x4F5BF33: g_test_run_suite_internal (gtestutils.c:2697)
==6207== by 0x4F5C679: g_test_run_suite (gtestutils.c:2772)
==6207== by 0x4F5C694: g_test_run (gtestutils.c:2007)
==6207== by 0x166B4D: main (test-common.c:2092)
==6207== Address 0x1ffeffeecf is on thread 1's stack
==6207== in frame #1, created by _ioctl_call (nm-platform-utils.c:110)
==6207==
"ifname" is the stack-allocated array "known_ifnames" of suitable
IFNAMSIZ bytes. But it may not be fully initialized, so using memcpy()
to copy the string leads to unintialized warning.
We really should only copy the valid bytes, either with strcpy() or our
nm_utils_ifname_cpy() wrapper.
Fixes: 856322562e ('platform/ethtool,mii: retry ioctl when interface name was renamed for ehttool/mii')
"struct ifreq" contains a union field, and initalizing the struct is not
guaranteed to fill all bytes with zero (it only sets the first union
member to zero).
Since we later return the entire struct, ensure that it's initialized to
all zero by using memset().
libnm/tests/test-general statically links against libnm/libnm-utils.la
and dynamically against libnm/libnm.so. Hence, _nm_utils_init() is invoked
twice, failing the assertion.
That is a bug that must be fixed. For now, just don't assert.
Since we can easily lookup the vtable for a NMSetting8021xSchemeType,
it is convenient to also easily get the scheme_type for a given
NMSetting8021xSchemeVtable.
On my x86_64, this change is basically for free as it does not increase
the size of NMSetting8021xSchemeVtable, because the scheme_type fits in a
previously unused part of the NMSetting8021xSchemeVtable struct.
Ooherwise, the file has wrong permissions:
# ls -la /var/lib/NetworkManager/secret_key
----r-xr-x. 1 root root 50 May 14 13:52 /var/lib/NetworkManager/secret_key
Luckily, /var/lib/NetworkManager should be already
# ls -lad /var/lib/NetworkManager
drwx------. 2 root root 8192 May 14 13:57 /var/lib/NetworkManager
which mitigates this a bit.
Fixes: dbcb1d6d97 ('core: let nm_utils_secret_key_read() handle failures internally')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/175
We want the the hash-seed array is alined so it can be used both as
guint, guint32, and guint64 directly. Don't use _nm_alignas() but
instead just add the fields to the union so we get proper alignment.
While at at, also let the seed argument to c_siphash_init() be aligned
to 64 integers. c_siphash_init() does not require that, but it tries to
read the seed as (unaligned) LE 64 bit integers. So, it doesn't hurt.
We want to log pointer values to indicate the related parties of a
log message. But we should not, because plain pointer values can be
used to defeat ASLR.
Instead, we have nm_hash_obfuscate_ptr() to managle a pointer and give
a distinct (albeit not 100% unique) 64 bit integer for logging.
But for the logging messages to be meaning-full, all related parties
must use the same static-seed.
Add a macro NM_HASH_OBFUSCATE_PTR() that uses a particular seed.
- use GDBusConnection instead of GDBusProxy.
- namespace global variables with a "gl" struct.
- don't log __func__. If a log line should have a certain
topic/tag, the tag should be set manually, not based on the
function name. It just looks odd. Also, it's unnecessary.
- use GDBusConnection instead of GDBusProxy.
- rename "call-id" to "conf-id". It's really not a "call" but
configuration that gets added and NMPacrunnerManager ensures that
the configuration is send to pacrunner.
- let "conf-id" keep a reference to NMPacrunnerManager. For one,
when we remove configurations we need to call DestroyProxyConfiguration
to remove it again. We cannot just abort the requests but must linger
around until our configuration is properly cleaned up. Hence, we
anyway cannot destroy the NMPacrunnerManager earlier.
With respect to fixing shutdown not to leak anything, this merely
means that we must wait (and iterate the main loop) as long as
NMPacrunnerManager singleton still exits (that is anyway the plan
how to fix shutdown).
With these considerations it's also clear that our D-Bus calls must
have a stricter timeout: NM_SHUTDOWN_TIMEOUT_MS.
This is also nice because nm_pacrunner_manager_remove() no longer
needs a manager parameter, it can just rely on having a reference
to the manager.
- for logging the configuration IDs, don't log pointer values.
Logging pointer values should be avoided as it defeats ASLR.
Instead, give them a "log_id" number.
- pacrunner is a D-Bus activatable service. D-Bus activatable services
needs special care. We don't want to start it over and over again.
Instead, we only try to "StartServiceByName" if
- we have any configuration to add
- if pacrunner is currently confirmed not to be running (by watching
name owner changes)
- we didn't try to start it already. That means, only start it
at the beginning and afterwards set a flag to block it. When
we see pacrunner appear on D-Bus we always clear that flag,
that means if pacrunner drops of, we will try to restart it
(once).
PolicyKit is a D-Bus activatable service. I don't think it exits on idle (but maybe
it does, it certainly should).
Anyway, NetworkManager was watching the NameOwner of polkit and if the name was lost(!)
it would emit a NM_AUTH_MANAGER_SIGNAL_CHANGED, which causes the internal code to re-authenticate
right away. That means, if you stop policy kit, NetworkManager will ask it right away and
D-Bus activate it. This is not right.
In fact, we don't have to care about the name owner at all. Whenever we make a request,
we just make it and D-Bus activate the service as needed. If polkit starts, it emits a
Changed signal that we watch on D-Bus. That is the only moment when we should actually
emit NM_AUTH_MANAGER_SIGNAL_CHANGED, not when polkit disconnects.
Aside avoiding the unnecessary overhead of GDBusProxy, this simplifies
NMAuthManager because the instance is ready from the start to use D-Bus.
Previously, in the early phase requests needed to be queued until
GDBusProxy could be created asynchronously. Now, there is nothing
asynchronous involved during construction of the NMAuthManager (and
of course there are no blocking calls).
I also like this because it's non-obvious that subscription IDs from
GDBusConnection are "guint" (contrary to signal handler IDs which are
"gulong"). So, by using this API you get a compiler error when using the
wrong type.
In the past, when switching to nm_clear_g_signal_handler() this uncovered
multiple bugs where the wrong type was used to hold the ID.
