bugGNU Octave - Bugs: bug #52904, "mesh" with input array...

 
 

bug #52904: "mesh" with input array of "logical" causes error

Submitter:  None
Submitted:  Mon 15 Jan 2018 10:42:35 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  Alexandre Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * 4.2.1
Operating System:  * Any Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 17 Jan 2018 07:27:08 PM UTC, comment #15: 

I pushed the fix to stable here (http://hg.savannah.gnu.org/hgweb/octave/rev/60d6914d801b).  Marking as fixed and closing report.

Rik <rik5>
Group administrator
Wed 17 Jan 2018 08:34:05 AM UTC, comment #14: 

This is in Matlab R2016a:

>> h = line ([0 1], [0 1], 'createfcn', @() error ('fail to create'))
Error using @()error('fail to create')
Too many input arguments.

Error while evaluating Line CreateFcn


h =

  Line with properties:

              Color: [0 0.4470 0.7410]
          LineStyle: '-'
          LineWidth: 0.5000
             Marker: 'none'
         MarkerSize: 6
    MarkerFaceColor: 'none'
              XData: [0 1]
              YData: [0 1]
              ZData: [1x0 double]

  Show all properties

>> ishghandle(h)

ans =

     1


The line is created even if the CreateFcn fails.

There seems to be another difference with respect to Octave. If I want the error message to appear, the anonymous function has to be changed (note the additional varargin):

>> h = line ([0 1], [0 1], 'createfcn', @(varargin) error ('fail to create'))
Error using @(varargin)error('fail to create')
fail to create

Error while evaluating Line CreateFcn


h =

  Line with properties:

              Color: [0 0.4470 0.7410]
          LineStyle: '-'
          LineWidth: 0.5000
             Marker: 'none'
         MarkerSize: 6
    MarkerFaceColor: 'none'
              XData: [0 1]
              YData: [0 1]
              ZData: [1x0 double]

  Show all properties


But that is a different compatibility issue. With respect to this bug here, both behave the same.

Markus Mützel <mmuetzel>
Group administrator
Tue 16 Jan 2018 10:03:39 PM UTC, comment #13: 

Yes, this could be a case where the error is allowed to happen, i.e., in the following:


octave:1> function [a] = badfunction()
>   this = isbad();
> end
octave:2> set(groot,"defaultaxescreatefcn", @badfunction);
octave:3> h = mesh(zeros(25));
error: 'isbad' undefined near line 2 column 10
execution error in graphics callback function
octave:4> h
h = -6.3352
octave:5>


All seems well, except for the fact that the callback failed.

If more protection is required around those functions, it should probably be one at a time rather than everything in a single try/catch.  Otherwise the code will still be continuing on to use an incomplete graphics object, if only for just another one or two internal functions...that use could be the source of a segfault.  But, again, maybe the current patch is proper/acceptable.

Dan Sebald <sebald>
Tue 16 Jan 2018 09:50:56 PM UTC, comment #12: 

@Dan: My guess is that we should wrap the entire set of commands in a try/catch block, but it is subject to verification with Matlab.

@Markus: Could you try the following under Matlab


h = line ([0 1], [0 1], 'createfcn', @() error ('fail to create'))


Under Octave, it runs the createfcn which produces an error, but doesn't stop an object from being created.


octave:8> h = line ([0 1], [0 1], 'createfcn', @() error ('fail to create'))
error: fail to create
execution error in graphics callback function
h = -19.524
octave:9> ishghandle (h)
ans = 1



Rik <rik5>
Group administrator
Tue 16 Jan 2018 09:42:17 PM UTC, comment #11: 

The changeset works here and looks like the proper thing to do (but may need adjustment); if the initialization fails, delete the graphics object and error.  And there is no longer a graphics t.k. redraw error:


octave:1> h = mesh(zeros(25)==0);
error: invalid value for array property "zdata"
error: __go_surface__: unable to create graphics handle
error: called from
    surface>__surface__ at line 189 column 5
    surface at line 63 column 19
    mesh at line 77 column 12


and there are empty axes, which I believe was not happening prior to your changeset, so nice work.

There are a couple other functions after the change that aren't "sure-thing" use of the handle h (the h.value() is pretty innocuous).


  xcreatefcn (h);
  xinitialize (h);

  retval = h.value ();


Do those need some checks and cleanup as well?  As I understand it, I would say, "Yes, these need consideration as well."

Please clarify that my understanding of the try/catch is correct.  So, something that is within the try/catch will attempt the code, and if there is an error() somewhere within that attempt, the error() is queued rather than immediately executed.  Otherwise, there could be a problem if the error() path is taken immediately and doesn't come back to the calling location, i.e., the code won't have a chance to clean up any memory that has been assigned and remove things from lists, etc.  (Typically, one doesn't put things on container lists until all is verified correct, rather than put something on a list then take it off if there is a problem, but that's fine.)  I do see an error() inside do_make_graphics_handle():


  if (! bgo)
    error ("gh_manager::do_make_graphics_handle: invalid object type '%s'",
           go_name.c_str ());


which is an example of what I'm pointing out: if error() were to not return, there'd potentially be some memory unaccounted for and leak...maybe not in this particular case, but generally speaking.

OK, so the xset() calls the base properties initialization, and that is where the current example fails, i.e., that "logical" class is not acceptable.  Your changeset covers that case, good.

Now, what about xcreatefcn()?  I assume that routine will look at what the user-set property


     createfcn = [](0x0)


is and if there is a valid function there, execute it.  Given that is a user-defined function, we can reasonably expect that to fail in the course of a user's debugging.  In fact, I can implement a fail:


octave:1> function [a] = badfunction()
>   this = isbad();
> end
octave:2> set(groot,"defaultaxescreatefcn", @badfunction);
octave:3> axes()
error: 'isbad' undefined near line 2 column 10
execution error in graphics callback function


This looks to have no apparent crash problems, but understand that it's likely that what is after this, i.e.,


  xinitialize (h);
  retval = h.value ();


aren't run, so we are still left with the question of whether we should have an incompletely initialized graphics object hanging around.  So, is this another case in which we destroy the graphics object at creation if the createfcn property is not successful?

And xinitialize(); how about that one?  Is that one pretty safe?  I see that xinitialize() is the following:


static void
xinitialize (const graphics_handle& h)
{
  graphics_object go = gh_manager::get_object (h);

  if (go)
    go.initialize ();
}


and the only non-base-graphics-object I see with an initialize is


void
axes::initialize (const graphics_object& go)
{
  base_graphics_object::initialize (go);

  xinitialize (xproperties.get_title ());
  xinitialize (xproperties.get_xlabel ());
  xinitialize (xproperties.get_ylabel ());
  xinitialize (xproperties.get_zlabel ());

  xproperties.sync_positions ();
}


Is it possible that the user could pass into the routine properties for "title", "xlabel", "ylabel" and "zlabel" and those might fail along the way if they are poorly formated strings or have bad non-ascii characters?

Anyway, my thinking right now is we should likely try/catch at least the xcreatefcn(h) as well.

Thoughts?

Dan Sebald <sebald>
Tue 16 Jan 2018 07:47:46 PM UTC, comment #10: 

Agreed.  Octave should not construct a partially valid graphics object.  Probably just need to use a try/catch block in the right place.  Try attached changeset 52904.cset and see what you think.

(file #42930)

Rik <rik5>
Group administrator
Tue 16 Jan 2018 07:26:09 PM UTC, comment #9: 

@Rik:
"Incidentally, running this in Octave results in a segfault because the handle h is created, but it is in an invalid state."
"As an example of what is wrong,
h = surf ('foobar')
ishghandle (h)
error: 'h' undefined near line 1 column 13
get (gca, 'children')
ans = -18.192
h is not defined, but the current axes does contain a surface object."

OK, you've isolated the main issue: that handle still hanging around even after an incomplete graphics object.  I suppose because that handle still resides on the parent graphics object (axes) the update_XYZdata() callbacks still remain active.  Hence the attempted redraw of the surface in the graphics_toolkit.

@Markus:
">> h = surf (Z);
Error using matlab.graphics.chart.primitive.Surface/set
Invalid parameter/value pair arguments.
So at least the development branch seems to behave in this respect."

That seems the most logical thing to do, otherwise "logical" class data would be acceptable as input but not setting zdata directly.  That's just best for consistency sake; but this doesn't agree with what the original post said, i.e., that Matlab converts the logical class data to something else... Hold on, I just looked at the original post's example code, and there's no evidence to suggest that is what Matlab does.  There is "+0" tossed into the code to convert the logical-class data to double-class data prior to the mesh() creation:


a=(abs(z)<0.8) & (abs(z)>0.3); # mmmm, donuts!
mesh(a+0); % workaround, convert to real


I'm not sure if the original submission meant that the above code was something rehabilitated from previous Matlab-valid code or that the "+0" was added to make it work in Octave.

More generally, yeah, I suspect that there needs to be some more fundamental change in the validation test handling in graphics.cc.  I suspect if we were to simply add checks at the .m level to solve the mesh/surface problem, this same issue might eventually manifest somewhere else in some way, such as if a user were to build some custom GUI code/program from graphics objects.

Dan Sebald <sebald>
Tue 16 Jan 2018 06:36:46 PM UTC, comment #8: 

Matlab R2016a:

>> Z = logical (ones (25,25));
>> h = mesh (Z);
Error using matlab.graphics.chart.primitive.Surface/set
While setting the 'ZData' property of 'Surface':
Value must be a scalar, vector or array of numeric type

Error in matlab.graphics.chart.internal.ctorHelper (line 6)
      set(obj, pvpairs{:});

Error in matlab.graphics.chart.primitive.Surface

Error in mesh (line 100)
    hh = matlab.graphics.chart.primitive.Surface('ZData',z,'FaceColor',fc,'EdgeColor','flat', ...

>> get(gca, 'Children')

ans =

  0x0 empty GraphicsPlaceholder array.


Again, a figure with empty axes appears. I added the last command to make completely sure that there isn't anything in that axes.

I think I prefer your option 2) in this case. The validation is already done in graphics.cc. I don't think that we should multiply each test in each .m file. If an error occurs during object construction however, the object should not be returned or added to its parent.
That said I still think that there can be cases where we should validate input in the .m files.

I tried the example from comment #5 on Windows with 4.2.1 and an old 4.3.0+ (from April last year!): both crashed at "h = surf (Z);". Will try with a newer 4.3.0+ later.

Markus Mützel <mmuetzel>
Group administrator
Tue 16 Jan 2018 06:02:02 PM UTC, comment #7: 

@Markus: Wow, Matlab's messages don't seem any more useful than Octave's.  For completeness, could you run the original motivating example:


Z = logical (ones (25,25));
h = mesh (Z);


My guess now is that this is an unsupported syntax and this will also produce an error.

If it does then I think there are two possibilities: 1) increase the the input validation either in surface.m or within graphics.cc, 2) ignore input validation, and make sure that a partially complete graphics object can not be constructed and returned.

