bugGNU Octave - Bugs: bug #49010, mxSetDimensions for cell arrays

 
 

bug #49010: mxSetDimensions for cell arrays

Submitter:  Guillaume <gyom>
Submitted:  Mon 05 Sep 2016 04:20:58 PM UTC
   
 
Category:  Libraries Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  Guillaume Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Fri 09 Sep 2016 02:29:18 PM UTC, comment #4: 

@Rik: Many thanks for this, this is very useful. If it can make it to 4.2, it would be great.

You were entirely right in comment 1: my snippet was just a way to reproduce the issue taken from a larger piece of code and I didn't realize I was introducing another subtlety.

Guillaume <gyom>
Thu 08 Sep 2016 09:48:13 PM UTC, comment #3: 

Octave was not allocating new memory on the heap for the dims array when mxSetDimensions was called.  I fixed that in this cset (http://hg.savannah.gnu.org/hgweb/octave/rev/e337b8e3592c).  Both the original example, and my code which makes a duplicate of the inputs before modifying them, now pass.  Marking as fixed and closing report.

Rik <rik5>
Group administrator
Thu 08 Sep 2016 07:48:41 PM UTC, comment #2: 

I duplicated the array so that I am not working on the constant inputs.  I think this is the correct strategy, but it does still fail which means there likely is a problem with mxSetDimensions in Octave.


#include "mex.h"

void mexFunction (int nlhs, mxArray *plhs[], int nrhs, const mxArray *prhs[]) {

    mxArray *mx[1];
    mwSize *dim = NULL;

    /* Create copy of input, *never* modify an input */
    mx[0] = mxDuplicateArray (prhs[0]);

    mexCallMATLAB (0, NULL, 1, mx, "disp");

    /* assume first input argument has two elements */
    dim = mxMalloc (2 * sizeof (*dim));
    dim[0] = 1; dim[1] = 2;
    mxSetDimensions(mx[0], dim, 2);
    mxFree(dim);

    mexCallMATLAB (0, NULL, 1, mx, "disp");
}




Rik <rik5>
Group administrator
Thu 08 Sep 2016 06:54:47 PM UTC, comment #1: 

It may work in Matlab, but the documentation seems to suggest that anything in the prhs array is supposed to be constant and left unchanged.  Changing the dimensions of a supposedly const mxArray is probably not good practice.

From the documentation (http://www.mathworks.com/help/matlab/matlab_external/matlab-data.html),


Input Argument prhs

An mxArray passed to a MEX-file through the prhs input parameter exists outside the scope of the MEX-file. Do not free memory for any mxArray in the prhs parameter. Additionally, prhs variables are read-only; do not modify them in your MEX-file.


It seems that the correct approach is to duplicate any input array that you may modify.  A note on memory management (http://www.mathworks.com/help/matlab/matlab_external/memory-management-issues.html#f24829) talks about cell arrays


Incorrectly Constructing a Cell or Structure mxArray

Do not call mxSetCell or mxSetField variants with prhs[] as the member array.
Example

In the following example, when the MEX file returns, MATLAB destroys the entire cell array. Since this includes the members of the cell, this implicitly destroys the MEX file's input arguments. This can cause several strange results, generally having to do with the corruption of the caller's workspace, if the right-hand side argument used is a temporary array (for example, a literal or the result of an expression):

myfunction('hello')
/* myfunction is the name of your MEX file and your code
/* contains the following:    */

    mxArray *temp = mxCreateCellMatrix(1,1);
      ...
    mxSetCell(temp, 0, prhs[0]);  /* INCORRECT */

Solution

Make a copy of the right-hand side argument with mxDuplicateArray and use that copy as the argument to mxSetCell (or mxSetField variants). For example:

mxSetCell(temp, 0, mxDuplicateArray(prhs[0]));  /* CORRECT */


Rik <rik5>
Group administrator
Mon 05 Sep 2016 04:20:58 PM UTC, original submission:  

There seems to be an issue if the new dimensions array passed to mxSetDimensions is freed explicitly when called on a cell array. The following piece of code illustrates the problem by trying to reshape an array with two elements in a 1x2 array:


#include "mex.h"

void mexFunction(int nlhs, mxArray *plhs[], int nrhs, const mxArray *prhs[]) {
    mxArray **mx;
    mwSize *dim = NULL;

    mx = (mxArray **)prhs;

    mexCallMATLAB(0, NULL, 1, mx, "disp");

    /* assume first input argument has two elements */
    dim = mxMalloc(2 * sizeof(*dim));
    dim[0] = 1; dim[1] = 2;
    mxSetDimensions(mx[0], dim, 2);
    mxFree(dim);

    mexCallMATLAB(0, NULL, 1, mx, "disp");
}



octave:1> test_mexCallMATLAB2([1;2])
   1
   2
   1   2
octave:2> test_mexCallMATLAB2({1;2})
panic: Segmentation fault -- stopping myself...

{
  [1,1] =  1
  [2,1] =  2
}
attempting to save variables to 'octave-workspace'...
save to 'octave-workspace' complete
Segmentation fault


The segfault disappears if the line "mxFree(dim);" is removed. It works as expected in MATLAB. I assume it is valid to free the dimensions array as mxSetDimensions makes a copy of it, if I understand the documentation right:

  http://www.mathworks.com/help/matlab/apiref/mxsetdimensions.html
"mxSetDimensions allocates heap space to hold the input size array."

It works for numeric and struct arrays, but not cell arrays.

Guillaume <gyom>

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2016-09-08 rik5 StatusNone Fixed
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code