bugGNU Octave - Bugs: bug #49779, print /non/existing/dir/file.svg...

 
 

bug #49779: print /non/existing/dir/file.svg crashes octave

Submitter:  Muhali <muhali>
Submitted:  Tue 06 Dec 2016 05:03:53 PM UTC
   
 
Category:  Plotting with OpenGL 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

Tue 31 Jan 2017 02:30:24 PM UTC, comment #18: 

Closing this report since the remaining issue is now documented in bug #50198.

Rik <rik5>
Group administrator
Tue 31 Jan 2017 02:18:44 PM UTC, comment #17: 

I posted bug #50198. The test case is somewhat artificial, since I didn't know how to force "error" to be called I added a call to error by hand.

Pantxo Diribarne <pantxo>
Group Member
Tue 31 Jan 2017 01:45:06 PM UTC, comment #16: 

How is it about handling exceptions in the GUI thread?

Could you create a simplified example of just that problem and post it in a new bug report?

Thanks.

John W. Eaton <jwe>
Group administrator
Mon 30 Jan 2017 09:34:47 PM UTC, comment #15: 

I pushed the patch here:
http://hg.savannah.gnu.org/hgweb/octave/rev/46b914a213db

@Rik: I'll let you decide if this report should stay open and where to discuss the real deal, handling exceptions in the GUI thread.

Pantxo Diribarne <pantxo>
Group Member
Mon 30 Jan 2017 09:04:26 PM UTC, comment #14: 

@Pantxo: Yes, this can go on stable.

Rik <rik5>
Group administrator
Mon 30 Jan 2017 08:55:29 PM UTC, comment #13: 

Thanks Avinoam, it works form me. I attached a modified hg changeset. The modifications are:

  • added the change log
  • added spaces before parenthesis in function call
  • added spaces after ! operator
  • "error" accepts the same (template, arguments) structure as "sprintf" so I simplified this,


@Rik: should this go on stable since it avoids an easily triggered segfault?

