Bug 1368353

no idea why this would happen...

assigning to bgalvani :)

The crash happens when editing a connection with the 'nmcli connection
edit', apparently because the @g_class field of a connection object is
invalid in g_object_get_property().

I suspect this is caused by a race condition since the editor runs in
its own thread different from the main thread of the GMainLoop, and
GObjects are not thread-safe.

I think the reason why the thread was added was to allow the use of
readline() without blocking the processing of D-Bus events; but I'm
not sure if this is safe since there is the possibility that the two
threads access the same object concurrently.

A possible fix would be to move the editor loop back to the main
thread and switch to the readline alternate interface [1], which
allows asynchronous operation.

[1] https://cnswww.cns.cwru.edu/php/chet/readline/readline.html#SEC41

(In reply to Beniamino Galvani from comment #2)

> I suspect this is caused by a race condition 

yeah, that sounds plausible (cannot say for sure).

But surely, the editor thread is a bug and unexpected crashes may happen in edit mode.

Pushed branch bg/cli-readline-async-rh1368353 for review.

(In reply to Beniamino Galvani from comment #4)
> Pushed branch bg/cli-readline-async-rh1368353 for review.

please refer in the commit message to https://bugzilla.gnome.org/show_bug.cgi?id=732097


          if (nmc_get_in_readline ()) {
+              g_print ("^C\n");

can we not print anything and just wrap the line?

For the terminal, this is configurable via `stty -ctlecho`. It seems, we should not print this ourself.




     /* Add string to the history */
+    if (rl_string && *rl_string)
+         add_history (rl_string);

I think, if the user pressed CTRL+C, we should add the command to the history.




let's drop nmc_set_in_readline() and have "g_return_val_if_fail (!in_readline, NULL);" at the preample of nmc_readline_helper().



I think signal_handler() must be async-signal-safe. g_message() is not allowed. 
I think g_unix_signal_add() is better.


g_message() and other g_log() functions are not suitable inside nmcli, because they print a prefix and the logging severity. g_print() or g_printerr() should be used -- or better a wrapper like nmcli_print().

(In reply to Thomas Haller from comment #5)
> (In reply to Beniamino Galvani from comment #4)
> > Pushed branch bg/cli-readline-async-rh1368353 for review.
> 
> please refer in the commit message to
> https://bugzilla.gnome.org/show_bug.cgi?id=732097

ok


>           if (nmc_get_in_readline ()) {
> +              g_print ("^C\n");
> 
> can we not print anything and just wrap the line?
> 
> For the terminal, this is configurable via `stty -ctlecho`. It seems, we
> should not print this ourself.

Mmh, doesn't seem to work with readline. I've restored the old way to print the ^C using "rl_echo_signal_char (SIGINT)".


>      /* Add string to the history */
> +    if (rl_string && *rl_string)
> +         add_history (rl_string);
> 
> I think, if the user pressed CTRL+C, we should add the command to the
> history.

Fixed.

> let's drop nmc_set_in_readline() and have "g_return_val_if_fail
> (!in_readline, NULL);" at the preample of nmc_readline_helper().

nmc_set_in_readline() is still needed to recognize whether SIGINT happens during a readline or not.

> I think signal_handler() must be async-signal-safe. g_message() is not
> allowed. 
> I think g_unix_signal_add() is better.

Fixed.

> g_message() and other g_log() functions are not suitable inside nmcli,
> because they print a prefix and the logging severity. g_print() or
> g_printerr() should be used -- or better a wrapper like nmcli_print().

Fine.

Repushed branch bg/cli-readline-async-rh1368353.

lgtm now

*** Bug 1374635 has been marked as a duplicate of this bug. ***

lgtm

Merged to master:

https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=36f561ead0191a914589fed284a05a9b3192293d

*** Bug 1379816 has been marked as a duplicate of this bug. ***

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