bugGNU Octave - Bugs: bug #57284, Crash in MEX file when outputs are...

 
 

bug #57284: Crash in MEX file when outputs are not used

Submitted by:  Guillaume <gyom>
Submitted on:  Thu 21 Nov 2019 01:02:48 PM UTC  
 
Category:  Libraries Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Wont Fix Assigned to:  None
Originator Name:  Guillaume Open/Closed:  Closed
Release:  dev Operating System:  GNU/Linux

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Fri 22 Nov 2019 02:11:02 PM UTC, comment #16: 

Closing report as "Won't Fix".

Rik <rik5>
Project Administrator
Fri 22 Nov 2019 10:07:28 AM UTC, comment #15: 

I think it is fair to leave it as is. What got me surprised here was that a MEX file that seemed to be working fine repeatedly on Matlab would segfault with Octave (for something as specific as having a fourth output argument or not). But this highlights an issue with the implementation in the MEX file and it would actually be better if Matlab also crashed in that situation.

Guillaume <gyom>
Thu 21 Nov 2019 06:38:32 PM UTC, comment #14: 

We can allocate an uniitialized array for plhs because normally it is used to set values.  But prhs is typically accessed.  So if we allocate an uninitialized array, we'd still end up with crashes.  And if we set the elements to nullptr, then we still have crashes because the functions like mxGetScalar access their arguments without checking.  My recollection is that Matlab does the same but my understanding may be out of date.  Someone could check whether mxGetScalar (NULL) crashes current versions of Matlab or throws some other kind of error.

My preference is to not attempt to fix this "problem" for either inputs or outputs unless someone can find some documentation that says that Matlab guarantees N output (or input) arguments will be allocated automatically, and what they should be initialized to.

John W. Eaton <jwe>
Project Administrator
Thu 21 Nov 2019 06:19:36 PM UTC, comment #13: 

For consistency, if we are not expecting programmers to check the number of outputs before using them then we should do something similar for inputs.  Here is a small file "mexin.c" that sums its inputs.

#include "mex.h"

void mexFunction(int nlhs, mxArray *plhs[], int nrhs, const mxArray *prhs[])
{
  int count = mxGetScalar (prhs[0]);

  double sum = 0.0;

  for (int i = 1; i <= count; i++)
    sum += mxGetScalar (prhs[i]);

  plhs[0] = mxCreateDoubleScalar (sum);
}

It is a pretty common problem that programmers can't imagine that their function would be called in the wrong way, and don't put any input validation to verify their assumptions.  For example, a function named "add_two_numbers" is likely meant to be called as "add_two_numbers (a, b)".  But you know that somewhere, at sometime, someone is going to get it wrong.

Unlike the output case, I get a segfault on the input whenever I exceed nrhs by even 1.

mexin (2, 1)
fatal: caught signal Segmentation fault -- stopping myself...
Segmentation fault (core dumped)

(file #47918)

Rik <rik5>
Project Administrator
Thu 21 Nov 2019 05:29:49 PM UTC, comment #12: 

I used a bit of Perl to determine the number of outputs for all functions in the scripts/ directory.

0: 308
1: 1017
2: 149
3: 75
4: 30
5: 15
6: 6
7: 4
8: 4
9: 2

As you can see, most functions have 0 or 1 outputs.  Using 4 outputs would cover 96% of all cases.

Even though memory is cheap these days, it still doesn't feel like the right thing to do to increase this to 32.  If we're going to go with a fixed number then 4 or 8 seem like reasonable choices.  The smaller number, because that already gets 95% of programming cases, the larger number if necessary since that will be the next power-of-2 aligned value and would handle 99.9% of all cases.

Rik <rik5>
Project Administrator
Thu 21 Nov 2019 05:13:38 PM UTC, comment #11: 

I think the +2 result is happening because of padding or alignment or luck.

We do have to allocate at least one output so that a function that produces one output but is called with none will work and allow assignment to ans.

Since we are only talking about allocating an array of pointers, we could allocate a minimum of 32 and it would just cost 256 bytes per function call.  I don't think that's much considering the size of user-define function objects (for example).

John W. Eaton <jwe>
Project Administrator
Thu 21 Nov 2019 05:06:25 PM UTC, comment #10: 

Could it be "feature" of the C compiler? I remember long long time I worked with some C compiler that padded malloc'ed memory.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Thu 21 Nov 2019 04:54:53 PM UTC, comment #9: 

23 is a very odd number.  I don't see a reason to mimic that number exactly.  If we want to make a change I think we should either A) use an absolute number than encompasses 95% of programming cases or B) use a number relative to the number of outputs.

If we use solution A, my guess is that 95% of all functions have less than 4 outputs and therefore that would be a good constant to use.  This could be checked by some clever grepping of the function declarations in the scripts/ directory of Octave to find out what a measured sample for Octave functions is.

If we use solution B, I think the value should be nlhs + 2.  Getting loop counters wrong by 1 is a very common programming error so it should at least be nlhs + 1.  I would throw in the extra pointer because 8 bytes is cheap.

Rik <rik5>
Project Administrator
Thu 21 Nov 2019 04:51:51 PM UTC, comment #8: 

Thanks Rik. I'm a bit embarrassed by my script in comment #6 - maybe I need a little break... I get the same values again: 4 and 23.

Guillaume <gyom>
Thu 21 Nov 2019 04:43:21 PM UTC, comment #7: 

For testing purposes, try compling this file "mexout.c"

#include "mex.h"

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

  const mwSize dims[] = {1, 1, 1};

  if (nrhs != 1 || ! mxIsDouble (prhs[0]))
    mexErrMsgTxt ("input must be a single integer");

  int count = mxGetScalar (prhs[0]);

  for (int i = 0; i < count; i++)
    plhs[i] = mxCreateNumericArray(3, dims, mxDOUBLE_CLASS, mxREAL);
}

