After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 514630 - Crash in xl_chart_import_error_bar()
Crash in xl_chart_import_error_bar()
Status: RESOLVED FIXED
Product: Gnumeric
Classification: Applications
Component: import/export MS Excel (tm)
git master
Other All
: Normal critical
: ---
Assigned To: Morten Welinder
Jody Goldberg
Depends on:
Blocks:
 
 
Reported: 2008-02-05 22:15 UTC by sum1
Modified: 2008-02-07 23:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fuzzed chart-tests-excel.xls (173.50 KB, application/vnd.ms-excel)
2008-02-05 22:17 UTC, sum1
  Details
proposed patch (665 bytes, patch)
2008-02-06 06:16 UTC, Jean Bréfort
none Details | Review
enhanced patch (915 bytes, patch)
2008-02-06 17:04 UTC, Jean Bréfort
none Details | Review

Description sum1 2008-02-05 22:15:43 UTC
Version: r16354
OS: Ubuntu Gutsy

The upcoming sample is a fuzzed version of chart-tests-excel.xls.

Steps to reproduce:
- Load the upcoming attachment in Gnumeric to trigger a crash

Backtrace:

Program received signal SIGSEGV, Segmentation fault.

Thread NaN (LWP 10439)

  • #0 xl_chart_import_error_bar
    at ms-chart.c line 3113
  • #1 ms_excel_chart_read
    at ms-chart.c line 3459
  • #2 ms_excel_chart_read_BOF
    at ms-chart.c line 3570
  • #3 ms_read_OBJ
    at ms-obj.c line 1268
  • #4 ms_escher_read_ClientData
    at ms-escher.c line 1989
  • #5 ms_escher_read_container
    at ms-escher.c line 2093
  • #6 ms_escher_read_SpContainer
    at ms-escher.c line 504
  • #7 ms_escher_read_container
    at ms-escher.c line 2093
  • #8 ms_escher_read_SpgrContainer
    at ms-escher.c line 1930
  • #9 ms_escher_read_container
    at ms-escher.c line 2093
  • #10 ms_escher_read_DgContainer
    at ms-escher.c line 1935
  • #11 ms_escher_read_container
    at ms-escher.c line 2093
  • #12 ms_escher_parse
    at ms-escher.c line 2160
  • #13 excel_read_sheet
  • #14 excel_read_BOF
    at ms-excel-read.c line 6464
  • #15 excel_read_workbook
    at ms-excel-read.c line 6532
  • #16 excel_file_open
    at boot.c line 191
  • #17 go_plugin_loader_module_func_file_open
    at go-plugin-loader-module.c line 239
  • #18 go_plugin_file_opener_open
    at go-plugin-service.c line 476
  • #19 go_file_opener_open
    at file.c line 294
  • #20 wb_view_new_from_input
    at workbook-view.c line 1212
  • #21 wb_view_new_from_uri
    at workbook-view.c line 1264
  • #22 main
    at main-application.c line 417

Comment 1 sum1 2008-02-05 22:17:00 UTC
Created attachment 104518 [details]
fuzzed chart-tests-excel.xls
Comment 2 Jean Bréfort 2008-02-06 06:16:33 UTC
Created attachment 104537 [details] [review]
proposed patch
Comment 3 sum1 2008-02-06 06:50:03 UTC
(In reply to comment #2)
> Created an attachment (id=104537) [edit]
> proposed patch
> 

+	g_return_if_fail (pspec);
+
 	state->plot = parent->series->plot;

It looks like the following if statement will always be false due to the g_return_if_fail call above.

 	if (pspec == NULL) {
 		pspec = g_object_class_find_property (
Comment 4 Jean Bréfort 2008-02-06 10:06:47 UTC
You are right, I did that in a hurry before leaving to work. The solution is to check parent&&parent->series before any attempt to set pspec. Something as:

	GParamSpec *pspec;
	g_return_if_fail (parent && parent->series);
	pspec = g_object_class_find_property (
		G_OBJECT_GET_CLASS (parent->series),
		prop_name);

or use the same test as in xl_chart_import_trend_line

anyway as it is clearly a file consistency issue, I'm fealing that the use of g_return_if_fail is better (for both functions).
Comment 5 Jean Bréfort 2008-02-06 17:04:25 UTC
Created attachment 104572 [details] [review]
enhanced patch

In the end, my preference goes to g_return_if_fail since this is not a normal situation at all, may be we should issue a report to the user when we encounter such situations instead of that.
Comment 6 Morten Welinder 2008-02-07 00:53:08 UTC
I have a fix in my tree for this.  We should not use g_return_if_fail
since that causes a "CRITICAL".  XL_CHECK_CONDITION or some variant
is it for now.  At some point it might be redirected at the gui.
Comment 7 Morten Welinder 2008-02-07 23:28:29 UTC
This problem has been fixed in our software repository. The fix will go into the next software release. Thank you for your bug report.