As an example of what is wrong,


h = surf ('foobar')
error: set: invalid number of arguments
error: called from
    surface>__surface__ at line 189 column 5
    surface at line 63 column 19
    surf at line 72 column 10
ishghandle (h)
error: 'h' undefined near line 1 column 13
get (gca, 'children')
ans = -18.192
hs = ans
hs = -18.192
get (hs, 'type')
ans = surface


h is not defined, but the current axes does contain a surface object.  That is probably not right, and not what Matlab does.  Maybe the whole object creation needs to be wrapped in an unwind_protect to remove the object if the constructor fails.

Rik <rik5>
Group administrator
Tue 16 Jan 2018 05:36:09 PM UTC, comment #6: 

I see the following in Matlab R2016a:

Z = logical (ones (25,25));
>> Z = logical (ones (25,25));
>> h = surf (Z);
Error using matlab.graphics.chart.primitive.Surface/set
Invalid parameter/value pair arguments.

Error in matlab.graphics.chart.internal.ctorHelper (line 6)
      set(obj, pvpairs{:});

Error in matlab.graphics.chart.primitive.Surface

Error in surf (line 126)
hh = matlab.graphics.chart.primitive.Surface(allargs{:});

>> Z2 = get (h, 'zdata');
Undefined function or variable 'h'.