(file #39616)

Pantxo Diribarne <pantxo>
Group Member
Mon 30 Jan 2017 05:23:40 AM UTC, comment #12: 

The patch from comment #11 attached.

(file #39609)

Avinoam Kalma <avinoam>
Group Member
Mon 30 Jan 2017 05:18:53 AM UTC, comment #11: 

Please review the following diff.
The changes in print.m are according to comment #10.
I have also added another input check in /__print_parse_opts__.
I hope it is correct and I did not miss something.


diff -r 868daa374c49 scripts/plot/util/print.m
--- a/scripts/plot/util/print.m        Tue Nov 15 23:10:15 2016 +0100
+++ b/scripts/plot/util/print.m        Mon Jan 30 07:16:20 2017 +0200
@@ -314,6 +314,22 @@

   opts = __print_parse_opts__ (varargin{:});

+  folder = fileparts (opts.name);
+  if (!isempty(folder))
+      if (!exist(folder,"dir"))
+        str = sprintf ("print: directory %s does not exist", folder);
+        error (str);
+      end
+  end
+
+  fid = fopen (opts.name, "w");
+  if (fid == -1)
+    str = sprintf ("print: cannot open file %s for writing",  opts.name);
+    error (str);
+  end
+  fclose (fid);
+  unlink (opts.name);
+
   opts.pstoedit_cmd = @pstoedit;
   opts.fig2dev_cmd = @fig2dev;
   opts.latex_standalone = @latex_standalone;
diff -r 868daa374c49 scripts/plot/util/private/__print_parse_opts__.m
--- a/scripts/plot/util/private/__print_parse_opts__.m        Tue Nov 15 23:10:15 2016 +0100
+++ b/scripts/plot/util/private/__print_parse_opts__.m        Mon Jan 30 07:16:20 2017 +0200
@@ -262,7 +262,8 @@
   if (any (match))
     default_suffix = suffixes{match};
   else
-    default_suffix = arg_st.devopt;
+    str = sprintf ("__print_parse_opts__: unknown device %s", arg_st.devopt);
+    error (str);
   endif

   if (dot == 0 && ! isempty (arg_st.name))


Avinoam Kalma <avinoam>
Group Member
Sat 28 Jan 2017 02:04:35 PM UTC, comment #10: 

@Rik: I think bug #44387 is the place where the GUI exception handler could be discussed

@Avinoam: could you put your snippet from comment #7 into a patch with a few modifications:

  • use double quotes
  • use ! instead of ~
  • delete the file after fclose (there may be another error afterward, in which case we don't wan't this file to be leftover)

After that we will still need to handle non implemented devices.

Pantxo Diribarne <pantxo>
Group Member
Fri 27 Jan 2017 04:55:06 PM UTC, comment #9: 

@Pantxo: I'm a big fan of the 80/20 rule, i.e., you can get 80% of what you want with just a 20% effort.  That seems to be the case here, and I don't have problems adding another input validation test.

However, we still need to keep a bug report open somewhere (maybe a new one with a clear title) that documents the root problem.  Otherwise we will keep encountering this bug and solving different aspects of it in different files until the code base because littered with workarounds.

Rik <rik5>
Group administrator
Fri 27 Jan 2017 10:41:08 AM UTC, comment #8: 

@Rik: I agree that in many cases using exception instead of carefully checking input is a simpler approach, but in this case, even when we manage to rethrow exceptions from the GUI thread, it means gl2ps will spend eventually a lot of time preparing an eps that can't be written or converted. Imagine


demo light 3
print -dpgn toto.png
print /non-existing/toto.png
print locked-file.png


Anyone of those print commands will lead to an error after 37s on my rather new computer when we could have had it return an error early.


Pantxo Diribarne <pantxo>
Group Member
Fri 27 Jan 2017 06:48:56 AM UTC, comment #7: 

What about the following check:


folder = fileparts (opts.name);
  if (~isempty(folder))
      if (~exist(folder,'dir'))
        str = sprintf ('print: directory %s does not exist', folder);
        error (str);
      end
  end

  fid = fopen (opts.name, 'w')
  if (fid == -1)
    str = sprintf ('print: cannot open file %s for writing',  opts.name);
    error (str);
  end
  fclose (fid);


It is close to Pantxo suggestion (comment #4), and solves this bug and bug #50146. I know that this not the desired solutions, and does not solve other expected and unexpected situations, it can
give an interim fix.

Avinoam Kalma <avinoam>
Group Member
Mon 09 Jan 2017 05:52:57 AM UTC, comment #6: 

I believe that 90% of the crashes will be because of a wrong
directory, so I still think that my trivial patch from comment #3 should be applied, and another exception mechanism that will treat
other expected and unexpected situations should be added later.

Avinoam Kalma <avinoam>
Group Member
Wed 07 Dec 2016 04:22:33 PM UTC, comment #5: 

My concern is that we spend a lot of time coding ever more elaborate input validation tests which still manage to miss some cases.  The whole point of exception handling is being able to handle unexpected situations.

From Stroustroup himself at http://www.stroustrup.com/C++11FAQ.html.


Copying and rethrowing exceptions

How do you catch an exception and then rethrow it on another thread? Use a bit of library magic as described in the standard 18.8.5 Exception Propagation:

    exception_ptr current_exception(); Returns: An exception_ptr object that refers to the currently handled exception (15.3) or a copy of the currently handled exception, or a null exception_ptr object if no exception is being handled. The referenced object shall remain valid at least as long as there is an exception_ptr object that refers to it. ...
    void rethrow_exception(exception_ptr p);
    template<class E> exception_ptr copy_exception(E e); Effects: as if


            try {
                    throw e;
            } catch(...) {
                    return current_exception();
            }

    This is particularly useful for transmitting an exception from one thread to another


Maybe it would take just as much coder time to do this correctly.  I'm adding jwe to the CC list since I don't have a great understanding of the cross-thread issues going on with the GUI.

Rik <rik5>
Group administrator
Wed 07 Dec 2016 03:04:44 PM UTC, comment #4: 

Yes I think Avinoam is right. Since this crash can be triggered by a simple typo, checking print options early is the right way to go for the time being:

  • check the destination directory exists
  • check the user has write access to this directory (open a temporary file for writting there and delete it immediately)
  • check the print device is correct: a typo like "-depcs" instead of "-depsc" also crashes Octave
Pantxo Diribarne <pantxo>
Group Member
Wed 07 Dec 2016 07:33:38 AM UTC, comment #3: 

What about trivial check of directory existance in print.m?


 opts = __print_parse_opts__ (varargin{:});

  folder = fileparts (opts.name);
  if (~isempty(folder))
      if (~exist(folder,'dir'))
        error ('directory does not exist');
      end
  end


Avinoam Kalma <avinoam>
Group Member
Tue 06 Dec 2016 09:17:43 PM UTC, comment #2: 

Confirmed. This bug has been reported in other guises.  The issue is that the GUI has multiple threads with one for Qt plotting and one for Octave processing.  When using C++ exceptions in the Qt thread the system libraries are not channeling the exception to the handler that the Octave main thread has installed.  Instead, no handler is registered so the default terminate function is called which stops the program.

This is a nasty but of cross-thread programming that I don't know how to resolve just yet.

Rik <rik5>
Group administrator
Tue 06 Dec 2016 05:06:02 PM UTC, comment #1: 

sorry, I meant

graphics_toolkit qt ;

Muhali <muhali>
Tue 06 Dec 2016 05:03:53 PM UTC, original submission:  

By doing


graphics_toolkit fltk ;
rmdir /tmp/foo s ;
peaks ;
print /tmp/foo/foo.svg ;


one makes octave-dev crash, like so:

sh: /tmp/foo/foo.svg: No such file or directory
warning: broken pipe
error: gl2ps_renderer::draw: internal pipe error
terminate called after throwing an instance of 'octave::execution_exception'
panic: Aborted -- stopping myself...
attempting to save variables to 'octave-workspace'...
save to 'octave-workspace' complete
Aborted (core dumped)

The stable version just exits after the broken pipe.

Muhali <muhali>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #39616:  print2.patch added by pantxo (2KiB - text/x-diff)
file #39609:  print.diff added by avinoam (1KiB - application/octet-stream)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by pantxo (Posted a comment)
  • -email is unavailable- added by avinoam (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by muhali (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-01-31 rik5 StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2017-01-30 pantxo StatusPatch Submitted Patch Reviewed
    2017-01-30 pantxo Attached File- Added print2.patch, #39616
        Item GroupNone Segfault, Bus Error, etc.
    2017-01-30 avinoam StatusConfirmed Patch Submitted
    2017-01-30 avinoam Attached File- Added print.diff, #39609
    2017-01-26 rik5 Dependencies- bugs #50146 is dependent
    2016-12-07 rik5 Carbon-Copy- Added jwe
    2016-12-06 rik5 StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-ee12.
    Corresponding source code