bugGNU Octave - Bugs: bug #47381, unwind_protect_cleanup block not...

 
 

bug #47381: unwind_protect_cleanup block not run for some errors in C++ code

Submitter:  Rik <rik5>
Submitted:  Thu 10 Mar 2016 06:06:12 PM UTC
   
 
Category:  Interpreter Severity:  4 - Important
Priority:  5 - Normal Item Group:  Incorrect Result
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 22 Mar 2016 10:25:22 PM UTC, comment #6: 

I pushed a cset here (http://hg.savannah.gnu.org/hgweb/octave/rev/ab8760b1245d) which clears the SIGPIPE condition and raises an error to the user.  Marking as fixed and closing report.

Rik <rik5>
Group administrator
Sat 12 Mar 2016 09:17:36 AM UTC, comment #5: 

@Rik: I must say I am completely ignorant when it comes to interaction with the system and signal handling. For example I don't know what will happen when the stream is a file, not a pipe, and the memory of the drive is insufficient. Will the system throw a signal that we should also handle? Will fwrite simply not write as much characters as expected (in which case your new construct is useful anyway)?

Pantxo Diribarne <pantxo>
Group Member
Fri 11 Mar 2016 09:37:16 PM UTC, comment #4: 

I'll try to look at this soon.

John W. Eaton <jwe>
Group administrator
Thu 10 Mar 2016 07:41:54 PM UTC, comment #3: 

The problem is in gl2ps_print.cc. 


      // Copy temporary file to pipe
      gnulib::fseek (tmpf, 0, SEEK_SET);
      char str[256];
      int nread = 1;
      while (! feof (tmpf) && nread)
        {
          nread = gnulib::fread (str, 1, 256, tmpf);
          if (nread)
            gnulib::fwrite (str, 1, nread, fp);
        }


If the pipe is closed, because the command failed, then this code does not detect that condition nor raise an error.  Instead, the call to gnulib::fwrite sends a signal SIGPIPE which is caught by Octave's signal handler over in sighandlers.cc.


#ifdef SIGPIPE
static void
sigpipe_handler (int /* sig */)
{
  octave_signal_caught = 1;

  octave_signals_caught[SIGPIPE] = true;

  // Don't loop forever on account of this.

  if (pipe_handler_error_count++ > 100 && octave_interrupt_state >= 0)
    octave_interrupt_state++;
}
#endif


Since the file being copied is quite large (potentially MB of eps data) the pipe_handler_error_count eventually exceeds 100 and Octave's interrupt state gets set.  Thus, when the drawnow function call eventually returns the interpreter detects that and returns to the command line immediately bypassing the unwind_protect_cleanup blocks.

I tried this


# HG changeset patch
# User Rik <rik@octave.org>
# Date 1457638063 28800
#      Thu Mar 10 11:27:43 2016 -0800
# Node ID 91d825a9f5c993597fb2b85cce490d0b67c29b84
# Parent  067662ac6bfea2cd70b6f176080e330d5317d499
Raise an error if writes to pipe fail (bug #47381).

* gl2ps-print.cc (gl2ps_renderer::draw): When copying temporary file to pipe,
check that write succeeds.  Raise an error if it does not, rather than
letting SIGPIPE interrupt handler eventually deal with the problem.

diff -r 067662ac6bfe -r 91d825a9f5c9 libinterp/corefcn/gl2ps-print.cc
--- a/libinterp/corefcn/gl2ps-print.cc        Wed Mar 09 20:05:27 2016 -0500
+++ b/libinterp/corefcn/gl2ps-print.cc        Thu Mar 10 11:27:43 2016 -0800
@@ -269,12 +269,17 @@ gl2ps_renderer::draw (const graphics_obj
       // Copy temporary file to pipe
       gnulib::fseek (tmpf, 0, SEEK_SET);
       char str[256];
-      int nread = 1;
+      size_t nread, nwrite;
+      nread = 1;
       while (! feof (tmpf) && nread)
         {
           nread = gnulib::fread (str, 1, 256, tmpf);
           if (nread)
-            gnulib::fwrite (str, 1, nread, fp);
+            {
+              nwrite = gnulib::fwrite (str, 1, nread, fp);
+              if (nwrite != nread)
+                error ("gl2ps_renderer::draw: internal pipe error");
+            }
         }
     }
   else


and it seems to work.  The possible changeset is attached to the report as SIGPIPE.cset, and needs review.

One thing I notice is that this sequence works.


sombrero
print -dfoo tst1.png
print -dpng tst2.png
warning: broken pipe


But, it produces the extra message "warning: broken pipe".  This comes from octave_signal_handler in sighandlers.cc.  It is a minor quibble, but maybe it indicates that the patch should take a different direction.  Instead of calling error when a bad pipe condition is detected it could call octave_signal_handler.  That would reset the state of SIGPIPE, as well as print the warning message correctly attached to the command that caused it.

Or maybe we should be setting up Octave to ignore SIGPIPE signals when running this code.  That sounds like more work, and I don't know exactly how to do it.


(file #36599)

Rik <rik5>
Group administrator
Thu 10 Mar 2016 07:29:22 PM UTC, comment #2: 

Adding jwe and panxto to the CC list for review.

Rik <rik5>
Group administrator
Thu 10 Mar 2016 06:24:55 PM UTC, comment #1: 

Small correction.  It is the call to drawnow, not _osmesa_print_ which is the problem.


      drawnow (gl2ps_device{n}, ['|' pipeline{n}]);



Rik <rik5>
Group administrator
Thu 10 Mar 2016 06:06:12 PM UTC, original submission:  

The purpose of the unwind_protect/unwind_protect_cleanup construct is to be able to trap errors and recover if necessary.  Unfortunately, I have discovered at least one exception to this when printing.

I've constructed a test script which tests a path of multiple unwind blocks.


function tst_mult_unwind
unwind_protect
  disp "In unwind block 1");
  x = 1
  subfun1 (x)
unwind_protect_cleanup
  disp "In cleanup block 1");
  x = -x
end_unwind_protect
endfunction

function x = subfun1 (x)
  unwind_protect
    disp "In unwind block 2");
    sombrero;
    x++
    x = subfun2(x)
  unwind_protect_cleanup
    disp "In cleanup block 2");
    x = -2
  end_unwind_protect
