bugGNU Octave - Bugs: bug #65431, crash after hgload certain data

 
 

bug #65431: crash after hgload certain data

Submitter:  Dmitri A. Sergatskov <dasergatskov>
Submitted:  Fri 08 Mar 2024 02:34:54 AM 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:  * 9.0.90 Operating System:  * Any
Fixed Release:  9.2.0 (current stable) Planned Release:  9.2.0 (current stable)
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 04 Apr 2024 06:54:15 PM UTC, comment #82: 

Marking as Fixed and closing report.

Rik <rik5>
Group administrator
Tue 12 Mar 2024 09:59:34 PM UTC, comment #81: 

Created separate report as bug# 65457 to address the ambiguous case issue at a later date.

Nicholas Jankowski <nrjank>
Group Member
Mon 11 Mar 2024 09:21:01 PM UTC, comment #80: 

comment #79 update - this behavior appears to exist identically in 8.4.0. so the patches have not altered behavior.

Nicholas Jankowski <nrjank>
Group Member
Mon 11 Mar 2024 09:07:57 PM UTC, comment #79: 

I ran some different combinations and it seems most of the inputs we expect to work do work. Testing matlab, we're missing at one ambiguity with C, not sure yet if it was there before, or if it's from the changes, or if it's just them being ambiguous.  but if we have 3 faces:

matlab

hp = patch ([x1,x2,x1], [y1,y2,y2], [0 1 0])
# plots 3 green faces

