Re: [GNC-dev] gnucash stable: Fix NULL dereference in gnc_plugin_page_report_focus_widget.

2023-11-02 Thread john
It's crashing before the call to g_type_check_instance_is_a. The stack trace in 
https://lists.gnucash.org/pipermail/gnucash-user/2023-November/109345.html 
points to offset 1374 in gnc_plugin_page_report_focus_widget. Here's the 
disassembly:
 
 tub for: gnc_window_set_progressbar_window
libgnc-gnome.dylib[0x7c30e] <+1406>: testl  %eax, %eax
libgnc-gnome.dylib[0x7c310] <+1408>: jne0x7c05c   ; <+716>
libgnc-gnome.dylib[0x7c316] <+1414>: movq   %r15, %rdi
libgnc-gnome.dylib[0x7c319] <+1417>: callq  0x8fa5e   ; symbol 
stub for: gtk_widget_grab_focus
libgnc-gnome.dylib[0x7c31e] <+1422>: jmp0x7c05c   ; <+716>
libgnc-gnome.dylib[0x7c323] <+1427>: callq  0x8d520   ; symbol 
stub for: __stack_chk_fail
libgnc-gnome.dylib[0x7c328] <+1432>: nopl   (%rax,%rax)

G_

> On Nov 2, 2023, at 02:04, Geert Janssens  wrote:
> 
> This seems to point at a regression in Gtk.
> 
> From what I understand GTK_IS_WIDGET should return FALSE if widget is NULL.
> 
> It's defined here:
> https://gitlab.gnome.org/GNOME/gtk/-/blob/main/gtk/gtkwidget.h?
> ref_type=heads#L46
> 
> It's a macro that calls G_TYPE_CHECK_INSTANCE_TYPE, which is defined here:
> https://gitlab.gnome.org/GNOME/glib/-/blob/main/gobject/gtype.h?
> ref_type=heads#L541
> 
> The comment above this definition states G_TYPE_CHECK_INSTANCE_TYPE should 
> return FALSE if instance is NULL.
> 
> So I'm rather surprised this use of GTK_IS_WIDGET crashes gnucash.

Hmm. It does indeed look properly null-checked.  G_TYPE_CHECK_INSTANCE_TYPE 
just forwards to _G_TYPE_CIT at 
https://gitlab.gnome.org/GNOME/glib/-/blame/main/gobject/gtype.h?page=3#L2671

(# define _G_TYPE_CIT(ip, gt) (G_GNUC_EXTENSION ({ \
  GTypeInstance *__inst = (GTypeInstance*) ip; GType __t = gt; gboolean __r; \
  if (!__inst) \
__r = FALSE; \
  else if (__inst->g_class && __inst->g_class->g_type == __t) \
__r = TRUE; \
  else \
__r = g_type_check_instance_is_a (__inst, __t); \
  __r; \
}))
The disassembly of gnc_plugin_page_report_focus_widget:
libgnc-gnome.dylib[0x7c2d8] <+1352>: callq  0x8fa4c   ; symbol 
stub for: gtk_widget_get_type
libgnc-gnome.dylib[0x7c2dd] <+1357>: testq  %r15, %r15  
   <<< if 
(!__inst)
libgnc-gnome.dylib[0x7c2e0] <+1360>: je 0x7c05c   ; <+716>
libgnc-gnome.dylib[0x7c2e6] <+1366>: movq   (%r15), %rcx
libgnc-gnome.dylib[0x7c2e9] <+1369>: testq  %rcx, %rcx  
    if 
