Re: [PATCH] libnm: Check the priv pointer before loop traverse.

[Shih-Yuan Lee (FourDollars)]

On Thu, May 5, 2016 at 4:33 PM, Thomas Haller  wrote:

> On Wed, 2016-05-04 at 16:09 +0800, Shih-Yuan Lee (FourDollars) wrote:
> > When we used WWAN and enabled the PIN on SIM, we encountered the
> > following
> > crash during the stress suspend&resume test.
> >
> > This patch can avoid this crash.
> >
> > GNU gdb (Ubuntu 7.11-0ubuntu1) 7.11
> > Copyright (C) 2016 Free Software Foundation, Inc.
> > License GPLv3+: GNU GPL version 3 or later  > pl.html>
> > This is free software: you are free to change and redistribute it.
> > There is NO WARRANTY, to the extent permitted by law.  Type "show
> > copying"
> > and "show warranty" for details.
> > This GDB was configured as "x86_64-linux-gnu".
> > Type "show configuration" for configuration details.
> > For bug reporting instructions, please see:
> > .
> > Find the GDB manual and other documentation resources online at:
> > .
> > For help, type "help".
> > Type "apropos word" to search for commands related to "word".
> > Reading symbols from /usr/bin/nm-connection-editor...(no debugging
> > symbols found)...done.
> > warning: core file may not match specified executable file.
> > [New LWP 1895]
> > [New LWP 1896]
> > [New LWP 1898]
> > [New LWP 1897]
> > [New LWP 1899]
> > warning: Could not load shared library symbols for 2 libraries, e.g.
> > /usr/lib/x86_64-linux-gnu/liblttng-ust-tracepoint.so.0.
> > Use the "info sharedlibrary" command to see the complete listing.
> > Do you need "set solib-search-path" or "set sysroot"?
> > [Thread debugging using libthread_db enabled]
> > Using host libthread_db library "/lib/x86_64-linux-
> > gnu/libthread_db.so.1".
> > Core was generated by `/usr/bin/nm-connection-editor'.
> > Program terminated with signal SIGSEGV, Segmentation fault.
> > #0  recheck_pending_activations (self=0x193d220) at nm-manager.c:824
> > warning: Source file is more recent than executable.
> > 824   for (iter = priv->devices; iter; iter = iter-
> > >next) {
> > [Current thread is 1 (Thread 0x7f3c4197fa80 (LWP 1895))]
> > (gdb) bt
> > #0  recheck_pending_activations (self=0x193d220) at nm-manager.c:824
> > #1  0x7f3c400cbfa5 in g_closure_invoke () from /usr/lib/x86_64-
> > linux-gnu/libgobject-2.0.so.0
> > #2  0x7f3c400ddfc1 in ?? () from /usr/lib/x86_64-linux-
> > gnu/libgobject-2.0.so.0
> > #3  0x7f3c400e6d5c in g_signal_emit_valist () from
> > /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
> > #4  0x7f3c400e708f in g_signal_emit () from /usr/lib/x86_64-
> > linux-gnu/libgobject-2.0.so.0
> > #5  0x7f3c400d04d4 in ?? () from /usr/lib/x86_64-linux-
> > gnu/libgobject-2.0.so.0
> > #6  0x7f3c400d2961 in g_object_notify () from /usr/lib/x86_64-
> > linux-gnu/libgobject-2.0.so.0
> > #7  0x7f3c406e3e63 in deferred_notify_cb (data=)
> > at nm-object.c:252
> > #8  0x7f3c3fdf4fda in g_main_context_dispatch () from
> > /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #9  0x7f3c3fdf5380 in ?? () from /lib/x86_64-linux-gnu/libglib-
> > 2.0.so.0
> > #10 0x7f3c3fdf56a2 in g_main_loop_run () from /lib/x86_64-linux-
> > gnu/libglib-2.0.so.0
> > #11 0x00414e81 in main ()
> > (gdb) l
> > 819   if (!nm_settings_get_startup_complete (priv-
> > >settings)) {
> > 820   _LOGD (LOGD_CORE,
> > "check_if_startup_complete returns FALSE because of NMSettings");
> > 821   return;
> > 822   }
> > 823
> > 824   for (iter = priv->devices; iter; iter = iter-
> > >next) {
> > 825   NMDevice *dev = iter->data;
> > 826
> > 827   if (nm_device_has_pending_action (dev)) {
> > 828   _LOGD (LOGD_CORE,
> > "check_if_startup_complete returns FALSE because of %s",
> > (gdb) print priv
> > $1 = (NMManagerPrivate *) 0x0
> >
> > Shih-Yuan Lee (FourDollars) (1):
> >   libnm: Check the priv pointer before loop traverse.
> >
> >  libnm/nm-manager.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
>
>
> Sidenote:
>
> The "l" command shows code from src/nm-manager.c:824, instead it should
> show libnm/nm-manager.c:824. Something is wrong with the paths for the
> source files and the debug information.
>
> Note that core daemon (src/nm-manager.c) does not ues libnm (libnm/nm-
> manager.c). So, these two should not be in the same backtrace.
>
> Anyway, that is not really the issue here, just a bit confusing at
> first. The backtrace is still helpful...
>
>
> Thomas
>

Sorry, this is my fault.
i didn't notice that I associated it with the wrong nm-manager.c.

-- 
Shih-Yuan Lee (FourDollars) | Software Engineer | Commercial Engineering -
PC & Core Taipei | Ubuntu Engineering and Services | Canonical
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list

Re: [PATCH] libnm: Check the priv pointer before loop traverse.

[Thomas Haller]

On Wed, 2016-05-04 at 16:09 +0800, Shih-Yuan Lee (FourDollars) wrote:
> When we used WWAN and enabled the PIN on SIM, we encountered the
> following
> crash during the stress suspend&resume test.
> 
> This patch can avoid this crash.
> 
> GNU gdb (Ubuntu 7.11-0ubuntu1) 7.11
> Copyright (C) 2016 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later  pl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show
> copying"
> and "show warranty" for details.
> This GDB was configured as "x86_64-linux-gnu".
> Type "show configuration" for configuration details.
> For bug reporting instructions, please see:
> .
> Find the GDB manual and other documentation resources online at:
> .
> For help, type "help".
> Type "apropos word" to search for commands related to "word".
> Reading symbols from /usr/bin/nm-connection-editor...(no debugging
> symbols found)...done.
> warning: core file may not match specified executable file.
> [New LWP 1895]
> [New LWP 1896]
> [New LWP 1898]
> [New LWP 1897]
> [New LWP 1899]
> warning: Could not load shared library symbols for 2 libraries, e.g.
> /usr/lib/x86_64-linux-gnu/liblttng-ust-tracepoint.so.0.
> Use the "info sharedlibrary" command to see the complete listing.
> Do you need "set solib-search-path" or "set sysroot"?
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-
> gnu/libthread_db.so.1".
> Core was generated by `/usr/bin/nm-connection-editor'.
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0  recheck_pending_activations (self=0x193d220) at nm-manager.c:824
> warning: Source file is more recent than executable.
> 824   for (iter = priv->devices; iter; iter = iter-
> >next) {
> [Current thread is 1 (Thread 0x7f3c4197fa80 (LWP 1895))]
> (gdb) bt
> #0  recheck_pending_activations (self=0x193d220) at nm-manager.c:824
> #1  0x7f3c400cbfa5 in g_closure_invoke () from /usr/lib/x86_64-
> linux-gnu/libgobject-2.0.so.0
> #2  0x7f3c400ddfc1 in ?? () from /usr/lib/x86_64-linux-
> gnu/libgobject-2.0.so.0
> #3  0x7f3c400e6d5c in g_signal_emit_valist () from
> /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
> #4  0x7f3c400e708f in g_signal_emit () from /usr/lib/x86_64-
> linux-gnu/libgobject-2.0.so.0
> #5  0x7f3c400d04d4 in ?? () from /usr/lib/x86_64-linux-
> gnu/libgobject-2.0.so.0
> #6  0x7f3c400d2961 in g_object_notify () from /usr/lib/x86_64-
> linux-gnu/libgobject-2.0.so.0
> #7  0x7f3c406e3e63 in deferred_notify_cb (data=)
> at nm-object.c:252
> #8  0x7f3c3fdf4fda in g_main_context_dispatch () from
> /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #9  0x7f3c3fdf5380 in ?? () from /lib/x86_64-linux-gnu/libglib-
> 2.0.so.0
> #10 0x7f3c3fdf56a2 in g_main_loop_run () from /lib/x86_64-linux-
> gnu/libglib-2.0.so.0
> #11 0x00414e81 in main ()
> (gdb) l
> 819   if (!nm_settings_get_startup_complete (priv-
> >settings)) {
> 820   _LOGD (LOGD_CORE,
> "check_if_startup_complete returns FALSE because of NMSettings");
> 821   return;
> 822   }
> 823   
> 824   for (iter = priv->devices; iter; iter = iter-
> >next) {
> 825   NMDevice *dev = iter->data;
> 826   
> 827   if (nm_device_has_pending_action (dev)) {
> 828   _LOGD (LOGD_CORE,
> "check_if_startup_complete returns FALSE because of %s",
> (gdb) print priv
> $1 = (NMManagerPrivate *) 0x0
> 
> Shih-Yuan Lee (FourDollars) (1):
>   libnm: Check the priv pointer before loop traverse.
> 
>  libnm/nm-manager.c | 2 ++
>  1 file changed, 2 insertions(+)
> 


Sidenote:

The "l" command shows code from src/nm-manager.c:824, instead it should
show libnm/nm-manager.c:824. Something is wrong with the paths for the
source files and the debug information.

Note that core daemon (src/nm-manager.c) does not ues libnm (libnm/nm-
manager.c). So, these two should not be in the same backtrace.

Anyway, that is not really the issue here, just a bit confusing at
first. The backtrace is still helpful...


Thomas


signature.asc
Description: This is a digitally signed message part
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list

Re: [PATCH] libnm: Check the priv pointer before loop traverse.

[Shih-Yuan Lee (FourDollars)]

I see. Thx for your review.

On Thu, May 5, 2016 at 4:11 PM, Thomas Haller  wrote:

> On Wed, 2016-05-04 at 16:09 +0800, Shih-Yuan Lee (FourDollars) wrote:
> > ---
> >  libnm/nm-manager.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/libnm/nm-manager.c b/libnm/nm-manager.c
> > index a6c1f3f..9316fdb 100644
> > --- a/libnm/nm-manager.c
> > +++ b/libnm/nm-manager.c
> > @@ -816,6 +816,8 @@ recheck_pending_activations (NMManager *self)
> >   const GPtrArray *devices;
> >   NMDevice *device;
> >
> > + g_return_if_fail (priv);
> > +
> >   /* For each pending activation, look for an active
> > connection that has the
> >* pending activation's object path, where the active
> > connection and its
> >* device have both updated their properties to point to
> > each other, and
>
>
> Hi,
>
>
> This patch is not correct for two reasons:
>
> - g_return_if_fail() and similar macros are assertions. They must never
> be used for valid control flow.
>
> - @priv at this place can only be NULL/invalid, if @self was
> NULL/invalid. From the backtrace you see, that @self is not NULL. You
> canot inspect a dangling, invalid pointer to decide that it is invalid.
>
> The patch might accidentally avoid the crash, but it doesn't fix the
> bug.
>
>
> Thomas




-- 
Shih-Yuan Lee (FourDollars) | Software Engineer | Commercial Engineering -
PC & Core Taipei | Ubuntu Engineering and Services | Canonical
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list

Re: [PATCH] libnm: Check the priv pointer before loop traverse.

[Thomas Haller]

On Wed, 2016-05-04 at 16:09 +0800, Shih-Yuan Lee (FourDollars) wrote:
> ---
>  libnm/nm-manager.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libnm/nm-manager.c b/libnm/nm-manager.c
> index a6c1f3f..9316fdb 100644
> --- a/libnm/nm-manager.c
> +++ b/libnm/nm-manager.c
> @@ -816,6 +816,8 @@ recheck_pending_activations (NMManager *self)
>   const GPtrArray *devices;
>   NMDevice *device;
>  
> + g_return_if_fail (priv);
> +
>   /* For each pending activation, look for an active
> connection that has the
>    * pending activation's object path, where the active
> connection and its
>    * device have both updated their properties to point to
> each other, and


Hi,


This patch is not correct for two reasons:

- g_return_if_fail() and similar macros are assertions. They must never
be used for valid control flow.

- @priv at this place can only be NULL/invalid, if @self was
NULL/invalid. From the backtrace you see, that @self is not NULL. You
canot inspect a dangling, invalid pointer to decide that it is invalid.

The patch might accidentally avoid the crash, but it doesn't fix the
bug.


Thomas

signature.asc
Description: This is a digitally signed message part
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list