This can be replaced by nm_utils_escaped_tokens_split().
Note that nm_utils_escaped_tokens_split() does not behave exactly
the same. For example, nm_utils_str_simpletokens_extract_next() would
remove all backslashes and leave only the following character.
nm_utils_escaped_tokens_split() instead only strips backslashes
that preceed a delimiter, whitespace or another backslash.
But we should have one preferred way of tokenizing, and I find this
preferable, because it allows for most backslashes to appear verbatim.
(cherry picked from commit ced7dbe8bf)
Optimally, all list types properties in nmcli support proper escaping.
For that, we should use nm_utils_escaped_tokens_*() API.
For now, just change that for policy routes. They were just added recently,
so no change in behavior of released API.
(cherry picked from commit d59f046bb6)
Replace nm_utils_str_simpletokens_extract_next() by
nm_utils_escaped_tokens_split().
nm_utils_escaped_tokens_split() should become our first choice for
parsing and tokenizing.
Note that both nm_utils_str_simpletokens_extract_next() and
nm_utils_escaped_tokens_split() need to strdup the string once,
and tokenizing takes O(n). So, they are roughtly the same performance
wise. The only difference is, that as we iterate through the tokens,
we might abort early on error with nm_utils_str_simpletokens_extract_next()
and not parse the entire string. But that is a small benefit, since we
anyway always strdup() the string (being O(n) already).
Note that to-string will no longer escape ',' and ';'. This is a change
in behavior, of unreleased API. Also note, that escaping these is no
longer necessary, because nmcli soon will also use nm_utils_escaped_tokens_*().
Another change in behavior is that nm_utils_str_simpletokens_extract_next()
treated invalid escape sequences (backslashes followed by an arbitrary
character), buy stripping the backslash. nm_utils_escaped_tokens_*()
leaves such backslashes as is, and only honors them if they are followed
by a whitespace (the delimiter) or another backslash. The disadvantage
of the new approach is that backslashes are treated differently
depending on the following character. The benefit is, that most
backslashes can now be written verbatim, not requiring them to escape
them with a double-backslash.
Yes, there is a problem with these nested escape schemes:
- the caller may already need to escape backslash in shell.
- then nmcli will use backslash escaping to split the rules at ','.
- then nm_ip_routing_rule_from_string() will honor backslash escaping
for spaces.
- then iifname and oifname use backslash escaping for nm_utils_buf_utf8safe_escape()
to express non-UTF-8 characters (because interface names are not
necessarily UTF-8).
This is only redeamed because escaping is really only necessary for very
unusual cases, if you want to embed a backslash, a space, a comma, or a
non-UTF-8 character. But if you have to, now you will be able to express
that.
The other upside of these layers of escaping is that they become all
indendent from each other:
- shell can accept quoted/escaped arguments and will unescape them.
- nmcli can do the tokenizing for ',' (and escape the content
unconditionally when converting to string).
- nm_ip_routing_rule_from_string() can do its tokenizing without
special consideration of utf8safe escaping.
- NMIPRoutingRule takes iifname/oifname as-is and is not concerned
about nm_utils_buf_utf8safe_escape(). However, before configuring
the rule in kernel, this utf8safe escape will be unescaped to get
the interface name (which is non-UTF8 binary).
(cherry picked from commit b6d0be2d3b)
nmcli supports list options (optlist and multilist properties).
These commonly are individual items, concatenated by a delimiter.
It should be generally possibly to express every value. That means, we
need some for of escaping mechanism for delimiters.
Currently this is all inconsistent or no escaping is supported. I intend
to fix that (which will be a change in behavior).
For now, just add yet another style of tokenzing/concatenating list
items in nmcli. This is the style to replace all other styles.
(cherry picked from commit ba956bd499)
This escapes strings so that they can be concatenated with a delimiter
and without loss tokenized with nm_utils_escaped_tokens_split().
Note that this is similar to _nm_utils_escape_plain() and
_nm_utils_escape_spaces(). The difference is that
nm_utils_escaped_tokens_escape() also escapes the last trailing
whitespace. That means, if delimiters contains all NM_ASCII_SPACES, then
it is identical to _nm_utils_escape_spaces(). Otherwise, the trailing
space is treated specially. That is, because
nm_utils_escaped_tokens_split() uses NM_UTILS_STRSPLIT_SET_FLAGS_STRSTRIP,
to strip leading and trailing whitespace. To still express a trailing
whitespace, the last whitespace must be escaped. Note that
NM_UTILS_STRSPLIT_SET_FLAGS_STRSTRIP also honors escaping any whitespace
(not only at the last position), but when escaping we don't need to
escape them, because unescaped (non-trailing) whitespace are taken just
fine.
The pair nm_utils_escaped_tokens_split() and
nm_utils_escaped_tokens_escape() are proposed as default way of
tokenizing a list of items. For example, with
$ nmcli connection modify "$PROFILE" +ipv4.routing-rules 'priority 5 from 192.168.7.5/32 table 5, priority 6 iif a\, from 192.168.7.5/32 table 6'
Here we implement a to/from string function to handle one item
(nm_ip_routing_rule_{from,to}_string()). When such elements are combined with ',',
then we need to support an additional layer of escaping on top of that.
The advantage is that the indvidual to/from string functions are agnostic
to this second layer of escaping/tokenizing that nmcli employs to handle
a list of these items.
The disadvantage is that we possibly get multiple layers of backslash
escapings. That is only mitigated by the fact that nm_utils_escaped_tokens_*()
supports a syntax for which *most* characters don't need any special escaping.
Only delimiters, backslash, and the trailing space needs escaping, and
these are cases are expected to be few.
(cherry picked from commit e206e3d4d8)
Add a new flag that will remove escape characters after splitting
the string.
This implements a special kind of backslash escaping. It's not C escape
sequences (like '\n' or '\020'), but simply to take the special character
following the backslash verbatim. Note that the backslash is only
considered special, if it's followed by a delimiter, another backslash,
or a whitespace (in combination with %NM_UTILS_STRSPLIT_SET_FLAGS_STRSTRIP).
The main purpose of this form of escaping is nmcli's list options, like
$ nmcli connection modify "$PROFILE" +ipv4.routing-rules 'priority 5 from 192.168.7.5/32 table 5, priority 6 iif a\, from 192.168.7.5/32 table 6'
It's a contrieved example, but the list options are a list of IP
addresses, rules, etc. They implement their own syntax for one element,
and are concatenated by ','. To support that one element may have
arbitrary characters (including the delimiter and whitespaces), nmcli
employs a tokenization with this special kind of escaping.
(cherry picked from commit 9522aaf226)
This will essentially call g_strstrip() on each token.
There are some specialties:
- if the resulting word is empty after stripping, then according to
%NM_UTILS_STRSPLIT_SET_FLAGS_PRESERVE_EMPTY, the empty token will be
removed. If that results in an empty string array, %NULL will be
returned.
- if %NM_UTILS_STRSPLIT_SET_FLAGS_ALLOW_ESCAPING is set, then
whitespace that is backslash escaped is not removed.
Since this is a post-operation that happens after tokeninzing, it
could be done as a separate function. And we already have this function:
_nm_utils_unescape_plain() and _nm_utils_unescape_spaces().
However, that is ugly for several reasons:
- the stripping should be part of the tokenizing, you shouldn't need
several steps.
- nm_utils_strsplit_set_full() returns a "const char **" which
indicates the strings must not be freed. However, it is perfectly
valid to modify the string inplace. Hence, the post-op function
would need to cast the strings to "char *", which seems ugly
(although we do that on many places, and it's guaranteed to work).
- _nm_utils_unescape_plain()/_nm_utils_unescape_spaces() is indeed
already used together with nm_utils_strsplit_set_full(). However,
it requires to initialize the cb_lookup buffer twice. I would expect
that initializing the cb_lookup buffer is a large portion of what
the function does already (for short strings).
This issue will be solved in the next commit by adding yet another flag
which allows to unescape.
(cherry picked from commit 5b2b0dcadf)
Policy routing rules are global, and unlike routes not tied to an interface by ifindex.
That means, while we take full control over all routes of an interface during a sync,
we need to consider that multiple parties can contribute to the global set of rules.
That might be muliple connection profiles providing the same rule, or rules that are added
externally by the user. NMPRulesManager mediates for that.
This is done by NMPRulesManager "tracking" rules.
Rules that are not tracked by NMPRulesManager are completely ignored (and
considered externally added).
When tracking a rule, the caller provides a track-priority. If multiple
parties track a rule, then the highest (absolute value of the) priority
wins.
If the highest track-priority is positive, NMPRulesManager will add the rule if
it's not present.
When the highest track-priority is negative, then NMPRulesManager will remove the
rule if it's present (enforce its absence).
The complicated part is, when a rule that was previously tracked becomes no
longer tracked. In that case, we need to restore the previous state.
If NetworkManager added the rule earlier, then untracking the rule
NMPRulesManager will remove the rule again (restore its previous absent
state).
By default, if NetworkManager had a negative tracking-priority and removed the
rule earlier (enforced it to be absent), then when the rule becomes no
longer tracked, NetworkManager will not restore the rule.
Consider: the user adds a rule externally, and then activates a profile that
enforces the absence of the rule (causing NetworkManager to remove it).
When deactivating the profile, by default NetworkManager will not
restore such a rule! It's unclear whether that is a good idea, but it's
also unclear why the rule is there and whether NetworkManager should
really restore it.
Add weakly tracked rules to account for that. A tracking-priority of
zero indicates such weakly tracked rules. The only difference between an untracked
rule and a weakly tracked rule is, that when NetworkManager earlier removed the
rule (due to a negative tracking-priority), it *will* restore weakly
tracked rules when the rules becomes no longer (negatively) tracked.
And it attmpts to do that only once.
Likewise, if the rule is weakly tracked and already exists when
NMPRulesManager starts posively tracking the rule, then it would not
remove again, when no longer positively tracking it.
- fix the argument type to be "gint32" and not "int".
- assert in nmp_rules_manager_track_default() for the input
arguments.
- use boolean bitfield in private data.
The name "priority" is overused. Also rules have a "priority", but that'
something else.
Rename the priority of how rules are tracked by NMPRuleManager to
"track_priority".
All that setting track-default does, is calling nmp_rules_manager_track_default()
when the rules are first accessed.
That is not right API. Since nmp_rules_manager_track_default() is already public
API (good), every caller that wishes this behavior should track these routes explicitly.
The call to nm_utils_parse_variant_attributes() is useless. The following
_tc_read_common_opts() call does the same thing. This was probably left
in place by accident.
Before:
# nmcli c modify eth666 tc.qdiscs root
Error: failed to modify tc.qdiscs: '(null)' is not a valid kind The valid syntax is: '[root | parent <handle>] [handle <handle>] <qdisc>'.
After:
# nmcli c modify eth666 tc.qdiscs root
Error: failed to modify tc.qdiscs: kind is missing. The valid syntax is: '[root | parent <handle>] [handle <handle>] <kind>'.
==16725==ERROR: AddressSanitizer: global-buffer-overflow on address 0x0000005a159f at pc 0x00000046fc1b bp 0x7fff6038f900 sp 0x7fff6038f8f0
READ of size 1 at 0x0000005a159f thread T0
#0 0x46fc1a in _do_test_unescape_spaces libnm-core/tests/test-general.c:7791
#1 0x46fe5b in test_nm_utils_unescape_spaces libnm-core/tests/test-general.c:7810
#2 0x7f4ac5fe7fc9 in test_case_run gtestutils.c:2318
#3 0x7f4ac5fe7fc9 in g_test_run_suite_internal gtestutils.c:2403
#4 0x7f4ac5fe7e83 in g_test_run_suite_internal gtestutils.c:2415
#5 0x7f4ac5fe7e83 in g_test_run_suite_internal gtestutils.c:2415
#6 0x7f4ac5fe8281 in g_test_run_suite gtestutils.c:2490
#7 0x7f4ac5fe82a4 in g_test_run (/lib64/libglib-2.0.so.0+0x772a4)
#8 0x48240d in main libnm-core/tests/test-general.c:7994
#9 0x7f4ac5dc9412 in __libc_start_main (/lib64/libc.so.6+0x24412)
#10 0x423ffd in _start (/home/bgalvani/work/NetworkManager/libnm-core/tests/test-general+0x423ffd)
0x0000005a159f is located 49 bytes to the right of global variable '*.LC370' defined in 'libnm-core/tests/test-general.c' (0x5a1560) of size 14
'*.LC370' is ascii string 'nick-5, green'
0x0000005a159f is located 1 bytes to the left of global variable '*.LC371' defined in 'libnm-core/tests/test-general.c' (0x5a15a0) of size 1
'*.LC371' is ascii string ''
SUMMARY: AddressSanitizer: global-buffer-overflow libnm-core/tests/test-general.c:7791 in _do_test_unescape_spaces
Leak detection adds unhelpful messages to the stderr of nmcli, making
tests fail. For example:
=================================================================
==17156==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 256 byte(s) in 2 object(s) allocated from:
#0 0x7f08c7e27c88 in realloc (/lib64/libasan.so.5+0xefc88)
#1 0x7f08c7546e7d in g_realloc (/lib64/libglib-2.0.so.0+0x54e7d)
We already have code that parses exactly this kinds of string:
nm_utils_parse_inaddr_prefix_bin(). Use it.
Also, it doesn't use g_strsplit_set() to separate a string at the first
'/'. Total overkill.
- use cleanup macros everywhere.
- In particular use nm_auto_clear_variant_builder to free the
GVariantBuilder in the error cases. Note that the error cases
anyway are asserted against, so during a normal test run there
was no leak. But we should not write software like that.
- use nm_utils_strsplit_set_with_empty() instead of g_strsplit_set().
We should use our variant also in unit-tests, because that way the
function gets more test coverage. And it likely performs better
anyway.
Currently, if user configuration or settings specify that a software
device is unmanaged, for example:
[device-bond-unmanaged]
match-device=interface-name:bond*
managed=0
or
[keyfile]
unmanaged-devices=interface-name:bond*
and there is a connection for the device with autoconnect=yes, NM
creates the platform link and a realized device in unmanaged
state. Fix this, the device should not be realized if it is unmanaged.
https://bugzilla.redhat.com/show_bug.cgi?id=1679230
nm_device_spec_match_list_full() calls
nm_device_get_permanent_hw_address() which freezes the MAC address, so
currently callers must avoid the function when the device is not
completely platform-initialized.
Instead, use nm_device_get_permanent_hw_address_full() to avoid
freezing the MAC when the device is not platform-initialized. In this
way nm_device_spec_match_list_full() can be called from any state
without side effects.
Instead of growing the buffer for the tokens (and reallocating),
do one pre-run over the string and count the delimiters. This
way we know how much space we need and we don't need to
reallocate.
Interestingly, this is notably slower than the previous implementation,
because previously if would not bother determining the right number of
tokens but just over-allocate with a reasonable guess of 8 and grow the
buffer exponentially. Still, I like this better because while it may
be slower in common scenarios, it allocates the exact number of buffer
space.