(__inst->g_class
libgnc-gnome.dylib[0x7c2ec] <+1372>: je 0x7c2f3   ; <+1379>
libgnc-gnome.dylib[0x7c2ee] <+1374>: cmpq   %rax, (%rcx)
 < && 
__inst->g_class->g_type == __t  *** Crash is here ***
libgnc-gnome.dylib[0x7c2f1] <+1377>: je 0x7c306   ; <+1398>
libgnc-gnome.dylib[0x7c2f3] <+1379>: movq   %r15, %rdi
libgnc-gnome.dylib[0x7c2f6] <+1382>: movq   %rax, %rsi
libgnc-gnome.dylib[0x7c2f9] <+1385>: callq  0x8da0c   ; symbol 
stub for: g_type_check_instance_is_a
libgnc-gnome.dylib[0x7c2d1] <+1345>: movq   -0x98(%rbp), %r15
libgnc-gnome.dylib[0x7c2fe] <+1390>: testl  %eax, %eax
libgnc-gnome.dylib[0x7c300] <+1392>: je 0x7c05c   ; <+716>
libgnc-gnome.dylib[0x7c306] <+1398>: movq   %r15, %rdi
libgnc-gnome.dylib[0x7c309] <+1401>: callq  0x8fa7c   ; symbol 
stub for: gtk_widget_is_focus

It's crashing dereferencing __inst->g_class->g_type, and the diagnostic says 
it's a nullptr:
VM Region Info: 0 is not in any region.  Bytes before following region: 
4537495552
I've asked Michael for the register block to confirm that %rcx contains 
nullptr, but I'm baffled how it can go from not null at 1369 to null at 1374.
Regards,
John Ralls




___
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel


Re: [GNC-dev] gnucash stable: Fix NULL dereference in gnc_plugin_page_report_focus_widget.

2023-11-02 Thread Geert Janssens
This seems to point at a regression in Gtk.

>From what I understand GTK_IS_WIDGET should return FALSE if widget is NULL.

It's defined here:
https://gitlab.gnome.org/GNOME/gtk/-/blob/main/gtk/gtkwidget.h?
ref_type=heads#L46

It's a macro that calls G_TYPE_CHECK_INSTANCE_TYPE, which is defined here:
https://gitlab.gnome.org/GNOME/glib/-/blob/main/gobject/gtype.h?
ref_type=heads#L541

The comment above this definition states G_TYPE_CHECK_INSTANCE_TYPE should 
return FALSE if instance is NULL.

So I'm rather surprised this use of GTK_IS_WIDGET crashes gnucash.

Regards,

Geert

Op donderdag 2 november 2023 04:15:31 CET schreef John Ralls:
> Updatedvia  https://github.com/Gnucash/gnucash/commit/5e06c8d8 
> (commit)
>   from  https://github.com/Gnucash/gnucash/commit/d617129d (commit)
> 
> 
> 
> commit 5e06c8d8a00935eff5908d71da46f148d6f01e43
> Author: John Ralls 
> Date:   Wed Nov 1 19:54:54 2023 -0700
> 
> Fix NULL dereference in gnc_plugin_page_report_focus_widget.
> 
> Reported by Michael Hendry in gnucash-user.
> 
> diff --git a/gnucash/gnome/gnc-plugin-page-report.cpp
> b/gnucash/gnome/gnc-plugin-page-report.cpp index 724008b4d8..a8a58ce7e0
> 100644
> --- a/gnucash/gnome/gnc-plugin-page-report.cpp
> +++ b/gnucash/gnome/gnc-plugin-page-report.cpp
> @@ -334,7 +334,7 @@ gnc_plugin_page_report_focus_widget (GncPluginPage
> *report_plugin_page) if (!priv->loaded) // so we only do the load once
>  gnc_plugin_page_report_load_uri (report_plugin_page);
> 
> -if (GTK_IS_WIDGET(widget))
> +if (widget && GTK_IS_WIDGET(widget))
>  {
>  if (!gtk_widget_is_focus (GTK_WIDGET(widget)))
>  gtk_widget_grab_focus (GTK_WIDGET(widget));
> 
> 
> 
> Summary of changes:
>  gnucash/gnome/gnc-plugin-page-report.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> ___
> gnucash-changes mailing list
> gnucash-chan...@gnucash.org
> https://lists.gnucash.org/mailman/listinfo/gnucash-changes




___
gnucash-devel mailing list
gnucash-devel@gnucash.org
https://lists.gnucash.org/mailman/listinfo/gnucash-devel