bugGNU Octave - Bugs: bug #54096, mxCreateStructMatrix with a NULL...

 
 

bug #54096: mxCreateStructMatrix with a NULL argument creates a "corrupt" structure

Submitter:  Julien Bect <jbect>
Submitted:  Mon 11 Jun 2018 02:03:44 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 4.4.0 Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 12 Jun 2018 03:23:51 PM UTC, comment #14: 

I verified jwe's fix this morning in csets 25450, 25451.

Marking bug as fixed and closing report.

Rik <rik5>
Group administrator
Mon 11 Jun 2018 09:00:16 PM UTC, comment #13: 

Thanks.  I'm working on a way to integrate tests for MEX functions into the test suite.  I can start by adding tests for these things and bug #51725 and then attempting to fix this issue without undoing the fix for bug #51725.

Once that is done, we'll have a template for MEX function tests and I'm sure a multitude of contributed tests will come rolling in.  :-)

John W. Eaton <jwe>
Group administrator
Mon 11 Jun 2018 08:30:18 PM UTC, comment #12: 

Note that, in Matlab, structures containing NULL pointers can be produced directly, without a MEX-file.

Indeed, consider the struct x obtained as follows:

x = struct ();
x(2).a = 'yeah';


and assume that px is an mxArray* to x.  Then, mxGetField (px, 0, "a") returns NULL, while mxGetField (px, 1, "a") returns a non-NULL pointer.

Julien Bect <jbect>
Mon 11 Jun 2018 07:05:43 PM UTC, comment #11: 

Actually, I concluded in comment #12 that NULL was interpreted as [] in Octave.  Further testing showed that this was not the behavior in Matlab.  In Comment #18 in bug #51725:


Yes. NULL values should be converted to undefined, rather than to empty matrix.

After that change it might be necessary to check whether some of the MEX routines have been relying on that behavior to actually return an empty matrix.


It seems that the second paragraph was prescient.  In some cases we need NULL to indicate an empty matrix and in other cases it should mean undefined.

The particular case in bug #51725 was the variable plhs which is of type mxArray, but is also very special.  Maybe the correct way to handle this is to revert cset 23914 and then for plhs avoid calling mxArray:as_octave_value () if the ptr is NULL?  This would introduce a loop over the number of lhs variables, but that number is usually very small.


Rik <rik5>
Group administrator
Mon 11 Jun 2018 05:57:14 PM UTC, comment #10: 

Comment #12 in bug #51725, by Rik, also says that NULL should be interpreted as [] (0x0).

Julien Bect <jbect>
Mon 11 Jun 2018 05:33:57 PM UTC, comment #9: 

I guess now is the time to come up with a framework for MEX interface tests.  Otherwise we are likely to just continue going around in circles with these kinds of bugs.

John W. Eaton <jwe>
Group administrator
Mon 11 Jun 2018 05:32:38 PM UTC, comment #8: 

This is the changeset that caused the regression:

http://hg.savannah.gnu.org/hgweb/octave/rev/2e4033e1f28f

We can undo that, but then I think we will be reintroducing the problem reported in bug #bug 51725 that resulted in this change.

Or was that change just the wrong thing to do?

John W. Eaton <jwe>
Group administrator
Mon 11 Jun 2018 05:29:03 PM UTC, comment #7: 

The patch solves the problem in the particular case that I initially described, but not from a more general perspective.