hp = patch ([x1,x2,x1], [y1,y2,y2], [0 1 0]')
# plots 3 faces each a different color

hp = patch ([x1,x2,x1], [y1,y2,y2], reshape([0 1 0],1,1,3))
# plots 3 green faces


hp = patch ([x1,x2,x1], [y1,y2,y2], [1 2 3])
Error using patch
Invalid RGB triplet. Specify a three-element vector of values between 0 and 1.

hp = patch ([x1,x2,x1], [y1,y2,y2], [1 2 3])
#  plots 3 faces each a different color.

hp = patch ([x1,x2,x1], [y1,y2,y2], reshape([1 2 3],1,1,3))
Error using patch
Invalid RGB triplet. Specify a three-element vector of values between 0 and 1.

hp = patch ([x1,x2,x1], [y1,y2,y2], [.1 .2 .3])
#  plots 3 faces all the same color.

hp = patch ([x1,x2,x1], [y1,y2,y2], [.1 .2 .3]')
#  plots 3 faces each a different color.

hp = patch ([x1,x2,x1], [y1,y2,y2], reshape([.1 .2 .3],1,1,3))
#  plots 3 faces all the same color.


patched octave:


hp = patch ([x1,x2,x1], [y1,y2,y2], [0 1 0])
hp = patch ([x1,x2,x1], [y1,y2,y2], [0 1 0]')
hp = patch ([x1,x2,x1], [y1,y2,y2], reshape([1 2 3],0 ,1,0))
## plots three faces, 2 dark one light.

hp = patch ([x1,x2,x1], [y1,y2,y2], [1 2 3])
hp = patch ([x1,x2,x1], [y1,y2,y2], [1 2 3]')
hp = patch ([x1,x2,x1], [y1,y2,y2], reshape([1 2 3],1,1,3))
hp = patch ([x1,x2,x1], [y1,y2,y2], [.1 .2 .3])
hp = patch ([x1,x2,x1], [y1,y2,y2], [.1 .2 .3]')
hp = patch ([x1,x2,x1], [y1,y2,y2], reshape([.1 .2 .3],1,1,3))
# each plots 3 faces each a different color.


Nicholas Jankowski <nrjank>
Group Member
Mon 11 Mar 2024 05:55:13 PM UTC, comment #78: 

I can confirm that my Matlab accepts both row and column. I updated my default branch, ran some patch tests that I have on hand and it looks fine to me. Also, the warnings when creating the documentation have disappeared.

Hg200 <hg200>
Mon 11 Mar 2024 04:47:56 PM UTC, comment #77: 

@Liang Tang: You clearly seem to have trouble understanding how color values are set for patch objects. Please, move that discussion to the Discourse forum (or at least don't spam this report):
https://octave.discourse.group/

Markus Mützel <mmuetzel>
Group administrator
Mon 11 Mar 2024 04:45:09 PM UTC, comment #76: 

Tested with the following example in Matlab R2023b:

clf;
t1 = (1/16:1/8:1)' * 2*pi;
t2 = ((1/16:1/8:1)' + 1/32) * 2*pi;
x1 = sin (t1) - 0.8;
y1 = cos (t1);
x2 = sin (t2) + 0.8;
y2 = cos (t2);
figure(1);
patch ([x1,x2], [y1,y2], [1,2]);
figure(2);
patch ([x1,x2], [y1,y2], [1;2]);


Both figure 1 and figure 2 render the same, and no warning appears for either.
Pushed the following change to support both forms in Octave:
https://hg.savannah.gnu.org/hgweb/octave/rev/430eccf64677

Markus Mützel <mmuetzel>
Group administrator
Mon 11 Mar 2024 04:44:00 PM UTC, comment #75: 

Just a reminder.  After you tightening the checks, the legit software features are not disabled.  Thanks.

Liang Tang <lt1234>
Mon 11 Mar 2024 04:15:36 PM UTC, comment #74: 

I pushed a change to add a note about the stricter validation of color data for patch objects:
https://hg.savannah.gnu.org/hgweb/octave/rev/27afeba070ed

Additionally, I slightly simplified the change from comment #71 by setting a scalar value for the patch color, added a commit message, and pushed it with you as the author:
https://hg.savannah.gnu.org/hgweb/octave/rev/6cd9c54e00e7

Re comment #72: The following change should restore handling a n-by-1 vector as the input "C" for the function "patch" work with a color value per face of the patch:
https://hg.savannah.gnu.org/hgweb/octave/rev/0bd22664f028

I hope, we are coming close to getting this correct now.

Does Matlab actually care about the orientation of the input C? Would a 1-by-n vector also work?
Our documentation of the patch function is currently unclear of whether a row vector or a column vector is expected. (It would probably be easy to allow either.)

Markus Mützel <mmuetzel>
Group administrator
Mon 11 Mar 2024 07:24:25 AM UTC, comment #73: 

@Nik: Your patch from comment #71 looks good to me. 👍

We should probably also add a note to the NEWS file that there are better tests whether input for the "cdata" and "facevertexcdata" properties of patch objects are valid. There might be more cases where invalid input "happened to work" before those better tests were added.

Markus Mützel <mmuetzel>
Group administrator
Mon 11 Mar 2024 02:31:13 AM UTC, comment #72: 

i think the ambiguity might stem from the fact that when you do what matlab calls the 'high level calling form' patch (x,y,z,c), the matlab patch docs specifically says C to be a column vector to specify one-color-per-face.

see https://www.mathworks.com/help/matlab/ref/patch.html#br92bi7-1-C

One color per face
n-by-1 vector of colormap colors, where n is the number of faces. The CDataMapping property determines how the values map into the colormap.


it seems that they hold C as equivalent to CData for other uses, so let it be a column vector. if we treat C and Cdata the same when working through the patch input properties in __patch__ we may need to do the same?

Nicholas Jankowski <nrjank>
Group Member
Mon 11 Mar 2024 02:25:25 AM UTC, comment #71: 

if we plan to keep this patch to patch for stable, the following simple fix corrects the cdata call in bar to row vectors:


diff -r c4500208f280 scripts/plot/draw/private/__bar__.m
--- a/scripts/plot/draw/private/__bar__.m       Sat Mar 09 19:29:51 2024 +0100
+++ b/scripts/plot/draw/private/__bar__.m       Sun Mar 10 22:22:46 2024 -0400
@@ -283,10 +283,10 @@

       if (vertical)
         h = patch (hax, xb(:,:,i), yb(:,:,i),
-                   "cdata", i*ones (columns (xb),1), "FaceColor", "flat");
+                   "cdata", i*ones (1, columns (xb)), "FaceColor", "flat");
       else
         h = patch (hax, yb(:,:,i), xb(:,:,i),
-                   "cdata", i*ones (columns (yb),1), "FaceColor", "flat");
+                   "cdata", i*ones (1, columns (yb)), "FaceColor", "flat");
       endif

       if (! isempty (varargin))


(appears we've been using this general ambiguity for some time.  I was looking through old archived matlab docs and i can't find something that doesn't list the cdata and facevertexcdata properties and intended shapes as we see them now. )

Nicholas Jankowski <nrjank>
Group Member
Mon 11 Mar 2024 01:36:59 AM UTC, comment #70: 

@hg200 - yes, CData being a column vector is technically malformed. it should be a row vector to meet the recently enforced documentation.

Nicholas Jankowski <nrjank>
Group Member
Mon 11 Mar 2024 01:33:45 AM UTC, comment #69: 

hmm.  bar calls patch with xdata/ydata/cdata, so i wouldn't think this patch would have caught it.

Nicholas Jankowski <nrjank>
Group Member
Mon 11 Mar 2024 12:32:26 AM UTC, comment #68: 

It is probably not intended. One of those patches broke histogram.


hist(randn(100))
warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.
...


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sun 10 Mar 2024 11:54:43 PM UTC, comment #67: 

I have a question here, since building the documentation on changeset 33185:4eba030c0edc (default branch) throws warnings. It is coming from the hist function, that calls bar:

bar ([0.1;0.3;0.5;0.7], [3;10;18;4], "hist")
warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.

What is happening is:

xb = [0   2   4   6;
      0   2   4   6;
      2   4   6   8;
      2   4   6   8];

yb = [0    0    0    0;
      3   10   18    4;
      3   10   18    4;
      0    0    0    0];

cd = [1; 1; 1; 1];

patch (xb, yb, cd);

This code fails. But it passes when cdata is transposed. Is this intended?



Hg200 <hg200>
Sun 10 Mar 2024 03:44:51 PM UTC, comment #66: 

From my end, with 8.4 (8.2 crashed), the figures are shown in the attached file.  Thanks.

LHS is xyz.  RHS is face/vertex.


Liang Tang <lt1234>
Sun 10 Mar 2024 03:03:06 PM UTC, comment #65: 

Are the number of unique vertices not meeting the number of face color?

Liang Tang <lt1234>
Sun 10 Mar 2024 02:57:27 PM UTC, comment #64: 

Thanks.  Let me check.

Liang Tang <lt1234>
Sun 10 Mar 2024 02:46:16 PM UTC, comment #63: 

I commented out "ismatlab" loop.
With the latest patches I see neither Fig 1 or Fig 2 rendered and lots of warnings

octave:3> sphere_2_face_vs_xyz
warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.
warning: called from
    figure at line 96 column 7
    sphere_2_face_vs_xyz at line 38 column 1

    2.0000         0   31.4589    2.0000         0         0
warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.
warning: called from
    figure at line 96 column 7
    sphere_2_face_vs_xyz at line 88 column 1

warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.
warning: called from
    uimenu at line 97 column 8
    __add_default_menu__ at line 59 column 5
    figure at line 97 column 5
    sphere_2_face_vs_xyz at line 88 column 1

    2.0000         0   31.4589    2.0000         0         0
octave:4> warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.
warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.
warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.
warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.
warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.
warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.
warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.
warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.
warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.
warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.
...


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sun 10 Mar 2024 01:47:22 PM UTC, comment #62: 

For comment # 57. 

I attached a zip file.  This file does not address Nik's request.  I have not been able to reproduce the FaceVertexCData crash today.  If I will not be able to do that, I will have to walk back my earlier comments. 

The attached m file contains two input formats that you prefer.  The basic observations from 8.2 Soft/8.4 Hard on window 64x are at the top of the file.  After a figure is shown, look at the color and also do something (rotate, drag, zoom) to it. I have zero issue with the histogram plot. Performance/inconsistency is always from the two figures with patches.   "inconsistency" is the keyword, what I experienced yesterday may or may not be what I experience today.

In prior communication with Dmitri, Dmitri did not observe the figure window freeze and plot tool issues, I think, on a Linux build.

(file #55820)

Liang Tang <lt1234>
Sun 10 Mar 2024 05:56:25 AM UTC, comment #61: 

Re the first part of comment #57:

The content of "FaceVertexCData" looks like they are assuming that shared vertices had been duplicated. But they didn't actually do that. That looks like a bug in Matlab to me.
Octave is doing a better job at keeping the properties consistent in this case IMHO.

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 11:06:18 PM UTC, comment #60: 

As was pointed out in the other report about input ordering, properties were very poorly defined in octaves documentation for graphics properties.  That was almost Lou remedied recently and will be better documented when octave 9.1 is released. That documentation will specify cdata and facevertexcdata forms following the Matlab published documentation. That specifies the forms we’ve been describing here, and I believe I posted a copy of to that other bug report. That’s the minimum “requirement” - to be compatible with their published Docs.

If you can point to documentation stating that cdata must be able to take the input forms of fvcd, we can look at adding that to the requirement. Otherwise, “it doesn’t say you can’t” is not a requirement. It might be a nice to have, but there are “costs” involved with maintaining support for undocumented, changeable features.

So for now, we focus on getting the published compatibility without crashing.

Nicholas Jankowski <nrjank>
Group Member
Sat 09 Mar 2024 09:39:43 PM UTC, comment #59: 

I will package a small example. It is a different form of the same input I provided.   The outcome is either crash or plot window freeze on my end. Not as straightforward/clean as the prior example.  Thanks.  

Again, I just cannot find the documentation to tell me that only certain combinations of patch inputs would work and others won't.

And I think (better than 50% guess) that the final uniform representation of patch in Octave is face/vertex, not xdata, ydata and zdata. So I no longer look at the xyz data in the handle as geometry.

Liang Tang <lt1234>
Sat 09 Mar 2024 09:27:54 PM UTC, comment #58: 

I stand somewhat corrected: 

if you instead create the patch with x/y/cdatahp.


x = [0 0;0 1;1 1];
y = [0 1;1 0;0 1];
 cdata = reshape (1:6, 3, 2);
hp = patch ("XData",x,"YData",y, "cdata", cdata, "FaceColor", "interp")


things plot fine, the properties become:


>> hp.Faces

ans =

     1     2     3
     4     5     6

>> hp.Vertices

ans =

     0     0
     0     1
     1     0
     0     1
     1     0
     1     1

(so it does expand the vertices here too)

the color data becomes:

>> hp.CData

ans =

     1     4
     2     5
     3     6

>> hp.FaceVertexCData

ans =

     1
     2
     3
     4
     5
     6


if you create CData with one color per face instead, it yells and fails to render because it seems when FaceColor=Interp it requires number of colors to match number of vertices.


Warning: Error creating or updating Patch
 Error in value of property FaceVertexCData
 Number of colors must equal number of vertices.


if you set facecolor = flat, and create a patch with 1 color per face it also expands vertices, but it stores colors as column vectors, which seems wierd.


>> hp = patch ("XData",x,"YData",y, "cdata", [10 20], "FaceColor", "flat");get(hp)

>> hp.Vertices

ans =

     0     0
     0     1
     1     0
     0     1
     1     0
     1     1

>> hp.Faces

ans =

     1     2     3
     4     5     6

>> hp.CData

ans =

    10
    20

>> hp.FaceVertexCData

ans =

    10
    20


If you call it with xydata and FaceVertexCData, it also extends Vertices.

If i call back and call it with Face/vertex and CData for each face, it does NOT extend vertices.

trying to set cdata back to 3x2 afterward gives warnings:

>> set(hp, 'CData', cdata)
Warning: Error creating or updating Patch
 Error in value of property FaceVertexCData
 Number of colors must equal number of vertices.
Warning: Error creating or updating Patch
 Error in value of property FaceVertexCData
 Number of colors must equal number of vertices or faces.


but Vertices hasn't been changed.
 

I'm sure i'm missing some combination there.

SO, it seems the decision to extend vertices is based on whether or not the patch is created with xyzdata.  if so, vertices is extended to have the same number of points.


Nicholas Jankowski <nrjank>
Group Member
Sat 09 Mar 2024 09:07:59 PM UTC, comment #57: 

comment #54 was me.  device keeps logging out of savannah and didn't want to retype when i realized it.

regarding that example - checking compatibility:  it works but produces a warning in matlab:


Warning: Error creating or updating Patch
 Error in value of property FaceVertexCData
 Number of colors must equal number of vertices.


what's actually stored in hp, x/ydata are 3x2 to match the 3 vertices per face. 

Faces and Vertices are both:


>> hp.Faces

ans =

     1     2     3
     2     3     4

>> hp.Vertices

ans =

     0     0
     0     1
     1     0
     1     1



and CData and FaceVertexCData are both set to

>> hp.CData
ans =

     1
     2
     3
     4
     5
     6

>> hp.FaceVertexCData
ans =

     1
     2
     3
     4
     5
     6


So, the CDATA and FVCD are strange, not actually matching the inputs, but they are not changing the geometry.


@Liang Tang - can you please provide an example that crashes when you specify FaceVertexCData?  I have not seen that yet.

We aim for functionality and compatibility, but we cannot mimic every undocumented quirk of the other software. if we don't match documented behavior, that's a bug. if we don't match undocumented behavior, that's a decision after a good discussion, and past experience has typically had us err on the side of not implementing undocumented features that are more that trivial changes. 

At the moment I have been unable to generate a crash with your data when input correctly as FaceVertexCData. So it doesn't require a reformat, just a rename of that property when creating the patch.  if that is not correct we can investigate further with that information.

Nicholas Jankowski <nrjank>
Group Member
Sat 09 Mar 2024 07:43:32 PM UTC, comment #56: 

@Anonymous, I will give u a long response.

(1) I view software documentation as if it is a contract between the developers and users.
(2) For the patch function, regardless matlab or octave, the patch function permissible inputs include all the possible combinations in  this report.
(3) If there are conflicting input configurations, they should be rejected for that purpose, not the mismatch array dimensions due to software internal.
(4) Earlier matlab adapters tend to be very sophisticated. Massive amount of knowledge went into that software.  It is best that we can, without going through the pains, extract the best out of it by closely following it.  So it bothers me somehow that I need to make changes for things that work. 
(5) Lastly I don't understand what the fix would do.  So I just want the exiting data to run.   If the fix does not address the existing conflicts but just preventing it reaching the conflicts for the known paths.  I am sure that crash will continue with a different format. 
(6) Yes.  I can move the cdata.  But, as today, I am crashing, maybe, and cause software to freeze with FaceVertexCData too.

Liang Tang <lt1234>
Sat 09 Mar 2024 07:25:39 PM UTC, comment #55: 

@ the author of comment #54:
Maybe, I wasn't able to explain that before. So, here with an example:

vertices = [0, 0; 0, 1; 1, 0; 1, 1];
faces = [1, 2, 3; 2, 3, 4];
cdata = reshape (1:6, 3, 2);
hp = patch ("Vertices", vertices, "Faces", faces, "cdata", cdata, "FaceColor", "interp");


It wouldn't be possible to keep "cdata" and "facevertexcdata" of that patch object consistent without duplicating vertices.

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 06:50:11 PM UTC, comment #54: 

@liang tang - it seems that the problems you have providing fvcd shaped data to c data would all be alleviated by inputting that as faceverrexcdata. That would also work in Matlab.  Is there some other aspect requiring it to be input as cdata?  For the time being avoiding a hard crash takes precedence over preserving compatibility of undocumented behavior.

@markus - sounds good. Another thought about the vertices, after creating the patch by face/vertices, I should be able to later set() either cdata or facevertexcdata based on the input geometry. x/y/zdata would have to be learned from a get(), but I shouldn’t expect vertices to have changed from the input specification because I changed color data.   I suspect Matlab cdata only looks at xyzdata and fvcd only looks at f/v geometry, so there’s no need to e.g. adjust vertices to match vertex-cdata.  Maybe we do? That might be worth looking into to tackle the fixme. Maybe a good opportunity for me to learn to navigate graphics.cc

Anonymous
Sat 09 Mar 2024 06:41:03 PM UTC, comment #53: 

Pushed the patch from comment #38 to the default branch:
https://hg.savannah.gnu.org/hgweb/octave/rev/4eba030c0edc

We could potentially graft that change to the stable branch when we feel more confident about it. (We are currently very close to a new release.)

Marking as ready for test.

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 06:35:09 PM UTC, comment #52: 

I added a FIXME note about potentially trying to share vertices between adjacent faces and pushed the patch from comment #22 to the stable branch (because it avoids a potential crash on inconsistent input from a user):
https://hg.savannah.gnu.org/hgweb/octave/rev/c4500208f280


Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 06:23:02 PM UTC, comment #51: 

@Liang Tang: "facevertexcdata" supports color data per vertex or per face. Also "cdata" supports color data per vertex or per face. They just cannot be used interchangeably.
Why does that not work for the use cases you have in mind?

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 06:20:32 PM UTC, comment #50: 


> we seem to have had that for quite some time


If that ever worked, I guess it did only by accident.
If users indeed used that undocumented feature, the fix would be easy: Replace "cdata" with "facevertexcdata" in the input to the patch function.
The two properties were never meant to be used interchangeably (except for scalar color values).

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 06:19:59 PM UTC, comment #49: 

I need to let you know that, for a data with face input, it is impossible to extract the vertex properties from the existing info.  However, for a data with vertex input, it is feasible to construct face properties.  It is not always a two-way traffic.

When there is no way to convert to input data format that the software will take, crash or no crash becomes irrelevant.

Liang Tang <lt1234>
Sat 09 Mar 2024 06:13:49 PM UTC, comment #48: 


> I'm not sure if we should implement this un-documented behavior. It might change at any time in the future.


I'd generally agree, except we seem to have had that for quite some time and apparently MATLAB has as well. So wed be turning off a (admittedly broken) feature. I haven't dug through any of their other published information to see if there's any confirmation.

Nicholas Jankowski <nrjank>
Group Member
Sat 09 Mar 2024 06:11:27 PM UTC, comment #47: 

(Early submit)

And so far octave has also permitted submitting fvcd formed data to cdata as well, but just has apparently been mangling patch data with it. (It seems most shapes of cdata to fvcd get rejected by class constraints, but there seems to have been no check on any counts matching, as this and another recent but report point out)

My suggestion would be we get this to a crash and data safe state for 9.1, even if that restricts things to "published compatibility, and then debate on whether to expand inputs to allow undocumented compatibility for later?

Nicholas Jankowski <nrjank>
Group Member
Sat 09 Mar 2024 06:09:33 PM UTC, comment #46: 


> But, MATLAB let's you submit stuff sized for fcvd to the cdata property and then fixes that for you.


I'm not sure if we should implement this un-documented behavior. It might change at any time in the future.

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 06:08:05 PM UTC, comment #45: 

All patch properties need to stay consistent at all time.
It might be that duplication might not be needed in some cases and a more sophisticated algorithm might be able to reduce the duplication. But as long as the patch properties stay consistent, I wouldn't consider this a bug.
If someone would like to work on a changed implementation that reduces the duplication, they are welcome to do that. But that should be done in a different report.

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 06:04:21 PM UTC, comment #44: 

Sorry, not good to attempt pseudo code alongside actual code.

Strict definition: Fvcd wants column vector the length equal to 1, number of faces, or number of vertices.  Cdata wants row vector of length 1 or number of faces, or a matrix of size (vertices-per-face) x (number of faces). 

But, MATLAB let's you submit stuff sized for fcvd to the cdata property and then fixes that for you.

Nicholas Jankowski <nrjank>
Group Member
Sat 09 Mar 2024 05:59:52 PM UTC, comment #43: 

Well, it only needs to happen if the algorithm  is looking to the vertices list for mapping cdata color information, which it seems like it shouldn't do. Color can easily jump across faces when using the size(xdata) form. If it wasn't specified in that form, then there's no cross-face jumping to preserve and the size(xdata) form will have identical colors at all of the repeated nodes.  Also, as a compatibility issue vertices remains unchanged in MATLAB, as its the only matching value I still think that is related to the idx crash jwe pointed to below.

Nicholas Jankowski <nrjank>
Group Member
Sat 09 Mar 2024 05:54:38 PM UTC, comment #42: 

Your comments about "cdata" being a vector with 1xnumel(faces) elements don't look correct to me.
You might have meant the number for faces which is size(faces, 1).
Similarly, "numel(vertices)" doesn't look correct either...

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 05:40:40 PM UTC, comment #41: 


> I'm mostly concerned that setting cdata changes vertices. That shouldn't happen.


That needs to happen because shared vertices might need to be separated:
The (per-vertex) color assigned in "facevertexcdata" applies to all faces neighboring any given vertex.
On the other hand, the (per-vertex) color in "cdata" can be different for each face (also for shared vertices).
So, when a user sets "cdata" manually, shared vertices must be duplicated so that no face shares vertices with another face.

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 05:18:17 PM UTC, comment #40: 

@Nik,

I would prefer you have these added.  Thanks. 

length=numel(faces), <== if user input is face/vertex

or size=size(xdata) <== if user input is xyz_data

I'm mostly concerned that setting cdata changes vertices. That shouldn't happen.   <== this is a bug.  if the issue is within Octave, you can fix it.  If the issue is from 3rd party, it needs to be overwritten.

Liang Tang <lt1234>
Sat 09 Mar 2024 05:02:58 PM UTC, comment #39: 

Regarding the numbers of points/faces/vertices, I tried to layout the compatible behavior in comment #19, but it was a bit of a muddle. Ignoring the rgb dimension:

From the published support Markus linked, cdata should be a row vector of length=numel(faces), or size=size(xdata) for vertices. (Rgb extends in dim3)

Facevertexcdata should be a column vector of length=1, length=numel_(faces), or numel(vertices).

Strict adherence to published data would error on any other input, and inputting CData or FaceVertexCData should result in the other being set in a complementary way.

It appears MATLAB accepts facevertexcdata formatted input for cdata.  It will actually assign a column vector cdata to facevertexcdata instead, and set a compatible cdata. I didn't check it the other way around . 

I'm mostly concerned that setting cdata changes vertices. That shouldn't happen. 


We could choose to implement strict compatibility to the published behavior, but we haven't had that to date and it may break
code people expect to work.
But as some other bugs have revealed we have little input validation for cdata or fvc except for constraints defined in the patch class, adding that would be a plus.

Nicholas Jankowski <nrjank>
Group Member
Sat 09 Mar 2024 05:00:13 PM UTC, comment #38: 

The attached patch fixes the issue described in the last part of comment #22 for me.


(file #55815)

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 04:53:10 PM UTC, comment #37: 

This is not a help forum. It is a bug tracker. I'm going to ignore further questions from you here if you don't understand that.

To answer your comment #36:
"xdata", "ydata", "zdata", and "cdata" need to fulfill conditions for their dimensions. Also, "vertices", "faces", and "facevertexcdata" need to fulfill conditions for their dimensions.
If users chose to mix input from the two groups, they need to be careful to make sure those conditions are fulfilled for the derived properties.

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 04:47:13 PM UTC, comment #36: 

The straight question is when users' input contains face/vertex, not xdata, ydata, zdata, which meet the length requirements, why would you reject the the data because xdata is wrong? 


Liang Tang <lt1234>
Sat 09 Mar 2024 04:43:23 PM UTC, comment #35: 

@Liang Tang: Please, don't side-track this report in yet another direction. This report already handles three different issues. We don't need yet another discussion.
If you have questions about how to use the "patch" function, please ask them on the discourse forum:
https://octave.discourse.group/

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 04:39:00 PM UTC, comment #34: 

Good.  One down. 

The submitted patch specification has face & vertex but not xdata, ydata, zdata. 

I don't do software.  I can only ask questions. 

Do you convert users' input into a uniform representation before calling 3rd party software?  Out of the handler, there are duplicated info.  Which of the geometry and color related are used?  If x,y,z are used and users' spec did not include them, does Octave generate those or 3rd party tool?

  [1,1] = beingdeleted
  [2,1] = busyaction
  [3,1] = buttondownfcn
  [4,1] = children
  [5,1] = clipping
  [6,1] = contextmenu
  [7,1] = createfcn
  [8,1] = deletefcn
  [9,1] = handlevisibility
  [10,1] = hittest
  [11,1] = interruptible
  [12,1] = parent
  [13,1] = pickableparts
  [14,1] = selected
  [15,1] = selectionhighlight
  [16,1] = tag
  [17,1] = type
  [18,1] = userdata
  [19,1] = visible
  [20,1] = alphadatamapping
  [21,1] = ambientstrength
  [22,1] = backfacelighting
  [23,1] = cdata
  [24,1] = cdatamapping
  [25,1] = diffusestrength
  [26,1] = displayname
  [27,1] = edgealpha
  [28,1] = edgecolor
  [29,1] = edgelighting
  [30,1] = facealpha
  [31,1] = facecolor
  [32,1] = facelighting
  [33,1] = facenormals
  [34,1] = facenormalsmode
  [35,1] = faces
  [36,1] = facevertexalphadata
  [37,1] = facevertexcdata
  [38,1] = linestyle
  [39,1] = linewidth
  [40,1] = marker
  [41,1] = markeredgecolor
  [42,1] = markerfacecolor
  [43,1] = markersize
  [44,1] = specularcolorreflectance
  [45,1] = specularexponent
  [46,1] = specularstrength
  [47,1] = vertexnormals
  [48,1] = vertexnormalsmode
  [49,1] = vertices
  [50,1] = xdata
  [51,1] = ydata
  [52,1] = zdata


Liang Tang <lt1234>
Sat 09 Mar 2024 04:26:22 PM UTC, comment #33: 

The final patch has 3586 faces with 3 vertices each. (There is no way dimensions of "xdata" correspond to RGB color.)

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 04:24:17 PM UTC, comment #32: 

That is a very good.  Where did [3   3586] get in? 

When I see 3, it is likely related to rgb.

Liang Tang <lt1234>
Sat 09 Mar 2024 04:19:43 PM UTC, comment #31: 


> Pls send the length of the three variables that you have.


Using your example, I see the following (where `hp2` is a handle to the second patch):

>> size(get(hp2, "xdata"))
ans =

      3   3586

>> size(get(hp2, "cdata"))
ans =

   1837      1


These sizes don't match.

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 04:04:22 PM UTC, comment #30: 

Pls send the length of the three variables that you have.  The size of the variables I see here are in comments #12.

Liang Tang <lt1234>
Sat 09 Mar 2024 03:59:41 PM UTC, comment #29: 

Pushed the patch from comment #8 to the stable branch:
https://hg.savannah.gnu.org/hgweb/octave/rev/2bee38885727

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 03:55:18 PM UTC, comment #28: 


> Based on my understanding of the document, today's matlab documentation indicates that cdata and  FaceVertexCData can take both face and vertex color specification.


Correct. But the size of "cdata" in your example doesn't match for color per face nor for color per vertex.
In case we are looking at a different documentation:
https://www.mathworks.com/help/matlab/ref/matlab.graphics.primitive.patch-properties.html

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 03:50:51 PM UTC, comment #27: 


> the second figure is not rendered at all.


Odd. The figure is rendered for me (basically an empty axes). Only the patch with invalid data isn't rendered.
Is that different for you?

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 03:36:40 PM UTC, comment #26: 


comment #22:

> Turns out the second example in "simple_2_crash.m" is caused by a similar issue than the other one: There was no check that the number of color values is "cdata" matched the number of faces of the patch.
>
> The attached patch avoids that issue for me.


This avoid crash for octave /ASAN as well, though the second figure is not rendered at all.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sat 09 Mar 2024 03:26:12 PM UTC, comment #25: 

H Markus,

The sample example I submitted is based on a run-able example in github and matlab central.  I only duplicated the same geometries and properties 10 times to introduce the crash. The original data used cdata.  I then tried the FaceVertexCData to see if the condition improve.  When I said run-able, I don't mean to say it is run-able as-is in Octave.  To run the original data in Octave, a couple of missing functions will need to be accounted for. Instead of submitting those workaround (and add complexities), I sent the processed data that can be used directly for patch function. That is how simple_2_crash was derived.  The format of the inputs to patch function was never changed.   

Based on my understanding of the document, today's matlab documentation indicates that cdata and  FaceVertexCData can take both face and vertex color specification. If you think I have an incorrect interpretation, please let me know the source that indicates cdata must be altered. I will try again. Thanks.

Liang Tang <lt1234>
Sat 09 Mar 2024 02:06:41 PM UTC, comment #24: 

@Liang Tang: Not sure what you mean. The current behavior of Octave is compatible to what is documented for Matlab. The patch emits a warning for incompatible input.
I don't think we need to deviate from Matlab in this case. If you'd like to set color values per vertex, use "FaceVertexCData" instead.

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 01:54:40 PM UTC, comment #23: 

From application point of view, the color @ vertex is a better choice.  Thanks.

if any one of the face or vertex specification is rejected, it automatically becomes a matlab incompatibility issue.

Liang Tang <lt1234>
Sat 09 Mar 2024 01:28:24 PM UTC, comment #22: 

Turns out the second example in "simple_2_crash.m" is caused by a similar issue than the other one: There was no check that the number of color values is "cdata" matched the number of faces of the patch.

The attached patch avoids that issue for me.
Instead of trying to render the patch with invalid "cdata", it emits the following warning:

warning: opengl_renderer: cdata does not match number of faces or number of vertices per face.  Not rendering.


While looking at that part of the code, I noticed that per-vertex cdata is currently not correctly converted to per-vertex "facevertexcdata". I'll try to look into that. But that latter part should probably go to the default branch.


(file #55814)

Markus Mützel <mmuetzel>
Group administrator
Sat 09 Mar 2024 05:03:16 AM UTC, comment #21: 

This triggers ASAN crash:

octave:1> vert = [0 0 0; 0 1 0; 1 0 1; 1 1 1]
faces = [1 2 3; 2 3 4]
cdata = [1;30; 50; 60]
vert =

   0   0   0
   0   1   0
   1   0   1
   1   1   1

faces =

   1   2   3
   2   3   4

cdata =

    1
   30
   50
   60

octave:4> hp = patch ('faces',faces, 'vertices', vert, 'cdata', cdata, 'facecolor',
'interp', 'cdatamapping', 'direct')
hp = -39.771
octave:5> =================================================================
==3687642==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x608000a0b200 at pc 0x7f112d167225 bp 0x7ffce40c9320 sp 0x7ffce40c9318
READ of size 8 at 0x608000a0b200 thread T0
    #0 0x7f112d167224 in octave::opengl_renderer::draw_patch(octave::patch::properties const&) ../libinterp/corefcn/gl-render.cc:3367
    #1 0x7f112d137d0b in octave::opengl_renderer::draw(octave::graphics_object const&, bool) ../libinterp/corefcn/gl-render.cc:735
    #2 0x7f112d151984 in octave::opengl_renderer::draw_axes_children(octave::axes::properties const&) ../libinterp/corefcn/gl-render.cc:2277
    #3 0x7f112d152c61 in octave::opengl_renderer::draw_axes(octave::axes::properties const&) ../libinterp/corefcn/gl-render.cc:2363
    #4 0x7f112d1378cd in octave::opengl_renderer::draw(octave::graphics_object const&, bool) ../libinterp/corefcn/gl-render.cc:729
    #5 0x7f112d175ceb in octave::opengl_renderer::draw(Matrix const&, bool) ../libinterp/corefcn/gl-render.cc:4182
    #6 0x7f112d13a0c6 in octave::opengl_renderer::draw_figure(octave::figure::properties const&) ../libinterp/corefcn/gl-render.cc:797
    #7 0x7f112d137763 in octave::opengl_renderer::draw(octave::graphics_object const&, bool) ../libinterp/corefcn/gl-render.cc:727
    #8 0x7f112e979a71 in octave::GLWidget::draw(octave::graphics_object) ../libgui/graphics/GLCanvas.cc:79
    #9 0x7f112e97cf17 in octave::GLCanvas::draw(octave_handle const&) ../libgui/graphics/GLCanvas.cc:319
    #10 0x7f112e93355a in octave::Canvas::canvasPaintEvent() ../libgui/graphics/Canvas.cc:286
    #11 0x7f112e97c796 in octave::GLWidget::paintGL() ../libgui/graphics/GLCanvas.cc:215
    #12 0x7f112fb50024 in QOpenGLWidgetPrivate::render() (/lib64/libQt6OpenGLWidgets.so.6+0x9024)
    #13 0x7f112b01ae57 in QWidget::event(QEvent*) (/lib64/libQt6Widgets.so.6+0x21ae57)
    #14 0x7f112afc17b5 in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/lib64/libQt6Widgets.so.6+0x1c17b5)
    #15 0x7f112ec8824f in octave::octave_qapplication::notify(QObject*, QEvent*) ../libgui/src/octave-qobject.cc:148
    #16 0x7f1129d6dbe7 in QCoreApplication::notifyInternal2(QObject*, QEvent*) ../src/corelib/kernel/qcoreapplication.cpp:1118
    #17 0x7f112b012ba9 in QWidgetPrivate::sendPaintEvent(QRegion const&) (/lib64/libQt6Widgets.so.6+0x212ba9)
    #18 0x7f112b026253 in QWidgetRepaintManager::paintAndFlush() (/lib64/libQt6Widgets.so.6+0x226253)
    #19 0x7f112b01b5bb in QWidget::event(QEvent*) (/lib64/libQt6Widgets.so.6+0x21b5bb)
    #20 0x7f112e979196 in octave::FigureWindowBase::event(QEvent*) libgui/graphics/moc-FigureWindow.h:35
    #21 0x7f112afc17b5 in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/lib64/libQt6Widgets.so.6+0x1c17b5)
    #22 0x7f112ec8824f in octave::octave_qapplication::notify(QObject*, QEvent*) ../libgui/src/octave-qobject.cc:148
    #23 0x7f1129d6dbe7 in QCoreApplication::notifyInternal2(QObject*, QEvent*) ../src/corelib/kernel/qcoreapplication.cpp:1118
    #24 0x7f1129d71327 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) ../src/corelib/kernel/qcoreapplication.cpp:1898
    #25 0x7f1129ffd586 in postEventSourceDispatch ../src/corelib/kernel/qeventdispatcher_glib.cpp:243
    #26 0x7f1125f1ae3e in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x54e3e)
    #27 0x7f1125f6fec7 in g_main_context_iterate.constprop.0 (/lib64/libglib-2.0.so.0+0xa9ec7)
    #28 0x7f1125f1877f in g_main_context_iteration (/lib64/libglib-2.0.so.0+0x5277f)
    #29 0x7f1129ffcd5d in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/lib64/libQt6Core.so.6+0x3fcd5d)
    #30 0x7f1129d7a192 in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (/lib64/libQt6Core.so.6+0x17a192)
    #31 0x7f1129d76205 in QCoreApplication::exec() (/lib64/libQt6Core.so.6+0x176205)
    #32 0x7f112ec8b856 in octave::base_qobject::exec() ../libgui/src/octave-qobject.cc:427
    #33 0x7f112ecc0d7e in octave::qt_application::execute() ../libgui/src/qt-application.cc:102
    #34 0x403db8 in main ../src/main-gui.cc:150
    #35 0x7f1125c296cf in __libc_start_call_main (/lib64/libc.so.6+0x296cf)
    #36 0x7f1125c29788 in __libc_start_main_alias_2 (/lib64/libc.so.6+0x29788)
    #37 0x403384 in _start (/home/dima/src/octave/gcc_asan/src/.libs/lt-octave-gui+0x403384)

0x608000a0b200 is located 0 bytes to the right of 96-byte region [0x608000a0b1a0,0x608000a0b200)
allocated by thread T0 here:
    #0 0x7f112f4bb138 in operator new(unsigned long) (/lib64/libasan.so.8+0xbb138)
    #1 0x7f112e91a949 in std::__new_allocator<double>::allocate(unsigned long, void const*) /usr/include/c++/12/bits/new_allocator.h:137
    #2 0x7f112e91a28c in std::allocator_traits<std::allocator<double> >::allocate(std::allocator<double>&, unsigned long) /usr/include/c++/12/bits/alloc_traits.h:464
    #3 0x7f112e91a17a in Array<double, std::allocator<double> >::ArrayRep::allocate(unsigned long) ../liboctave/array/Array.h:198
    #4 0x7f112e94a4fd in Array<double, std::allocator<double> >::ArrayRep::ArrayRep(long) ../liboctave/array/Array.h:167
    #5 0x7f112e948ece in Array<double, std::allocator<double> >::Array(dim_vector const&) ../liboctave/array/Array.h:285
    #6 0x7f112e946f86 in MArray<double>::MArray(dim_vector const&) ../liboctave/array/MArray.h:69
    #7 0x7f112e943cea in NDArray::NDArray(dim_vector const&) ../liboctave/array/dNDArray.h:45
    #8 0x7f112d1bbd2a in convert_cdata ../libinterp/corefcn/graphics.cc:1028
    #9 0x7f112d49b9ec in octave::patch::properties::get_color_data() const ../libinterp/corefcn/graphics.cc:9428
    #10 0x7f112d166145 in octave::opengl_renderer::draw_patch(octave::patch::properties const&) ../libinterp/corefcn/gl-render.cc:3286
    #11 0x7f112d137d0b in octave::opengl_renderer::draw(octave::graphics_object const&, bool) ../libinterp/corefcn/gl-render.cc:735
    #12 0x7f112d151984 in octave::opengl_renderer::draw_axes_children(octave::axes::properties const&) ../libinterp/corefcn/gl-render.cc:2277
    #13 0x7f112d152c61 in octave::opengl_renderer::draw_axes(octave::axes::properties const&) ../libinterp/corefcn/gl-render.cc:2363
    #14 0x7f112d1378cd in octave::opengl_renderer::draw(octave::graphics_object const&, bool) ../libinterp/corefcn/gl-render.cc:729
    #15 0x7f112d175ceb in octave::opengl_renderer::draw(Matrix const&, bool) ../libinterp/corefcn/gl-render.cc:4182
    #16 0x7f112d13a0c6 in octave::opengl_renderer::draw_figure(octave::figure::properties const&) ../libinterp/corefcn/gl-render.cc:797
    #17 0x7f112d137763 in octave::opengl_renderer::draw(octave::graphics_object const&, bool) ../libinterp/corefcn/gl-render.cc:727
    #18 0x7f112e979a71 in octave::GLWidget::draw(octave::graphics_object) ../libgui/graphics/GLCanvas.cc:79
    #19 0x7f112e97cf17 in octave::GLCanvas::draw(octave_handle const&) ../libgui/graphics/GLCanvas.cc:319
    #20 0x7f112e93355a in octave::Canvas::canvasPaintEvent() ../libgui/graphics/Canvas.cc:286
    #21 0x7f112e97c796 in octave::GLWidget::paintGL() ../libgui/graphics/GLCanvas.cc:215
    #22 0x7f112fb50024 in QOpenGLWidgetPrivate::render() (/lib64/libQt6OpenGLWidgets.so.6+0x9024)

SUMMARY: AddressSanitizer: heap-buffer-overflow ../libinterp/corefcn/gl-render.cc:3367 in octave::opengl_renderer::draw_patch(octave::patch::properties const&)
Shadow bytes around the buggy address:
  0x0c10801395f0: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
  0x0c1080139600: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
  0x0c1080139610: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
  0x0c1080139620: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
  0x0c1080139630: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c1080139640:[fa]fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
  0x0c1080139650: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1080139660: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c1080139670: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c1080139680: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c1080139690: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==3687642==ABORTING


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sat 09 Mar 2024 04:59:20 AM UTC, comment #20: 

with a simple example i can recreate the behavior of it malforming the vertices data, but can't seem to trigger a crash.


vert = [0 0 0; 0 1 0; 1 0 1; 1 1 1]
faces = [1 2 3; 2 3 4]
cdata = [1;30; 50; 60]

hp = patch ('faces',faces, 'vertices', vert, 'cdata', cdata, 'facecolor', 'interp', 'cdatamapping', 'direct')

view(3)

get (hp)
                                        ans =

                                          scalar structure containing the fields:

                                            beingdeleted = off
                                            busyaction = queue
                                            buttondownfcn = [](0x0)
                                            children = [](0x1)
                                            clipping = on
                                            contextmenu = [](0x0)
                                            createfcn = [](0x0)
                                            deletefcn = [](0x0)
                                            handlevisibility = on
                                            hittest = on
                                            interruptible = on
                                            parent = -8.6013
                                            pickableparts = visible
                                            selected = off
                                            selectionhighlight = on
                                            tag =
                                            type = patch
                                            userdata = [](0x0)
                                            visible = on
                                            alphadatamapping = scaled
                                            ambientstrength = 0.3000
                                            backfacelighting = reverselit
                                            cdata =

                                                1
                                               10
                                               30
                                               50

                                            cdatamapping = direct
                                            diffusestrength = 0.6000
                                            displayname =
                                            edgealpha = 1
                                            edgecolor =

                                               0   0   0

                                            edgelighting = none
                                            facealpha = 1
                                            facecolor = interp
                                            facelighting = flat
                                            facenormals = [](0x0)
                                            facenormalsmode = auto
                                            faces =

                                               1   2   3
                                               4   5   6

                                            facevertexalphadata = [](0x0)
                                            facevertexcdata =

                                                1
                                               10
                                               30
                                               50

                                            linestyle = -
                                            linewidth = 0.5000
                                            marker = none
                                            markeredgecolor = auto
                                            markerfacecolor = none
                                            markersize = 6
                                            specularcolorreflectance = 1
                                            specularexponent = 10
                                            specularstrength = 0.9000
                                            vertexnormals = [](0x0)
                                            vertexnormalsmode = auto
                                            vertices =

                                               0   0   0
                                               0   1   0
                                               1   0   1
                                               0   1   0
                                               1   0   1
                                               1   1   1

                                            xdata =

                                               0   0
                                               0   1
                                               1   1

                                            ydata =

                                               0   1
                                               1   0
                                               0   1

                                            zdata =

                                               0   0
                                               0   1
                                               1   1



x/y/zdata are fine.  cdata is the input data, not changed to map to x/y/zdata, and duplicate vertices are created at shared points for each face. relatively fewer shared points in this example than the crash example, so only 6 versus intended 4, whereas the crash example had 3x as many. but not sure why it would take a certain number more to crash it.

Nicholas Jankowski <nrjank>
Group Member
Sat 09 Mar 2024 04:32:24 AM UTC, comment #19: 

ok, after comparing matlab and the different octave datasets (really need a minimal crash example), it appears supplying cdata with Faces/Vertices geometry corrupts the Vertices data.

in matlab:
when creating a patch with Faces/Vertices/FaceVertexCData, and supplying FVCData with 1 column x n vertices of data, the patch object is populated with consistent xdata, ydata, zdata, and CData.  size of those is 1 column per face, each element in each column is a point.  vertices/faces/fvcdata match what was input.

when creating a patch with Faces/Vertices/CData, and supplying CData with 1 column x n vertices of data (not a proper format but no error), the patch object is still populated with the same, consistent x/y/zdata _and_ CData as in the previous case, not the data that the user supplied.  FVCData contains the color data supplied, and CData is altered to be consistent.

also, on input validation: if facecolor is interp, then fvcd must be scalar or be Nx1 where N = rows (Vertices). if facecolor is flat, then fvcd must match the number of vertices or faces, error otherwise.

In Octave:

when creating a patch with Faces/Vertices/FaceVertexCData, and supplying FVCData with 1 column x n vertices of data,  the patch object is also populated with consistent xdata, ydata, zdata, and CData and  vertices/faces/fvcdata match what was input.

when creating a patch with Faces/Vertices/CData, and supplying CData with 1 column x n vertices of data, the patch object is still populated with the same, consistent x/y/zdata, but CData contains the supplied data, as does FVCData.  faces matches the input.  vertices for some reason is 3x the size of the number of faces (10758x3 instead of 1837x3)

so, whatever part of the creation algorithm is doing that, it's doing it wrong. based on the number that must be what @jwe found with it indexing fvc from 0:10757.    his patch stops it from iterating past the number of fvc rows.

on validation, unpatched Octave seems perfectly happy letting you set FVCD with any number of rows, when it should error if not 1, nfaces, or nvertices.  markus's patch appears to do that input validation.

I can only get the crash to happen when rotating the image.  I don't know what goes on under the hood, but i guess it calls the patch::update_data function.  (why? nothings changes in the patch object with a view change does it?)  then it iterates through fvc thinking there are 10000 rows, and there aren't. crash.


So, why does patch creation with CData corrupt Vertices?  I'm learning but i'm pretty terrible at trying to jump around graphics.cc from function to function to step through the creation process flow.

Nicholas Jankowski <nrjank>
Group Member
Sat 09 Mar 2024 03:15:31 AM UTC, comment #18: 

no crash in matlab 2023b. same variables saved in attached zip file (load file using -v7 format)

(file #55809)

Nicholas Jankowski <nrjank>
Group Member
Sat 09 Mar 2024 02:53:20 AM UTC, comment #17: 

hmmm... still not attaching and no message. trying as a zip?

(file #55808)

Nicholas Jankowski <nrjank>
Group Member
Sat 09 Mar 2024 02:51:03 AM UTC, comment #16: 

sorry, just realized files didn't attach to comment #14

Nicholas Jankowski <nrjank>
Group Member
Fri 08 Mar 2024 10:49:01 PM UTC, comment #15: 

I think simple_2_crash exposes a different problem than the one Markus's patch addresses.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Fri 08 Mar 2024 10:38:29 PM UTC, comment #14: 

running the simple_2_crash script, both before and after Markus's patch, I saved the contents of the input data and figure contents for the two figures.  files pltinfo.dat and pltinfo_afterpatch.dat contain:

pltdata = plt.data # color data fed to CData or FVCData
vert = [geom.x ; geom.y ; geom.z].' #input vertex data
fac = tri # input faces data

as well as output figure info
h1dat = get(hp1), and h2dat= get(hp2) from:


hp1 = patch(...
'Vertices', [geom.x ; geom.y ; geom.z].',...
'Faces', tri,...
'FaceColor', 'interp',...
'EdgeColor', 'interp',...
'FaceAlpha', plt.face_alpha,...
'LineWidth', plt.edge_width,...
'FaceVertexCData', plt.data ... %  'CData', plt.data  ...      %this crashes  'FaceVertexCData', plt.data ...  %
);


hp2 = patch(...
'Vertices', [geom.x ; geom.y ; geom.z].',...
'Faces', tri,...
'FaceColor', 'interp',...
'EdgeColor', 'interp',...
'FaceAlpha', plt.face_alpha,...
'LineWidth', plt.edge_width,...
'CData', plt.data  ...      %this crashes  'FaceVertexCData', plt.data ...  %
);


will compare h1dat.cdata, h1dat.facevectexcdata, h1dat.xdata, h1dat.ydata, h1dat.zdata, h1dat.vertices, h1dat.faces with same from h2dat to see where the problem arises, and see if there are any differences with the after-patch data.



Nicholas Jankowski <nrjank>
Group Member
Fri 08 Mar 2024 08:48:30 PM UTC, comment #13: 

When I run "simple_2_crash" on octave with ASAN and Markus patch #2:

octave:2> simple_2_crash
=================================================================
==3657835==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62e000107038 at pc 0x7fd892d67225 bp 0x7ffd59b1b6d0 sp 0x7ffd59b1b6c8
READ of size 8 at 0x62e000107038 thread T0
    #0 0x7fd892d67224 in octave::opengl_renderer::draw_patch(octave::patch::properties const&) ../libinterp/corefcn/gl-render.cc:3367
    #1 0x7fd892d37d0b in octave::opengl_renderer::draw(octave::graphics_object const&, bool) ../libinterp/corefcn/gl-render.cc:735
    #2 0x7fd892d51984 in octave::opengl_renderer::draw_axes_children(octave::axes::properties const&) ../libinterp/corefcn/gl-render.cc:2277
    #3 0x7fd892d52c61 in octave::opengl_renderer::draw_axes(octave::axes::properties const&) ../libinterp/corefcn/gl-render.cc:2363
    #4 0x7fd892d378cd in octave::opengl_renderer::draw(octave::graphics_object const&, bool) ../libinterp/corefcn/gl-render.cc:729
    #5 0x7fd892d75ceb in octave::opengl_renderer::draw(Matrix const&, bool) ../libinterp/corefcn/gl-render.cc:4182
    #6 0x7fd892d3a0c6 in octave::opengl_renderer::draw_figure(octave::figure::properties const&) ../libinterp/corefcn/gl-render.cc:797
    #7 0x7fd892d37763 in octave::opengl_renderer::draw(octave::graphics_object const&, bool) ../libinterp/corefcn/gl-render.cc:727
    #8 0x7fd894579a71 in octave::GLWidget::draw(octave::graphics_object) ../libgui/graphics/GLCanvas.cc:79
    #9 0x7fd89457cf17 in octave::GLCanvas::draw(octave_handle const&) ../libgui/graphics/GLCanvas.cc:319
    #10 0x7fd89453355a in octave::Canvas::canvasPaintEvent() ../libgui/graphics/Canvas.cc:286
    #11 0x7fd89457c796 in octave::GLWidget::paintGL() ../libgui/graphics/GLCanvas.cc:215
    #12 0x7fd8956b6024 in QOpenGLWidgetPrivate::render() (/lib64/libQt6OpenGLWidgets.so.6+0x9024)
    #13 0x7fd890a1ae57 in QWidget::event(QEvent*) (/lib64/libQt6Widgets.so.6+0x21ae57)
    #14 0x7fd8909c17b5 in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/lib64/libQt6Widgets.so.6+0x1c17b5)
    #15 0x7fd89488824f in octave::octave_qapplication::notify(QObject*, QEvent*) ../libgui/src/octave-qobject.cc:148
    #16 0x7fd88f76dbe7 in QCoreApplication::notifyInternal2(QObject*, QEvent*) ../src/corelib/kernel/qcoreapplication.cpp:1118
    #17 0x7fd890a12ba9 in QWidgetPrivate::sendPaintEvent(QRegion const&) (/lib64/libQt6Widgets.so.6+0x212ba9)
    #18 0x7fd890a26253 in QWidgetRepaintManager::paintAndFlush() (/lib64/libQt6Widgets.so.6+0x226253)
    #19 0x7fd890a1b5bb in QWidget::event(QEvent*) (/lib64/libQt6Widgets.so.6+0x21b5bb)
    #20 0x7fd894579196 in octave::FigureWindowBase::event(QEvent*) libgui/graphics/moc-FigureWindow.h:35
    #21 0x7fd8909c17b5 in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/lib64/libQt6Widgets.so.6+0x1c17b5)
    #22 0x7fd89488824f in octave::octave_qapplication::notify(QObject*, QEvent*) ../libgui/src/octave-qobject.cc:148
    #23 0x7fd88f76dbe7 in QCoreApplication::notifyInternal2(QObject*, QEvent*) ../src/corelib/kernel/qcoreapplication.cpp:1118
    #24 0x7fd88f771327 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) ../src/corelib/kernel/qcoreapplication.cpp:1898
    #25 0x7fd88f9fd586 in postEventSourceDispatch ../src/corelib/kernel/qeventdispatcher_glib.cpp:243
    #26 0x7fd88baefe3e in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x54e3e)
    #27 0x7fd88bb44ec7 in g_main_context_iterate.constprop.0 (/lib64/libglib-2.0.so.0+0xa9ec7)
    #28 0x7fd88baed77f in g_main_context_iteration (/lib64/libglib-2.0.so.0+0x5277f)
    #29 0x7fd88f9fcd5d in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/lib64/libQt6Core.so.6+0x3fcd5d)
    #30 0x7fd88f77a192 in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (/lib64/libQt6Core.so.6+0x17a192)
    #31 0x7fd88f776205 in QCoreApplication::exec() (/lib64/libQt6Core.so.6+0x176205)
    #32 0x7fd89488b856 in octave::base_qobject::exec() ../libgui/src/octave-qobject.cc:427
    #33 0x7fd8948c0d7e in octave::qt_application::execute() ../libgui/src/qt-application.cc:102
    #34 0x403db8 in main ../src/main-gui.cc:150
    #35 0x7fd88b8296cf in __libc_start_call_main (/lib64/libc.so.6+0x296cf)
    #36 0x7fd88b829788 in __libc_start_main_alias_2 (/lib64/libc.so.6+0x29788)
    #37 0x403384 in _start (/home/dima/src/octave/gcc_asan/src/.libs/lt-octave-gui+0x403384)

0x62e000107038 is located 0 bytes to the right of 44088-byte region [0x62e0000fc400,0x62e000107038)
allocated by thread T0 here:
    #0 0x7fd8950bb138 in operator new(unsigned long) (/lib64/libasan.so.8+0xbb138)
    #1 0x7fd89451a949 in std::__new_allocator<double>::allocate(unsigned long, void const*) /usr/include/c++/12/bits/new_allocator.h:137
    #2 0x7fd89451a28c in std::allocator_traits<std::allocator<double> >::allocate(std::allocator<double>&, unsigned long) /usr/include/c++/12/bits/alloc_traits.h:464
    #3 0x7fd89451a17a in Array<double, std::allocator<double> >::ArrayRep::allocate(unsigned long) ../liboctave/array/Array.h:198
    #4 0x7fd89454a4fd in Array<double, std::allocator<double> >::ArrayRep::ArrayRep(long) ../liboctave/array/Array.h:167
    #5 0x7fd894548ece in Array<double, std::allocator<double> >::Array(dim_vector const&) ../liboctave/array/Array.h:285
    #6 0x7fd894546f86 in MArray<double>::MArray(dim_vector const&) ../liboctave/array/MArray.h:69
    #7 0x7fd894543cea in NDArray::NDArray(dim_vector const&) ../liboctave/array/dNDArray.h:45
    #8 0x7fd892dbbd2a in convert_cdata ../libinterp/corefcn/graphics.cc:1028
    #9 0x7fd89309b9ec in octave::patch::properties::get_color_data() const ../libinterp/corefcn/graphics.cc:9428
    #10 0x7fd892d66145 in octave::opengl_renderer::draw_patch(octave::patch::properties const&) ../libinterp/corefcn/gl-render.cc:3286
    #11 0x7fd892d37d0b in octave::opengl_renderer::draw(octave::graphics_object const&, bool) ../libinterp/corefcn/gl-render.cc:735
    #12 0x7fd892d51984 in octave::opengl_renderer::draw_axes_children(octave::axes::properties const&) ../libinterp/corefcn/gl-render.cc:2277
    #13 0x7fd892d52c61 in octave::opengl_renderer::draw_axes(octave::axes::properties const&) ../libinterp/corefcn/gl-render.cc:2363
    #14 0x7fd892d378cd in octave::opengl_renderer::draw(octave::graphics_object const&, bool) ../libinterp/corefcn/gl-render.cc:729
    #15 0x7fd892d75ceb in octave::opengl_renderer::draw(Matrix const&, bool) ../libinterp/corefcn/gl-render.cc:4182
    #16 0x7fd892d3a0c6 in octave::opengl_renderer::draw_figure(octave::figure::properties const&) ../libinterp/corefcn/gl-render.cc:797
    #17 0x7fd892d37763 in octave::opengl_renderer::draw(octave::graphics_object const&, bool) ../libinterp/corefcn/gl-render.cc:727
    #18 0x7fd894579a71 in octave::GLWidget::draw(octave::graphics_object) ../libgui/graphics/GLCanvas.cc:79
    #19 0x7fd89457cf17 in octave::GLCanvas::draw(octave_handle const&) ../libgui/graphics/GLCanvas.cc:319
    #20 0x7fd89453355a in octave::Canvas::canvasPaintEvent() ../libgui/graphics/Canvas.cc:286
    #21 0x7fd89457c796 in octave::GLWidget::paintGL() ../libgui/graphics/GLCanvas.cc:215
    #22 0x7fd8956b6024 in QOpenGLWidgetPrivate::render() (/lib64/libQt6OpenGLWidgets.so.6+0x9024)

SUMMARY: AddressSanitizer: heap-buffer-overflow ../libinterp/corefcn/gl-render.cc:3367 in octave::opengl_renderer::draw_patch(octave::patch::properties const&)
Shadow bytes around the buggy address:
  0x0c5c80018db0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c5c80018dc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c5c80018dd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c5c80018de0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c5c80018df0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c5c80018e00: 00 00 00 00 00 00 00[fa]fa fa fa fa fa fa fa fa
  0x0c5c80018e10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5c80018e20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5c80018e30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5c80018e40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c5c80018e50: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==3657835==ABORTING


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Fri 08 Mar 2024 08:45:01 PM UTC, comment #12: 


comment #9:

> Dmitri, can u dump out what the code detects as mismatch.  These are what I see.  Thanks. 
>


After I run "simple_2_crash" (with the octave w/ Markus patch v2)
I see two figures and I can rotate/scleup and down w/o problem and w/o warnings.

If I run un-patched octave, sometimes it crashes with

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00007f4e5e4b5d96 in octave::opengl_renderer::draw_patch (this=0x197ef68, props=...) at ../libinterp/corefcn/gl-render.cc:3367
3367                      cc(0) = c(idx, 0), cc(1) = c(idx, 1), cc(2) = c(idx, 2);
[Current thread is 1 (Thread 0x7f4e55bf4380 (LWP 3656186))]

(full trace "crash2_gdb.txt" attached)
but usually it just works (Itried both optimized code and code
compiled with -O))

When it does not crash:

variables in scope: top scope

  Attr   Name         Size                     Bytes  Class
  ====   ====         ====                     =====  =====
         E         1837x2                      29392  double
         E_x       1837x2                      29392  double
         E_y       1837x2                      29392  double
         E_z       1837x2                      29392  double
         ans          1x12                        12  char
         data_3d      1x1                     451250  struct
         geom         1x1                     267018  struct
         plt          1x1                      14718  struct
         pts       1837x3                      44088  double
         tri       3586x3                      86064  double
         x         1837x1                      14696  double
         y         1837x1                      14696  double
         z         1837x1                      14696  double

Total is 36491 elements using 1024806 bytes


This looks like yet another problem.

Dmitri.
--




(file #55807)

Dmitri A. Sergatskov <dasergatskov>
Fri 08 Mar 2024 08:33:07 PM UTC, comment #11: 

Can u tell the length of three arrays involved? Thanks.

facevertexcdata
vertices
faces

The warning message in the prior email is:
 warning: opengl_renderer: number of facevertexcdata values matches neither
number of faces nor number of vertices.  Not rendering.

Liang Tang <lt1234>
Fri 08 Mar 2024 07:54:45 PM UTC, comment #10: 

comment #8:

> Forgot that a single color for the entire patch is also ok.
> Updated patch is attached.
>


That woks for me too.


comment #9:

> Dmitri, can u dump out what the code detects as mismatch. 


ofig does not have any data:

octave:1> who -file "sphere_crash.ofig"
Variables in the file sphere_crash.ofig:

s_oct40



Dmitri.
--




Dmitri A. Sergatskov <dasergatskov>
Fri 08 Mar 2024 07:34:15 PM UTC, comment #9: 

Dmitri, can u dump out what the code detects as mismatch.  These are what I see.  Thanks. 

螟  Attr   Name          Size                     Bytes  Class
  ====   ====          ====                     =====  =====
         E          1837x2                      29392  double
         E_x        1837x2                      29392  double
         E_y        1837x2                      29392  double
         E_z        1837x2                      29392  double
         geom          1x1                     267018  struct
         pts        1837x3                      44088  double
         r          3673x1                      29384  double
         tri        3586x3                      86064  double
         x          1837x1                      14696  double
         xdata      3586x3                      86064  double
         y          1837x1                      14696  double
         ydata      3586x3                      86064  double
         z          1837x1                      14696  double
         zdata      3586x3                      86064  double

Liang Tang <lt1234>
Fri 08 Mar 2024 07:32:29 PM UTC, comment #8: 

Forgot that a single color for the entire patch is also ok.
Updated patch is attached.


(file #55806)

Markus Mützel <mmuetzel>
Group administrator
Fri 08 Mar 2024 06:53:56 PM UTC, comment #7: 

The patch works for me too. I also tried compiling with ASAN and w/ patch I do not see ASAN errors.
Number of warnings though is quite large. If you try to rotate this figure you will get again. But may be this is OK, it is 
the price of messed up data.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Fri 08 Mar 2024 06:28:41 PM UTC, comment #6: 

It looks like the code assumed that the number of color values in "facevertexcdata" would match either the number of faces or the number of vertices of the patch object. But it didn't actually check that.

The attached patch adds such a check.
With it, I see the following in the command window when I try to load the .ofig from comment #0:

>> openfig patch_crash/sphere_crash.ofig
>> warning: opengl_renderer: number of facevertexcdata values matches neither number of faces nor number of vertices.  Not rendering.
warning: opengl_renderer: number of facevertexcdata values matches neither number of faces nor number of vertices.  Not rendering.
warning: opengl_renderer: number of facevertexcdata values matches neither number of faces nor number of vertices.  Not rendering.
warning: opengl_renderer: number of facevertexcdata values matches neither number of faces nor number of vertices.  Not rendering.


And the attached figure is shown.

Octave no longer crashes with it for me.


(file #55804,

Markus Mützel <mmuetzel>
Group administrator
Fri 08 Mar 2024 04:48:34 PM UTC, comment #5: 

I have no idea whether it is the correct thing to do, but the following simple change at least avoids the out of bounds indexing for me.


diff --git a/libinterp/corefcn/graphics.cc b/libinterp/corefcn/graphics.cc
--- a/libinterp/corefcn/graphics.cc
+++ b/libinterp/corefcn/graphics.cc
@@ -9736,7 +9736,7 @@ patch::properties::update_data ()
           if (has_zd)
             zd(ii, jj) = vert(row, 2);

-          if (pervertex)
+          if (pervertex && row < fvc.rows ())
             for (int kk = 0; kk < fvc.columns (); kk++)
               cd(ii, jj, kk) = fvc(row, kk);
         }


John W. Eaton <jwe>
Group administrator
Fri 08 Mar 2024 04:38:31 PM UTC, comment #4: 

I attached a zip.  I ran 8.2.0 win10 64x software rendering.   Thanks. 

There are two plots generated from the m file in the attachment.

(1) no crash - see note below. 

(2) crash - For 8.2.0, 100% success rate in causing a crash after the plot is generated.  I generated the ofig submitted previously in the script. Sometime the crash developed when the figure toolbox was in use.  Sometime, there was no need to do anything.  Octave gui crashed by itself shortly after. 

Note: There is an additional ofig, sphere_no_crash_8_4.ofig, saved from 8.4.0 (win10 64x hardware rendering) after the no_crash ofig loaded.  The rotate tool rotated the colorbar only.

The geometries and properties are derived from https://github.com/otvam/fem_mesh_matlab

(file #55803)

Liang Tang <lt1234>
Fri 08 Mar 2024 02:45:33 PM UTC, comment #3: 

I will provide the commands and geometry data corresponding to the ofig files I sent earlier .  I need time to package a small file for you.  Thanks.

Liang Tang <lt1234>
Fri 08 Mar 2024 02:26:51 PM UTC, comment #2: 

I'm hoping Liang Tang can provide the commands he used to create the figures that are causing the crash. 

from the other bug report, it seems it stems from calling patch using Faces/Vertices form and using CData to specify colors (rather than specifying the FaceVertexCData property, which if i read correctly I think is stored in fvc, or by using the x/y/zdata form which CData is designed to be used with).  How patch manipulates one property when you manipulate the other is sometimes odd, especially when you mix the calling forms. Apparently it's creating fvc and the index separately and the calculations go awry.

if this is correct, then the easy workaround is to avoid mixing the x/y/z/Cdata and Face/Vertices/FaceVertexCdata forms, but it seems matlab allows you to do so.  it may be that with some compatibility testing would tease out what should be happening with those properties and where we might be going wrong.

Nicholas Jankowski <nrjank>
Group Member
Fri 08 Mar 2024 01:15:58 PM UTC, comment #1: 

I'm able to reproduce the crash at that same line in graphics.cc.  See https://hg.savannah.gnu.org/hgweb/octave/file/b5ffc06b39b2/libinterp/corefcn/graphics.cc#l9728 for the context of looping around that assignment.

Applying the attached change, I'm able to display some info about the indices and dimensions at the point of the crash:


octave:1> hgload sphere_crash.ofig
fvc indices (row, kk): 1837, 0
cd indices (ii, jj, kk): 1, 612, 0
cd.dims: 3x3586
fvc.dims: 1837x1
idx.dims: 3x3586
idx.min/max - 1: 0, 10757
ii limit (idx.rows): 3
jj limit (nfaces): 3586
kk limit (fvc.columns): 1
error: __go_patch__: index (1838,_): out of bound 1837 (dimensions are 1837x1), unable to create graphics handle


and it shows that the indexing on FVC is incorrect and if I understand correctly, the row index on that variable in the loop where the crash occurs could be between 0 and 10757 but the row dimension of FVC is just 1837 elements.

I don't understand this code and I don't see an obvious fix.  Someone who knows or can say what is supposed to be happening here will have to help to resolve this problem.


(file #55802)

John W. Eaton <jwe>
Group administrator
Fri 08 Mar 2024 02:34:54 AM UTC, original submission:  

The crash was originally reported in bug #65421, but it looks to be a separate issue. The original report was for Window 8.4; I can reproduce it on linux (different OS/hardware) with the latest 9.0.91 snapshot.
Onn Centos Stream 9 / proprietary Nvidia driver:


  octave:1> hgload sphere_crash.ofig

Thread 9 "QThread" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffed6204640 (LWP 3245625)]
0x00007ffff74a8e13 in octave::patch::properties::update_data (this=this@entry=0x7ffebc846aa0) at ../libinterp/corefcn/graphics.cc:9741
9741                      cd(ii, jj, kk) = fvc(row, kk);


The full coredump is attached.

Dmitri.
--


Dmitri A. Sergatskov <dasergatskov>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #55820:  sphere_2_face_vs_xyz.zip added by lt1234 (616KiB - application/x-zip-compressed)
file #55809:  pltinfo_matlab.zip added by nrjank (332KiB - application/x-zip-compressed - -v7 data file containing variables from matlab 2023b)
file #55808:  pltinfo.zip added by nrjank (433KiB - application/x-zip-compressed - data file containing variables before and after patch.)
file #55807:  crash2_gdb.txt.gz added by dasergatskov (4KiB - application/gzip)
file #55805:  sphere_crash.png added by mmuetzel (36KiB - image/png)
file #55803:  simple_2_crash.zip added by lt1234 (390KiB - application/x-zip-compressed)
file #55802:  graphics-debug-diffs.txt added by jwe (1KiB - text/plain)
file #55799:  sphere_crash.ofig.gz added by dasergatskov (184KiB - application/gzip)
file #55800:  sphere_crash_gdb.txt.gz added by dasergatskov (5KiB - application/gzip)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by hg200 (Posted a comment)
  • -email is unavailable- added by mmuetzel (Updated the item)
  • -email is unavailable- added by lt1234 (Posted a comment)
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by jwe (Updated the item)
  • -email is unavailable- added by nrjank (identified original bug)
  • -email is unavailable- added by dasergatskov (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 23 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-04-04 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 9.2.0 (current stable)
        Planned Release9.1.0 9.2.0 (current stable)
    2024-03-12 nrjank Dependencies- bugs #65457 is dependent
    2024-03-10 lt1234 Attached File- Added eleven_sphere_different_color_face_vertex_vs_xyz_data.JPG, #55821
    2024-03-10 lt1234 Attached File- Added sphere_2_face_vs_xyz.zip, #55820
    2024-03-09 mmuetzel StatusPatch Submitted Ready For Test
    2024-03-09 mmuetzel Attached File- Added bug65431-patch-update-per-vertex-facevertexcdata.patch, #55815
    2024-03-09 mmuetzel Attached File- Added bug65431-patch-inconsistent-cdata.patch, #55814
    2024-03-09 nrjank Attached File- Added pltinfo_matlab.zip, #55809
    2024-03-09 nrjank Attached File- Added pltinfo.zip, #55808
    2024-03-08 dasergatskov Attached File- Added crash2_gdb.txt.gz, #55807
    2024-03-08 mmuetzel Attached File- Added bug65431-patch-inconsistent-facevertexcdata-v2.patch, #55806
    2024-03-08 mmuetzel Attached File- Added bug65431-patch-inconsistent-facevertexcdata.patch, #55804
        Attached File- Added sphere_crash.png, #55805
        StatusNone Patch Submitted
        Planned ReleaseNone 9.1.0
    2024-03-08 lt1234 Attached File- Added simple_2_crash.zip, #55803
    2024-03-08 jwe Attached File- Added graphics-debug-diffs.txt, #55802
    2024-03-08 nrjank Carbon-Copy- Added lt1234
    2024-03-08 dasergatskov Attached File- Added sphere_crash.ofig.gz, #55799
        Attached File- Added sphere_crash_gdb.txt.gz, #55800

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code