endfunction

function x = subfun2 (x)
  unwind_protect
    disp "In unwind block 3");
    # Comment/Uncomment this line to create problem
    #print -dfoo tst.eps
    x++
    error ("error from subfun2");
  unwind_protect_cleanup
    disp "In cleanup block 3");
    x = -3
  end_unwind_protect
endfunction


When run without the call to print I see that each cleanup block is executed.


tst_mult_unwind
In unwind block 1)
x =  1
In unwind block 2)
ans =  1
In unwind block 3)
ans =  2
error: error from subfun2
error: called from
    tst_mult_unwind>subfun2 at line 29 column 5
    tst_mult_unwind>subfun1 at line 17 column 7
    tst_mult_unwind at line 5 column 3
In cleanup block 3)
x = -3
In cleanup block 2)
x = -2
In cleanup block 1)
x = -1


However, when I uncomment the line which calls print I get


tst_mult_unwind
Unknown device: foo
sfopen: gs_parse_file_name failed.
sfopen: gs_parse_file_name failed.
  ./base/gsicc_manage.c:1084: gsicc_open_search(): Could not find default_gray.icc
| ./base/gsicc_manage.c:1690: gsicc_set_device_profile(): cannot find device profile
Unrecoverable error: unknownerror in .special_op
Operand stack:
    defaultdevice
Unrecoverable error: undefined in .uninstallpagedevice
Operand stack:
    defaultdevice
warning: broken pipe
In unwind block 1)
x =  3
In unwind block 2)


The ordering of message in stdout is slightly messed up, but you can see that no cleanup blocks are ever executed.

This isn't about merely having C++ code.  I used mkoctfile to build a DLDFCN which throws an error.  Calling that function in place of print works correctly.

The first problem is why the Octave interpreter is not working correctly when print eventually invokes _osmesa_print_ at the bottom of _opengl_print_.m.  The second question is whether there are other instances where this might happen such as GraphicsMagick or other DLDFCN libraries.

Incidentally, I tested with 'graphics_toolkit gnuplot' and then the script passes.

Rik <rik5>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #36599:  SIGPIPE.cset added by rik5 (1KiB - application/octet-stream)
file #36595:  tst_mult_unwind.m added by rik5 (694B - d2l/unknowntype)
file #36596:  throwerr.cc added by rik5 (318B - text/x-c)
file #36597:  tst_unwind.m added by rik5 (218B - d2l/unknowntype)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2016-03-22 rik5 StatusNone Fixed
        Open/ClosedOpen Closed
    2016-03-10 rik5 Attached File- Added SIGPIPE.cset, #36599
    2016-03-10 rik5 Carbon-Copy- Added jwe
        Carbon-Copy- Added pantxo
    2016-03-10 rik5 Attached File- Added tst_mult_unwind.m, #36595
        Attached File- Added throwerr.cc, #36596
        Attached File- Added tst_unwind.m, #36597

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code