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

 
 

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

Submitter:  Pantxo Diribarne <pantxo>
Submitted:  Tue 31 Jan 2017 02:14:48 PM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group 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>
Group 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:


try
{
  // function that could call "error" or throw an exception
}
catch (...)
{
  octave_link::post_exception (std::current_exception ());
}


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

(file #43154)

Pantxo Diribarne <pantxo>
Group 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>
Group 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>
Group 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>
Group administrator
Tue 30 Jan 2018 11:02:18 PM UTC, comment #4: 

The diff is now attached.

(file #43123)

Pantxo Diribarne <pantxo>
Group 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":


octave:1> graphics_toolkit fltk
octave:2> plot (rand (6))
octave:3> print /tmp/toto.svg
error: Expect octave to return gracefully
error: called from
    __opengl_print__ at line 183 column 7
    print at line 552 column 14
octave:3> close all
octave:4> graphics_toolkit qt
octave:5> plot (rand (6))
octave:6> print /tmp/toto.svg
error: Expect octave to return gracefully
octave:7> error: parse error
error: called from
    __opengl_print__ at line 183 column 7
    print at line 552 column 14
octave:7>


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>
Group 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>
Group 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>
Group 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.


diff -r 0224e047fbb9 libinterp/corefcn/gl2ps-print.cc
--- a/libinterp/corefcn/gl2ps-print.cc        Fri Jan 20 21:40:11 2017 +0100
+++ b/libinterp/corefcn/gl2ps-print.cc        Tue Jan 31 15:09:13 2017 +0100
@@ -190,6 +190,7 @@
   void
   gl2ps_renderer::draw (const graphics_object& go, const std::string& print_cmd)
   {
+    error ("Expect Octave to return gracefully");
     static bool in_draw = false;
     static std::string old_print_cmd;
     static GLint buffsize;


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):


octave:1> graphics_toolkit fltk
octave:2> plot (1:10)
octave:3> print toto.eps
error: Expect Octave to return gracefully
error: called from
    __opengl_print__ at line 169 column 7
    print at line 504 column 14
octave:3> close all
octave:4> graphics_toolkit qt
octave:5> plot (1:10)
octave:6> print toto.eps
error: Expect Octave to return gracefully
terminate called after throwing an instance of 'octave::execution_exception'
Aborted


Pantxo Diribarne <pantxo>
Group Member

 

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

Attach Files:
   
   
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
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by pantxo (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-02-02 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2018-02-01 pantxo Attached File- Added rethrow4.patch, #43154
        StatusIn Progress Patch Submitted
    2018-01-31 rik5 StatusNone In Progress
    2018-01-31 rik5 Dependencies- bugs #53010 is dependent
    2018-01-31 rik5 Attached File- Added 50198.cset, #43126
    2018-01-30 pantxo Attached File- Added rethrow.diff, #43123
    2017-01-31 pantxo Attached File- Added btGUI.txt, #39626

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code