svUnescape() can return a pointer to the input argument
(if the input argument requires no unescaping or truncation).
That is actually the predominant case because most often we
store values that don't require escaping.
Optimize for that case.
This is especially important because we don't support
line continuation. Thus, with
FOO='val
bar=3'
wrong line
F2=b
F3='b
XXX=adf'
XXX2=val2
'
we now write
FOO=
#NM: FOO='val
bar=
#NM: bar=3'
#NM: wrong line
F2=b
F3=
#NM: F3='b
XXX=
#NM: XXX=adf'
XXX2=val2
#NM: '
Basically, the writer will comment out any line that is
- not all-whitespace
- not a '#' comment (possibly proceeded by whitespace)
- not a valid variable assignment
This avoids that writer writes lines that are not understood by
ifcfg-rh plugin, but interferes with initscripts. E.g.
NAME=old-name'
rm -rf /
'
becomes
NAME=new-name
#NM: rm -rf /
#NM: '
'\'', '~': must not be escaped with backslash.
Also, within double quotes the backslash escape character is only
removed before special caracters like '$' or '`'. Not in general.
Yes, it means that older versions of svEscape produced invalid escape
sequences that we now treat differently. But that is not realy
avoidable, it was a bug that needs to be fixed.
This is especially important for the team config JSON, which is expected
to contain newlines.
ANSI C quotation is bash specific, but initscripts already use #!/bin/bash.
Unfortunately, g_strescape() doesn't escape '\'' and can thus not be
used.
Also add a test that svEscape() and svUnescape() do a round-trip.
Not only consider \r and \n as candidates for ANSI C quotation, but all
ANSI control characters.
Better support parsing of shell. Now we support:
- combining values, like
FOO=a"b"
FOO=$'\n'b
- bash style ANSI C quotation ($''). This will allow us to properly
handle newlines in string values.
- comments at the end of a line (after whitespace)
FOO=val #comment
Note that this is different from a # without space
FOO=val#with#hashes
- trailing spaces are ignored like
FOO=a[space]
FOR=[space]
- history expansion via ! is not done (this is not new).
We don't support:
- line continuation like
FOO='
'
FOO=a\
b
- any form of shell expansion via $, ``.
FOO="$a"
Such values are recognized to name a variable FOO, but with an
empty value, like
FOO=%{nil}
which is not the same as a valid empty value
FOO=
- any other form of (unquoted) shell meta characters, like ; < > ( ).
This especially means, that the command invocations are invalid, like
ls -1
LANG=C ls -1
FOO1=a; FOO2=b
This also means, that spaces immidiately after the assignment are invalid:
FOO= val
Also, svUnescape() can now return %NULL to signal an invalid line like
FOO='
When
- reading a key that is defined multiple times, accept
the last occurrence.
- when deleting such a key, delete all occurrences.
- when overwriting such a key, overwrite the last occurrence
and delete any previous definitions.
It was not used and it is bad style. Especially, because
in the next commit we want to remove multiple definitions
of a key. Thus, we usually always iterate until the end.
Move the g_strchomp() inside svUnescape(). It is part of the
escaping process (although of course wrong to do, because
it accepts "FOO= bar". That will be fixed later).
Thereby, change the signature to allow in the future
to do unescape without additional copy.
svGetValue() had the meaning of returning a string, except the
empty word "" was coerced to NULL.
svGetValueFull() had the meaing of returing the value as string,
including the empty word.
Rename those functions to better express what they do.
Same for svSetValue*().
It is wrong to allow access to unquoted ifcfg-rh values.
All users of this ~feature~ misused it to encode meaning
in the type of quotation, which is wrong.
Also, shvar.h is not able to fully parse shell. We can improve
that, but it should be handled internally, in one place. Not by
callers applying some quirks after getting a "verbatim" value.
It is wrong to move the handling of quotes outside of shvar.h.
The ifcfg-rh core library (shvar.h) should handle quotation
transparently.
It is also wrong to encode meaning of the WPA_PSK depending on whether
the value is quoted or not. A psk that is 64 chars *must* be in hex
representation, and anything else is taken literal.
Also, the special handling of bash-syntax with $' was wrong. Including
the unit test "keys-test-wifi-wpa-psk-2" which contained invalid shell.
Support for $' must be done inside of shvar.h, for all properties alike
not just for WPA_PSK.
Let shvar.h do the escaping/unescaping of the ESSID.
We should not treat a value differently whether it is quoted or not.
Also, cutting away double quotes and calling svUnescape() is just wrong.
Now, we write a value in hex if it contains non-printable characters
or if the reader would treat it like a hex value. Reader treats ESSID
now as hex if it starts with "0x" followed by pairs of hex digits.
svUnescape() has no problem with extremely long strings.
It does not allocate any memory and has O(n) complexity.
No problem.
If somebody has a problem with extremely large files it's
shvarFile itself which caches the entire file in memory.
Also, libnm-core allows team configs to be 1 MB in size.
So, allow that here too.
It's not clear why we would read the CIPHER_GROUP/CIPHER_PAIRWISE
verbatim=TRUE (without shell unescaping). Especially since ifcfg-rh
writer does
svSetValue (ifcfg, "CIPHER_PAIRWISE", str->str, FALSE);
Refactor the code to be simpler and use a single list for both
'request' and 'also request' statements. This also has the advantage
that we can properly handle 'request' statements and reset the list of
options instead of appending to it.
The interaction between the manager state and connectivity check code
is tricky. When there is an active connection with a default route and
NMConnectivity reports full connectivity, we set the CONNECTED_GLOBAL
state. However, if the connectivity check hasn't run yet, we stay in
CONNECTED_SITE state. If there are also other connections that are
activating, the state is set to CONNECTING.
This is a problem, because in CONNECTING we never run the connectivity
check and thus we fail to recognize that there is full connectivity
until a periodic check is run.
To solve this, schedule the connectivity check every time there is an
active connection with default route, even if other connection are
still activating, so that the check result can make the state progress
to CONNECTED_GLOBAL.
For example for tun devices we get a lot of
(tun7): hw-addr: failed reading current MAC address
warnings. Just be silent about it. We log when something
changes, we don't need to log when we fail to obtain
a MAC address.
Thereby, refactor nm_device_update_hw_address() to return early.
The unmanaged flags PLATFORM_INIT indicates whether UDEV is done
initializing the device. We should not handle IP config changes
before that pointer.
This avoids codepaths that require the permanent MAC address of the
device. We should not freeze the permanent MAC address before
UDEV initialized the device, for two reasons:
- getting the permanent MAC address using ethtool is racy as
UDEV might still rename the interface.
- freezing a fake permanent MAC address should only happen after
UDEV is done configuring the MAC address of software devices.
#0 0x000055555568bc7a in nm_device_update_permanent_hw_address (self=self@entry=0x555555f0fb70 [NMDeviceVeth], force_freeze=force_freeze@entry=1) at src/devices/nm-device.c:11817
#1 0x000055555568c443 in nm_device_get_permanent_hw_address_full (self=self@entry=0x555555f0fb70 [NMDeviceVeth], force_freeze=force_freeze@entry=1, out_is_fake=out_is_fake@entry=0x0)
at src/devices/nm-device.c:12227
#2 0x000055555568cb06 in nm_device_get_permanent_hw_address (self=self@entry=0x555555f0fb70 [NMDeviceVeth]) at src/devices/nm-device.c:12237
#3 0x000055555568cb50 in spec_match_list (self=0x555555f0fb70 [NMDeviceVeth], specs=0x555555a5c000 = {...}) at src/devices/nm-device.c:12294
#4 0x00005555556a4ee6 in spec_match_list (device=0x555555f0fb70 [NMDeviceVeth], specs=0x555555a5c000 = {...}) at src/devices/nm-device-ethernet.c:1461
#5 0x00005555556978db in nm_device_spec_match_list (self=self@entry=0x555555f0fb70 [NMDeviceVeth], specs=0x555555a5c000 = {...}) at src/devices/nm-device.c:12277
#6 0x000055555558e187 in _match_section_infos_lookup (match_section_infos=0x555555a5d500, keyfile=0x555555a46f80, property=property@entry=0x555555793123 "ipv4.route-metric", device=device@entry=0x555555f0fb70 [NMDeviceVeth], out_value=out_value@entry=0x7fffffffe018) at src/nm-config-data.c:1169
#7 0x00005555555922ca in nm_config_data_get_connection_default (self=0x555555a548c0 [NMConfigData], property=property@entry=0x555555793123 "ipv4.route-metric", device=device@entry=0x555555f0fb70 [NMDeviceVeth]) at src/nm-config-data.c:1234
#8 0x00005555556790cd in _get_ipx_route_metric (self=self@entry=0x555555f0fb70 [NMDeviceVeth], is_v4=is_v4@entry=1) at src/devices/nm-device.c:1142
#9 0x000055555567912e in nm_device_get_ip4_route_metric (self=self@entry=0x555555f0fb70 [NMDeviceVeth]) at src/devices/nm-device.c:1161
#10 0x000055555567da6c in ip4_config_merge_and_apply (self=self@entry=0x555555f0fb70 [NMDeviceVeth], config=config@entry=0x0, commit=commit@entry=0, out_reason=out_reason@entry=0x0)
at src/devices/nm-device.c:4787
#11 0x000055555567e0fb in update_ip4_config (self=self@entry=0x555555f0fb70 [NMDeviceVeth], initial=initial@entry=0) at src/devices/nm-device.c:9532
#12 0x0000555555693acd in queued_ip4_config_change (user_data=0x555555f0fb70) at src/devices/nm-device.c:9651
#13 0x00007ffff4c966ba in g_main_context_dispatch (context=0x555555a46af0) at gmain.c:3154
#14 0x00007ffff4c966ba in g_main_context_dispatch (context=context@entry=0x555555a46af0) at gmain.c:3769
#15 0x00007ffff4c96a70 in g_main_context_iterate (context=0x555555a46af0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3840
#16 0x00007ffff4c96d92 in g_main_loop_run (loop=0x555555a47400) at gmain.c:4034
#17 0x000055555558372a in main (argc=<optimized out>, argv=<optimized out>) at src/main.c:411
Before the link is initialized, that is before UDEV completed
initializing the device, we should not evaluate the user-settings
unmanaged flags.
The reason is, that evaluating it likely involves looking at the
permanent MAC address, which might use the wrong fake MAC address
(before UDEV set the right one). Also, it might use the wrong ifname
to lookup the permanent MAC address via ethtool.
The permanent MAC address of an NMDevice shall not change as
long as the device is realized. That is, we read it only once
and don't change it afterwards.
There are two issues that this commit tries to mitigate:
(1) users are advised to use UDEV to rename interfaces. As we lookup
the permenent MAC address using ethtool (which uses the interface
name), there is a race where we could read the permanent MAC
address using the wrong interface name. We should wait until
UDEV finished initializing the device and until the interface
name is stable (see rh#1388286).
This commit still cannot avoid the race of ethtool entirely. It only
tries to avoid ethtool until UDEV has done its work. That is, until we
expect the interface name no longer to change.
(2) some device types, don't have a permanent MAC address so we fall
back to use the currently set address (fake). Again, users are advised
to use UDEV to configure the MAC addresses on such software devices.
Thus, we should not get the fake MAC address until UDEV initialized
the device.
This patch actually doesn't solve the problem at all yet.
The reason is that a regular caller of nm_device_get_permanent_hw_address() can
not afford to wait until UDEV settled. Thus, any user who requests the
permanent MAC address before the link is initialized, runs into the
problems above.
In a next step, we shall revisit such calls to nm_device_get_permanent_hw_address()
and delay them until the link is initialized.
We repeatedly call nm_device_update_hw_address() to reset the cached
MAC address of the device. However, we don't allow changing the address
length once it is set.
Multiple entities (initial, current and permanent MAC address) are all
checked to have the same address length. Changing the length would be a
very strange thing (and probably indicate a bug somewhere else).
Just don't allow that.
On devices that have no real permanent hardware address (as returned
by ethtool), we take the current MAC address of the device.
Currently, NM is a bit flaky about whether to accept such fake permanent
addresses for settings like keyfile.unmanaged-devices or the per-
connection property ethernet.mac-address. Probably, we should allow
using fake addresses there in general.
However, that leads to problems because NetworkManager itself changes
the current MAC address of such devices. For example when
configuing
keyfile.unmanaged-device=22:33:44:55:66:77
and later activating a connection with
ethernet.cloned-mac-address=22:33:44:55:66:77
we have a strange situation after restart and the device becomes
unmanaged.
We are going to avoid that, by remembering the fake permanent address
in the device state file.
This only matters:
- for devices that don't have a real permanent address (veth)
- if the user or NetworkManager itself changed the MAC address
of the device
- after a restart of NetworkManager, without reboot. A reboot
clears the device state for /var/run/NetworkManager.
_nm_utils_hwaddr_length() did a validation of the string
and returned the length of the address. In all cases where
we were interested in that, we also either want to validate
the address, get the address in binary form, or canonicalize
the address.
We can avoid these duplicate checks, by using _nm_utils_hwaddr_aton()
which both does the parsing and returning the length.
Since connection.autoconnect-slaves was introduced, we only allowed it
to autoactivate slaves for connections that were not slave themselves.
It seems useful to remove such limitation, but we must prevent an
infinite loop if there is a circular dependency between connections.
https://bugzilla.redhat.com/show_bug.cgi?id=1360386