Then try invoking with

mexout (1)
mexout (2)
mexout (3)
mexout (4)
...

until you get a segfault.

It's not perfect because your machine may have allocated the pointer plhs[] to a location that has some spare unused memory after it.  Also, I believe that the Octave interpreter always allocates at least one location to handle the automatic variable 'ans'.

When I run this in Octave the threshold for segfault seems to be "nlhs + 2".  For example,

[a,b] = mexout (3)

works, but this does not.

[a,b] = mexout (4)
free(): invalid next size (fast)
fatal: caught signal Aborted -- stopping myself...
Abort (core dumped)

(file #47915)

Rik <rik5>
Project Administrator
Thu 21 Nov 2019 04:30:10 PM UTC, comment #6: 

With the script below, I get that Octave crashes at 4 and Matlab crashes at 23.

for i=0:128
    fprintf('TESTING WITH %d OUTPUTS\n',i);
    fid = fopen('testmex.c','wt');
    fprintf(fid,'#include "mex.h"\n');
    fprintf(fid,'void mexFunction(int nlhs, mxArray *plhs[], int nrhs, const mxArray *prhs[]) {\n');
    fprintf(fid,'const mwSize dims[] = {1, 1, 1};\n');
    for j=0:i-1
        fprintf(fid,'plhs[%d] = mxCreateNumericArray(3, dims, mxDOUBLE_CLASS, mxREAL);\n',j);
    end
    fprintf(fid,'}\n');
    fclose(fid);
    clear testmex
    %mkoctfile --mex testmex.c
    mex  testmex.c
    testmex
end

Guillaume <gyom>
Thu 21 Nov 2019 04:18:27 PM UTC, comment #5: 

jwe: that was what I was wondering when writing my last comment. Matlab might take you by the hand there and fix the mess in the background. I'll try to see if I can find the amount of preallocated items.

Guillaume <gyom>
Thu 21 Nov 2019 04:16:21 PM UTC, comment #4: 

This is extracted from a MEX file I have seen in the wild. What surprised me is that Matlab seems unfazed by it and that Octave also didn't crash if one creates fewer outputs. But, yes, dodgy code. Will suggest a fix upstream.

Guillaume <gyom>
Thu 21 Nov 2019 04:15:11 PM UTC, comment #3: 

gyom: Does this work in Matlab without smashing some memory locations?  If so, then maybe it always over-allocates the array that plhs points to?  But by how much?  At some point, I'm sure Matlab will also fail if you try to assign to plhs[very_large_index] in the mex function and attempt to call it without requesting any outputs.

John W. Eaton <jwe>
Project Administrator
Thu 21 Nov 2019 04:07:14 PM UTC, comment #2: 

This is invalid syntax.  The programmer needs to check the value of the variable 'nlhs', which is the number of left-hand side variables, and only write to variables which exist.  Prior to entering the mex function the Octave interpreter has taken care of allocating an array of pointers to mxArray which is sized according to the value of 'nlhs' and is given as the second argument of the function:

mxArray *plhs[]

It is no surprise that trying to assign to unallocated memory results in segfaults.

This is one of the reasons that Appendix A which discusses the mex and oct file interfaces recommends staying within the Octave language if at all possible.  Once you leave the interpreter you are programming for real and stack overflows, segmentation faults, infinite loops, out-of-memory errors, etc., etc. are all possible.

Rik <rik5>
Project Administrator
Thu 21 Nov 2019 03:53:40 PM UTC, comment #1: 

I don't think it is valid to call this mex function unless it is in an assignment statement that requests at least 4 outputs.  Otherwise, how can Octave know how large to make plhs?

John W. Eaton <jwe>
Project Administrator
Thu 21 Nov 2019 01:02:48 PM UTC, original submission:  

When executing the following MEX file:

#include "mex.h"

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

  const mwSize dims[] = {1, 1, 1};

  plhs[0] = mxCreateNumericArray(3, dims, mxDOUBLE_CLASS, mxREAL);
  plhs[1] = mxCreateNumericArray(3, dims, mxDOUBLE_CLASS, mxREAL);
  plhs[2] = mxCreateNumericArray(3, dims, mxDOUBLE_CLASS, mxREAL);
  plhs[3] = mxCreateNumericArray(3, dims, mxDOUBLE_CLASS, mxREAL);
}

Octave crashes with the following error message:

octave:1> testmex
*** Error in `octave-gui': free(): invalid next size (fast): 0x00007f25d8709710 ***
fatal: caught signal Aborted -- stopping myself...
Abort (core dumped)

It does not happen if called with "[a,b,c,d] = testmex;"

Guillaume <gyom>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #47918:  mexin.c added by rik5 (270B - text/x-csrc)
file #47915:  mexout.c added by rik5 (378B - text/x-csrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dasergatskov (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by gyom (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-11-22 rik5 StatusNone => Wont Fix
        Open/ClosedOpen => Closed
    2019-11-21 rik5 Attached File- => Added mexin.c, #47918
    2019-11-21 rik5 Attached File- => Added mexout.c, #47915
    2019-11-21 jwe StatusInvalid => None
        Open/ClosedClosed => Open
    2019-11-21 rik5 StatusNone => Invalid
        Open/ClosedOpen => Closed

    Back to the top


    Powered by Savane 3.5