Bug 1368353 - [NMCI] [abrt] [faf] NetworkManager: g_object_get_property(): /usr/bin/nmcli killed by 11
Summary: [NMCI] [abrt] [faf] NetworkManager: g_object_get_property(): /usr/bin/nmcli k...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: NetworkManager
Version: 7.3
Hardware: Unspecified
OS: Unspecified
medium
medium
Target Milestone: rc
: ---
Assignee: Beniamino Galvani
QA Contact: Desktop QE
URL: http://faf.lab.eng.brq.redhat.com/faf...
Whiteboard:
: 1374635 1379816 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-08-19 07:25 UTC by Vladimir Benes
Modified: 2017-08-01 09:17 UTC (History)
8 users (show)

Fixed In Version: NetworkManager-1.8.0-0.2.git20170215.1d40c5f4.el7
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-08-01 09:17:07 UTC


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Product Errata RHSA-2017:2299 normal SHIPPED_LIVE Moderate: NetworkManager and libnl3 security, bug fix and enhancement update 2017-08-01 12:40:28 UTC
GNOME Bugzilla 732097 None None None 2016-09-05 17:48:04 UTC

Description Vladimir Benes 2016-08-19 07:25:45 UTC
This bug has been created based on an anonymous crash report requested by the package maintainer.

Report URL: http://faf.lab.eng.brq.redhat.com/faf/reports/bthash/ea8d063089884094ab7de7a2158c22b74aec82ba/

Comment 1 Thomas Haller 2016-08-19 12:31:01 UTC
no idea why this would happen...

assigning to bgalvani :)

Comment 2 Beniamino Galvani 2016-08-19 14:13:57 UTC
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

Comment 3 Thomas Haller 2016-08-20 07:34:50 UTC
(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.

Comment 4 Beniamino Galvani 2016-09-03 09:25:13 UTC
Pushed branch bg/cli-readline-async-rh1368353 for review.

Comment 5 Thomas Haller 2016-09-05 17:48:05 UTC
(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().

Comment 6 Beniamino Galvani 2016-09-09 20:51:15 UTC
(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.

Comment 7 Thomas Haller 2016-09-11 19:28:14 UTC
lgtm now

Comment 8 Thomas Haller 2016-09-12 08:52:35 UTC
*** Bug 1374635 has been marked as a duplicate of this bug. ***

Comment 9 Francesco Giudici 2016-09-22 17:03:54 UTC
lgtm

Comment 11 Beniamino Galvani 2016-09-27 21:04:00 UTC
*** Bug 1379816 has been marked as a duplicate of this bug. ***

Comment 13 errata-xmlrpc 2017-08-01 09:17:07 UTC
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


Note You need to log in before you can comment on or make changes to this bug.