bugGNU Octave - Bugs: bug #50198, Calling "error" from the...

 
 

bug #50198: Calling "error" from the GUI thread lead to segfaults

Submitted by:  Pantxo Diribarne <pantxo>
Submitted on:  Tue 31 Jan 2017 02:14:48 PM UTC  
 
Category: GUISeverity: 3 - Normal
Priority: 5 - NormalItem Group: Segfault, Bus Error, etc.
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: devOperating System: GNU/Linux

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 02 Feb 2018 12:17:11 PM UTC, comment #10:

>> I still think it's odd to have "parse error" for any exception thrown over octave_link, but that is a small price for stopping segfaults.


It is also odd to have the prompt, "octave:7>" displayed in the middle of an error message. Ideally we would directly call "error" from the interpreter thread. For this we could have "error" throw a special exception (say gui_error_exception, in which arguments would be stored) when it is not ran from the interpreter thread, instead of displaying a message before throwing execution_exception. Then octave_link::post_exception could check the exception type:

  • gui_error_exception: retrieve the arguments and post error_callback, which would run "error" with original arguments
  • foreign exception: post rethrow_exception_callback

In any event we can't avoid the try/catch block around suspicious code path if we want to handle foreign exceptions (see e.g. pipe execution error in bug #53010).

Pantxo Diribarne <pantxo>
Project Member
Fri 02 Feb 2018 05:53:10 AM UTC, comment #9:

I checked in the change here (http://hg.savannah.gnu.org/hgweb/octave/rev/13c207835e79). I still think it's odd to have "parse error" for any exception thrown over octave_link, but that is a small price for stopping segfaults.

Rik <rik5>
Project Administrator
Thu 01 Feb 2018 11:03:09 PM UTC, comment #8:

I attached an updated version of the patch where I added a "post_exception" method to octave_link class, so that protecting a piece of code in the gui is done with:

I made the exception_ptr argument const since the exception should be rethrown as is.

(file #43154)

Pantxo Diribarne <pantxo>
Project Member
Wed 31 Jan 2018 06:29:38 AM UTC, comment #7:

The exception pointer could be const or not. There are several Stack Overflow articles on not treating pointer as const and modifying it. See

https://stackoverflow.com/questions/8481147/if-you-catch-an-exception-by-reference-can-you-modify-it-and-rethrow#8481171

https://stackoverflow.com/questions/30580128/may-i-modify-the-value-of-an-exception-inside-a-stdexception-ptr

I suppose one thing to consider is whether we are happy with the phrase "parse error" when something goes wrong with printing. Wouldn't it actually be nice to modify the message to be more accurate?

Rik <rik5>
Project Administrator
Wed 31 Jan 2018 05:46:49 AM UTC, comment #6:

Is there some reason that the argument to Canvas::rethrow_exception_callback can't be "const std::exception_ptr&"?

John W. Eaton <jwe>
Project Administrator
Wed 31 Jan 2018 12:48:42 AM UTC, comment #5:

I verified that the patch works. I removed the testing line which directly called error(). I've uploaded the new patch as well.

It also solves bug #53010 which is cool.

Finishing touches are to create the commit message.

(file #43126)

Rik <rik5>
Project Administrator
Tue 30 Jan 2018 11:02:18 PM UTC, comment #4:

The diff is now attached.

(file #43123)

Pantxo Diribarne <pantxo>
Project Member
Tue 07 Nov 2017 10:18:23 PM UTC, comment #3:

As suggested by Rik in some bug report, I tried the approach of catching octave::execution_exception thrown in the GUI thread by opengl graphics functions and rethrowing them in the interpreter thread using octave_link::post_event. At least Octave is able to recover from error (see attached diff). The output message is a bit messed up though and the error is interpreted as a "parse error":

At that point the Qt figure has recovered its original size so it means unwind_protect_cleanup blocks have been properly executed.

Pantxo Diribarne <pantxo>
Project Member
Tue 31 Jan 2017 03:13:03 PM UTC, comment #2:

Thanks, I'll try to take a look at this soon.

John W. Eaton <jwe>
Project Administrator
Tue 31 Jan 2017 03:00:47 PM UTC, comment #1:

I attached the backtrace of the GUI thread

(file #39626)

Pantxo Diribarne <pantxo>
Project Member
Tue 31 Jan 2017 02:14:48 PM UTC, original submission:

Since I don't know how to provoke an error in the GUI thread, I intensionally added a call to "error" in gl2ps_renderer::draw.

After recompiling with this in place I can see that printing with fltk returns gracefully while with Qt (which runs the the gl2ps_renderer from the GUI thread):

Pantxo Diribarne <pantxo>
Project Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #43154:  rethrow4.patch added by pantxo (3KiB - text/x-patch)
file #43126:  50198.cset added by rik5 (3KiB - application/octet-stream)
file #43123:  rethrow.diff added by pantxo (2KiB - text/x-patch)
file #39626:  btGUI.txt added by pantxo (1KiB - text/plain)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -unavailable- added by rik5 (Updated the item)
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by pantxo (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 02 Feb 2018 05:53:10 AM UTCrik5StatusPatch Submitted=>Fixed
      Open/ClosedOpen=>Closed
    Thu 01 Feb 2018 11:03:09 PM UTCpantxoAttached File-=>Added rethrow4.patch, #43154
      StatusIn Progress=>Patch Submitted
    Wed 31 Jan 2018 06:29:38 AM UTCrik5StatusNone=>In Progress
    Wed 31 Jan 2018 12:49:31 AM UTCrik5Dependencies-=>bugs #53010 is dependent
    Wed 31 Jan 2018 12:48:42 AM UTCrik5Attached File-=>Added 50198.cset, #43126
    Tue 30 Jan 2018 11:02:18 PM UTCpantxoAttached File-=>Added rethrow.diff, #43123
    Tue 31 Jan 2017 03:00:47 PM UTCpantxoAttached File-=>Added btGUI.txt, #39626

    Back to the top


    Powered by Savane 3.1-cleanup1