>> class (Z2)
Undefined function or variable 'Z2'.


So at least the development branch seems to behave in this respect.


In Matlab, a figure with empty axes opens for the invalid "surf" command.

Markus Mützel <mmuetzel>
Group administrator
Tue 16 Jan 2018 04:18:08 PM UTC, comment #5: 

I think we might as well be Matlab-compatible in this regard.  What does this test code return under Matlab?


Z = logical (ones (25,25));
h = surf (Z);
Z2 = get (h, "zdata");
class (Z2)


Incidentally, running this in Octave results in a segfault because the handle h is created, but it is in an invalid state.


octave:2> Z = logical (ones (25,25));
octave:3> h = surf (Z);
error: invalid value for array property "zdata"
error: called from
    surface>__surface__ at line 186 column 5
    surface at line 60 column 19
    surf at line 72 column 10
octave:3> Z2 = get (h, "zdata");
panic: Segmentation fault -- stopping myself...
attempting to save variables to 'octave-coredump'...
save to 'octave-coredump' complete
Segmentation fault


At least on the development branch the object is not created and no segfault happens.


octave:1> Z = logical (ones (25,25));
octave:2> h = surf (Z);
error: invalid value for array property "zdata"
error: called from
    surface>__surface__ at line 189 column 5
    surface at line 63 column 19
    surf at line 72 column 10