Indeed, mxCreateStructArray and mxCreateStructMatrix still create structures initialized with NULL pointers (as they are supposed to, according the Matlab's documentation).

Consider then the following example:


#include "mex.h"

static const char* field_names[] = {"dudule"};

void mexFunction (int nlhs, mxArray *plhs[], int nrhs, const mxArray *prhs[])
{
  if (nlhs > 1)   /* Check number of output arguments */
    mexErrMsgTxt ("Too many output arguments.");

  plhs[0] = mxCreateStructMatrix (1, 1, 1, field_names);
}


In Matlab this is OK, but in Octave 4.4 it results in the same problem as in my original post.


Julien Bect <jbect>
Mon 11 Jun 2018 05:24:00 PM UTC, comment #6: 

My change stores [] instead of NULL.

So, I guess you are saying that the previous code was correct to store NULL.  Could you check in a MEX file that Matlab actually stores NULL instead of [] in this case?

If so, then it is our conversion to an Octave struct array that is wrong.

John W. Eaton <jwe>
Group administrator
Mon 11 Jun 2018 05:14:12 PM UTC, comment #5: 


> Anyway, I think you mean a NULL argument to mxSetFieldByNumber?


In the title, yes, I meant mxSetFieldByNumber.  Can you please change that?


> I'm pretty sure that mxCreateDoubleMatrix (0, 0, mxREAL)
> does not return NULL.


Of course not.


> But maybe Matlab handles NULL specially in this case?


Yes.  My understanding is that, when a structure is created by mxCreateStructureArray, or even by struct () in Matlab, the fields are initialized to NULL, and Matlab treats this as "empty".


> Regardless, we should definitely not be creating invalid
> objects.


They are not invalid from Matlab's point of view...


> However, I'm curious to know whether it is documented
> that Matlab accepts NULL here to mean "empty matrix".


Sort of.  The documentation of mxGetFieldByNumber says that it "returns NULL in C (0 in Fortran) if passed an invalid argument or if there is no value assigned to the specified field."

https://www.mathworks.com/help/matlab/apiref/mxgetfieldbynumber.html

The possibility of a NULL pointer there is thus explicitely documented.  What is not clearly written, however, is the fact that this is interpreted as [] if the field is read (but what else could it be ?).

Julien Bect <jbect>
Mon 11 Jun 2018 05:09:21 PM UTC, comment #4: 

I pushed the following changeset on stable and merged with default:

http://hg.savannah.gnu.org/hgweb/octave/rev/97e64c23fd07

John W. Eaton <jwe>
Group administrator
Mon 11 Jun 2018 04:32:02 PM UTC, comment #3: 

See also bug #51725.

John W. Eaton <jwe>
Group administrator
Mon 11 Jun 2018 03:17:35 PM UTC, comment #2: 

The end of the post disappeared here because of a botched verbatim tag.

Anyway, I think you mean a NULL argument to mxSetFieldByNumber?

I'm pretty sure that mxCreateDoubleMatrix (0, 0, mxREAL) does not return NULL.  But maybe Matlab handles NULL specially in this case?

Regardless, we should definitely not be creating invalid objects.

However, I'm curious to know whether it is documented that Matlab accepts NULL here to mean "empty matrix".

John W. Eaton <jwe>
Group administrator
Mon 11 Jun 2018 02:05:01 PM UTC, comment #1: 

Sorry, the end of my post mysteriously disappeared.

I meant: "like this:"


mxSetFieldByNumber (plhs[0], 0, 0, mxCreateDoubleMatrix (0, 0, mxREAL));


Julien Bect <jbect>
Mon 11 Jun 2018 02:03:44 PM UTC, original submission:  

The following MEX-function:


#include "mex.h"

static const char* field_names[] = {"dudule"};

void mexFunction (int nlhs, mxArray *plhs[], int nrhs, const mxArray *prhs[])
{
  if (nlhs > 1)   /* Check number of output arguments */
    mexErrMsgTxt ("Too many output arguments.");

  plhs[0] = mxCreateStructMatrix (1, 1, 1, field_names);

  mxSetFieldByNumber (plhs[0], 0, 0, NULL);
}


returns in Octave 4.4 a "corrupt" structure that Octave cannot display:


>> toto
                                            ans =

                                              scalar structure containing the fields:

                                                dudule =

error: octave_base_value::print (): wrong type argument '<unknown type>'


The same code works fine in Octave <= 4.2 (which is why I labelled this ticket as "regression") and also in Matlab (and thus is also a "Matlab Compatibility" issue, but one ticket cannot belong to two groups ;-)).

The problem disappears if an empty 0x0 matrix is created manually, like this:


mxSetFieldByNumber (plhs[0], 0, 0, mxCreateDoubleMatrix (0, 0, mxREAL));
-verbatim

Julien Bect <jbect>

 

(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 jwe (Posted a comment)
  • -email is unavailable- added by jbect (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 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-06-12 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2018-06-11 jwe StatusNone Ready For Test

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code