Bug 1369380

Note, that NetworkManager only supports a subset of full-blown shell scripts. You can always create a ifcfg-rh file that NM will treat differently from initscripts (e.g. not shell expansion).

But yeah, for certain cases like here, we should aim to better parse shell.

As Thomas said there is a difference between initscripts and NM ifcfg reader. The scripts just source the file as a bash script. NM reads that as key/values and makes some escaping and other tweaks as appropriate.

However, commenting variables after their values seems useful, so lets allow it.

See NM upstream repository branch jk/ifcfg-comments-rh1369380.

+    if (s[0] == '"' || s[0] == '\'') {
+         do {
+              quote_end = strchr (quote_end + 1, s[0]);
+         } while (quote_end && *(quote_end-1) == '\\');

if the value is quoted with '', escaping is not supported. Should be like:
+         } while (quote_end &&
                   (s[0] == '\'' && *(quote_end-1) == '\\'));

Otherwise that looks good to me.

(In reply to Thomas Haller from comment #4)
> +    if (s[0] == '"' || s[0] == '\'') {
> +         do {
> +              quote_end = strchr (quote_end + 1, s[0]);
> +         } while (quote_end && *(quote_end-1) == '\\');
> 
> if the value is quoted with '', escaping is not supported. Should be like:
> +         } while (quote_end &&
>                    (s[0] == '\'' && *(quote_end-1) == '\\'));
> 
> Otherwise that looks good to me.

Actually svUnescape() supports escaping both inside "" and ''.

And I want to support something like this:
NAME='that\'s it  # part of value ' # comment
NAME="this is double quote \" # part of value " # comment

i.e. ' inside '' and " inside "" should be escaped with \. And thus checking whether '#' should be regarded as a comment or not, checks for \.
Thus I think the original code is the way to go.

this is not how shell treats single quotes. Backslash-escaping inside '' is not possible:

$ NAME='that\'s it  # part of value ' # comment
bash: it: command not found
$ NAME='that'\''s it  # part of value ' # comment
$ echo ">$NAME<"
>that's it  # part of value <


similarly, ifcfg-rh parser should aim to behave the same.


If svUnescape treats \ in '' special, that is another bug.

I am aware that bash doesn't allow escaping inside single quotes.
However, NM ifcfg parser treats quoted values and escaping a bit differently. Basically, svUnescape() just strips quotes (either '' or "") as the first and last character and then goes through the string and unespaces.
So, for example
ZONE='that\'s it'
accepts |that's it| string the same as 
ZONE='that's it' does.

And the new code for comments tries to preserve current behaviour and producing
|that's it| string for
ZONE='that\'s it' # comment

So I think this is better for the comments.
But if the escaping should be changed, it is definitely another topic and should be done outside of this bug. (I am not sure what compatibility consequences it would have.)

However, if you prefer, I added your suggestion as a separate commit to the branch. Thus the second ' closes the string.

maybe this should be just fixed altogether.

It's not that hard to walk through the string and support quoting and escaping like shell does.

For example: https://git.gnome.org/browse/network-manager-openvpn/tree/properties/import-export.c?id=bd7130b4e30046c7b12e5c2e518c95014452e789#n410
(note, that nm-openvpn parses the values like openvpn, which is not exactly the same as shell does).


Difficult is, if we see any ${VAR} or $(cmd) because of possibly nested braces. For that, I would treat $ as a normal character. Or maybe just ignore the value entire value as invalid and return NULL.

ups, last paragraph ... :)
I mean, in face of a $ or `, just error out and return NULL for that value. It's not supported.

I did a large rework of the shell parsing code:

th/ifcfg-rh-shell-parsing-rh1369380


It can support everything except:

 - no line continuations, like

      FOO1='
      '
      FOO2=a\
      b

  - no shell expansions via ``, $, !

  - no commands, like

      ls -1
      LANG=C ls -1

   - no multiple statements per line, like

      FOO1=a;FOO2=b


It now uses bash-syntax to escape \r and \n.

note that the branch fixes previous bugs, which unfortunately leads to certain configurations to be interpreted differently after update.


E.g. with previous versions of NM:

  $ nmcli connection modify t-ifcfg connection.stable-id "aa'bb"

result in

  STABLE_ID="aa\'bb"


After update, this is read back with the additional backslash -- like shell would do.


I don't see how that can be avoided. It was a bug, fixing it means to interpret previously written connections differently.

Note that this only matters for a few characters, like ' or ~. So, I'd hope the impact of this is acceptable.

> ifcfg-rh: unescape ifcfg value for CIPHER_GROUP/CIPHER_PAIRWISE

    It's not clear why we would read the CIPHER_GROUP/CIPHER_PAIRWISE
    verbatim=FALSE (without shell unescaping).

You meant 'verbatim=TRUE' ?

Branch th/ifcfg-rh-shell-parsing-rh1369380 LGTM.

(In reply to Beniamino Galvani from comment #12)

thanks. fixed and re-pushed.

branch merged to upstream master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=e3928c8c5ee83a09d73bf67820f51a7204e1cf50

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2017:2299