octave:2> Z2 = get (h, "zdata");
error: 'h' undefined near line 1 column 11
octave:2> class (Z2)
error: 'Z2' undefined near line 1 column 8



Rik <rik5>
Group administrator
Tue 16 Jan 2018 08:29:31 AM UTC, comment #4: 

OK, a bit trickier than I imagined.  Let me explain what the issues are, i.e., why the crash in 4.2.1 and why the strange error message.  (And I'll probably conclude it is best to handle this by adding a warning message in surface...or do the conversion from logical to something else.)

So, let me start by illustrating that Octave is doing as expected in a way.  "logical" is not a valid class for the zdata property.  I'm not considering the conversion; what I mean is that we can't have "ans" be "logical" in the following:


octave:20> h = mesh(zeros(25));
octave:21> class(get(h,'zdata'))
ans = double


But let me try to force that to happen after having created the valid surface object:


octave:22> set(h,'zdata',zeros(25)==0)
error: invalid value for array property "zdata"


OK, that's well and good, as far as expectations go given that "zdata" property can't be logical.

But that creates a conundrum.  If we convert zdata, i.e., this:

h = mesh(zeros(25)==0);

internally to uint8 (or whatever) prior to continuing on to __go_surface__, we still can't set the zdata property with logical data.  In other words, "logical" is fine at the input, but not directly--kind of strange.

Now the more difficult issue, and why I suspect 4.2.1 crashes.  Notice in the above bad set() command that there is a nice clean error message.  That error message does in fact occur if I set the input of the mesh() routine to logical (I'm going to write in the dump below where I think the problematic result is):


octave:22> h = mesh(zeros(25)==0);
error: invalid value for array property "zdata"
error: called from
    surface>__surface__ at line 198 column 5
    surface at line 63 column 19
    mesh at line 77 column 12
[HERE'S THE PROBLEM ERROR MESSAGE]
error: __gnuplot_draw_axes__: invalid grid data
error: called from
    __gnuplot_draw_axes__ at line 1249 column 11
    __gnuplot_draw_figure__ at line 164 column 17
    __gnuplot_drawnow__ at line 86 column 5
octave:22>


There are two methods here:

1) VALIDATE: There is an array_property::validate (const octave_value& v) in graphics.cc.  That's where the "error: invalid value for array property "zdata"" comes from.  It's very generic, and of course it is in __go_surface__ routine where this happens.  [We could add extra checks in the surface() routine and issue an error prior to calling __go_surface__ and not continue, as an option.]

2) ZDATA (ET AL.) UPDATE: The way that this drawing and re-drawing works is through a listener-like mechanism, update_data, update_xdata, update_ydata, update_zdata, etc.  Here's an example of the surface's routine from graphics.in.h:


    void update_zdata (void)
    {
      update_vertex_normals ();
      set_zlim (zdata.get_limits ());
    }


So, that's where the problem lies.  Even though the mesh()/surface() is failing as it is designed to do for "logical" class data, for some reason the callback method is attempting a redraw.  Exactly how that comes about I don't know.  Does the surface graphics object continue to reside in memory?  Is it bogus memory that the callback is operating on, sure to be prone to program faults?  Anyway, such a thing just shouldn't happen.  Note in the graphics.cc file is a FIXME construct to prevent such mismatched xdata, ydata, zdata errors:


void
surface::properties::update_vertex_normals (void)
{
  if (vertexnormalsmode_is ("auto"))
    {
      Matrix x = get_xdata ().matrix_value ();
      Matrix y = get_ydata ().matrix_value ();
      Matrix z = get_zdata ().matrix_value ();

      int p = z.columns ();
      int q = z.rows ();

      // FIXME: There might be a cleaner way to do this.  When data is changed
      // the update_xdata, update_ydata, update_zdata routines are called in a
      // serial fashion.  Until the final call to update_zdata the matrices
      // will be of mismatched dimensions which can cause an out-of-bound
      // indexing in the code below.  This one-liner prevents calculating
      // normals until dimensions match.
      if (x.columns () != p || y.rows () != q)
        return;

[SNIP]
    }
}


In summary, I'm not quite sure why the failed mesh("logical") routine is continuing on after the error message to incur a second error message during the redraw in the graphics-toolkit.  One would think the above test would prevent that (of course, after that initial error, can we expect normal program flow and tests to function expectedly?).

The following works reasonably:


octave:1> x = [1:25];
octave:2> y = [1:25]';
octave:3> z = zeros(27);
octave:4> mesh(x,y,z)
error: surface: rows (Z) must be the same as length (Y) and columns (Z) must be the same as length (X)
error: called from
    surface>__surface__ at line 131 column 11
    surface at line 63 column 19
    mesh at line 77 column 12


but that's because this test is done in __surface__ in surface.m prior to the __go_surface__ function that creates a surface graphics object.  And the following is a graceful error as well:


octave:4> z = zeros(25);
octave:6> h = mesh(x,y,z)
octave:7> set(h,'zdata',zeros(27))
error: __gnuplot_draw_axes__: invalid grid data
error: called from
    __gnuplot_draw_axes__ at line 1249 column 11
    __gnuplot_draw_figure__ at line 164 column 17
    __gnuplot_drawnow__ at line 86 column 5


but that is likely because the surface object still resides in memory as a valid object in the example.  Whereas, when we attempt such a thing at instantiation and there is an error, the surface object is probably deleted while the callback mechanisms are active.

I really don't feel like digging into that C++ code; rather covering this with plaster by adding some tests or conversions to suface() is the easy way out.  On the other hand, if I were to guess, I'd say that the graphics objects constructors shouldn't activate their callback mechanisms, i.e., the update_xyzcdata(), until all else passes without error.

Dan Sebald <sebald>
Tue 16 Jan 2018 05:54:40 AM UTC, comment #3: 

I suppose that is one solution, and it would mean (I believe) that the zdata ends up stored as something other than logical, which as you pointed out is not a valid type for zdata.  (There may be a similar issue with cdata, but that can come next.)

On the other hand, it seems that something should be done at the C++ level because the behavior/warning isn't very graceful when logical data is passed into the surface function.  If anything, a better warning and not attempting to continue with the plot via toolkit would seem the minimum.

I'm looking through the C++ code right now.  I'll look for another ten minutes or so to see what lines of code are at play.

Dan Sebald <sebald>
Tue 16 Jan 2018 05:42:11 AM UTC, comment #2: 

Confirmed.  I changed the summary because Octave doesn't crash, i.e., exit and return to the shell prompt.  It simply fails to complete the drawing of the surface object and issues an error.

The issue is pretty clear as well.  The line in question is


  h = __go_surface__ (ax, "xdata", x, "ydata", y, "zdata", z, "cdata", c,
                      other_args{:});


At this point, the "zdata" z is still logical.  But according to Matlab's own documentation this is not a legal value.  See http://www.mathworks.com/help/matlab/ref/matlab.graphics.primitive.surface-properties.html#property_d119e937079.

A simple fix would be for the surface m-file to convert logical to some numeric format which is accepted.  It could be double, but that also takes up more space than necessary.  Perhaps uint8 would be better.

Rik <rik5>
Group administrator
Tue 16 Jan 2018 04:50:59 AM UTC, comment #1: 

OK, there's a bug here, thanks.

This fails for all the graphics toolkits, so the problem is up stream.  Here are the dimensions of the data that is being passed into _gnuplot_draw_axes_.m:


octave:2> mesh(zeros(25,25)==0); # "logical" array argument
nr =  25
nc =  25
ans =

    1   25

ans = double
ans =

   25    1

ans = double
ans =

   25   25

ans = logical
error: invalid value for array property "zdata"
error: called from
    surface>__surface__ at line 195 column 5
    surface at line 63 column 19
    mesh at line 77 column 12
ans =

    1   25

ans =

   25    1

ans =

   3   3

ans = err 2
error: __gnuplot_draw_axes__: invalid grid data
error: called from
    __gnuplot_draw_axes__ at line 1249 column 11
    __gnuplot_draw_figure__ at line 164 column 17
    __gnuplot_drawnow__ at line 86 column 5


The dimensions of the zdata are 3 x 3, obviously wrong.

The place this originates from is here:


static Matrix
default_surface_zdata (void)
{
  Matrix m (3, 3, 0.0);

  for (int row = 0; row < 3; row++)
    m(row,row) = 1.0;

  return m;
}


in graphics.cc.  It appears as though the C code is deciding to do nothing if the data is logical so that default data gets propagated onward.  This shouldn't be too difficult of a fix.

Dan Sebald <sebald>
Mon 15 Jan 2018 10:42:35 PM UTC, original submission:  


case 1, works OK:

mesh(zeros(25,25)); # real data


case 2, one line is enough to make Octave immediately exit without any warning.

mesh(zeros(25,25)==0); # "logical" array argument


Looks like a crash to me. It's the type of the argument and not its value that causes the problem.

Expectation: should implicitly convert logical to real, like Matlab does.

My problem didn't arise from a silly experiment; I was rehabilitating some antique Matlab finite difference code that defined sampling points for an arbitrary shape, e.g. something like the following:


t=linspace(-1,1,50);
[x,y]=meshgrid(t,t);
z=x+sqrt(-1)*y;
clear t,x,y;
a=(abs(z)<0.8) & (abs(z)>0.3); # mmmm, donuts!
mesh(a+0); % workaround, convert to real


Version:
GNU Octave Version: 4.2.1
Operating System: Linux 4.13.0-26-generic #29~16.04.2-Ubuntu SMP Tue Jan 9 22:00:44 UTC 2018 x86_64

Also tested on Raspbian (Raspberry Pi). Here I get an almost appropriate error message about invalid "zdata". Should implicitly convert for Matlab compatibility.

The "contour" function doesn't crash on either Ubuntu or Raspbian, but displays the zdata error message.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #42930:  52904.cset added by rik5 (1KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2018-01-17 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2018-01-16 rik5 StatusNeed Info Patch Submitted
    2018-01-16 rik5 Attached File- Added 52904.cset, #42930
    2018-01-16 mmuetzel Operating SystemGNU/Linux Any
    2018-01-16 rik5 StatusNone Need Info
    2018-01-16 rik5 Summary&quot;mesh&quot; with input array of &quot;logical&quot; type causes Octave to crash "mesh" with input array of "logical" causes error

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code