bugGNU Octave - Bugs: bug #34868, subsref crashes octave

 
 

bug #34868: subsref crashes octave

Submitter:  Lukas Reichlin <paramaniac>
Submitted:  Sun 20 Nov 2011 05:01:20 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  paramaniac Open/Closed:  * Closed
Release:  * 3.4.3 Operating System:  * Mac OS
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 23 Nov 2011 08:06:34 PM UTC, comment #22: 

I think this issue has been settled.  I'm going to close the bug report now.

Rik <rik5>
Group administrator
Wed 23 Nov 2011 07:06:20 PM UTC, comment #21: 

Oops, if the struct is empty, then there is no type either, so


x() = rhs


is not right.  It is just


x = rhs


John W. Eaton <jwe>
Group administrator
Wed 23 Nov 2011 07:03:08 PM UTC, comment #20: 


x([]) = rhs ==> subsasgn (x, struct ("type", '()', subs [], rhs)


If the indexing struct is empty, then it seems the equivalent expression would be


x() = rhs


That is not valid syntax, but since there is no index, it seems to me that it should be interpreted as


x = rhs


Since subsasgn can not modify its first argument, but is normally used by writing


x = subsasgn (x, idx, rhs)


I think subsasgn should return rhs if the index struct is empty.

John W. Eaton <jwe>
Group administrator
Wed 23 Nov 2011 06:44:30 PM UTC, comment #19: 

I verified the new behavior and added some %!tests for it.  I checked in JWE's changeset here (http://hg.savannah.gnu.org/hgweb/octave/rev/6ead4dc1ca55).

Question, for subsref the returned value with an empty index is the original matrix.  For subsasgn the returned value is what would have been assigned if the assignment operator had not been empty.  This seems odd to me.  It seems like it should be the original matrix again.


x = 1:5;
x([]) = 167
x =

   1   2   3   4   5

BUT

idx = struct ("type", {}, "subs", {});
subsasgn (x, idx), 167)
ans =  167


Any thoughts?

Rik <rik5>
Group administrator
Wed 23 Nov 2011 05:07:44 PM UTC, comment #18: 

Sorry, I never compiled octave from hg sources before.

Lukas Reichlin <paramaniac>
Wed 23 Nov 2011 07:38:58 AM UTC, comment #17: 

How about the attached changeset?  Does it work for you?


(file #24432)

John W. Eaton <jwe>
Group administrator
Tue 22 Nov 2011 11:13:56 PM UTC, comment #16: 

This makes sense to me as well.  I didn't have the correct analogy before.  This is not indexing with a null matrix, but rather indexing with no argument as you said.


x = 1:5;
x()
ans =

   1   2   3   4   5


I wouldn't bother to extend this behavior to user-defined methods.

Rik <rik5>
Group administrator
Tue 22 Nov 2011 10:22:04 PM UTC, comment #15: 

I did not think of this case. But for my needs it would be sufficient if it is handled by the code which led to the core dump, presumably ov.cc. So I don't see a pressing need for bothering user-defined methods with empty structs.

Lukas Reichlin <paramaniac>
Tue 22 Nov 2011 09:59:05 PM UTC, comment #14: 

If we handle subsref and, presumably, subsasgn with empty indexing structs as simple subscript/assignment expressions, then should that be handled at the level of the subsref and subsasgn functions in ov.cc, or should we call the derived class subsref and subsasgn functions so that user-defined subsref and subsasgn methods get a chance to handle this case?

I suspect that letting the user-defined methods handle it could cause trouble for code written for Matlab that does not expect to see empty indexing structs...

John W. Eaton <jwe>
Group administrator
Tue 22 Nov 2011 09:47:43 PM UTC, comment #13: 

Yes, John is correct.

I noticed that from the example below, the first is a 0x0 and the second a 1x0 struct but I believe that this difference is not important here.


octave:1> struct ('type', {}, 'subs', {})
ans =

  0x0 struct array containing the fields:

    type
    subs

octave:2> b.type = "()";
octave:3> b.subs = {1:2};
octave:4> b(2:end)
ans =

  1x0 struct array containing the fields:

    type
    subs

octave:5>


I have a remark on comment #7:
... you have not told subsref what type of indexing you are attempting or what the values of the index are ...
There's no real indexing with this special kind of empty struct. So we don't need to know whether the hypothetic type is "{}", "." or "()" because the result is always the same: subsref just returns its first argument - unchanged.

Lukas Reichlin <paramaniac>
Tue 22 Nov 2011 06:10:04 PM UTC, comment #12: 

Rik,


x([]) == subsref (x, struct ('type', '()', 'subs', '[]'))


What Lukas is saying is that

+vertabim+
subsref (x, struct ('type', {}, 'subs', {}))
-verbatim-

is like no index at all, so should just return X, correct?  This does make sense to me, so I think we should consider it.

John W. Eaton <jwe>
Group administrator
Tue 22 Nov 2011 04:50:51 PM UTC, comment #11: 

Isn't the analogy closer to indexing with an empty matrix than horzcat with empty arguments?

It seems like subsref with a null index is conceptually equivalent to


x = 1:10;
idx = [];
x(idx)
ans = [](0x0)


It appears most reasonable to either flag an error, as Matlab does, or return an empty matrix since that is what the corresponding index code would do.


Rik <rik5>
Group administrator
Mon 21 Nov 2011 11:34:20 PM UTC, comment #10: 

PS: I regard this a bit like horzcat () or similar cases with empty arguments.

Lukas Reichlin <paramaniac>
Mon 21 Nov 2011 11:25:41 PM UTC, comment #9: 

I see your point. But the fact that I discovered the bug/crash after all these years indicates that there can't be much code out there relying on octave throwing an error. IMHO the feature would be much less dangerous than enhancements like Jordi's automatic bsxfun. However, I would still check whether fieldnames s.type and s.subs are present in the 1x0 struct array and only return argument "a" in this special case. For any other empty argument I would throw an error too, which seems to be already the case with existing octave releases.

@jwe: Yes, my intention is to avoid code like

if (numel (s) == 1)
  ret = q.w;
else
  ret = subsref (q.w, s(2:end));
endif


Lukas Reichlin <paramaniac>
Mon 21 Nov 2011 10:24:27 PM UTC, comment #8: 

I agree with JWE.  Matlab flags an error because the function input isn't what it is expecting, and I think Octave should do the same thing.

If you want to amend the patch that is fine.  I checked the emptyness of the input via the number of elements because that value was already available.  If the check for emptyness is moved up to the if statement that checks fieldnames (utils.c:1173) then the error message at line 1235 should be rewritten to something like "second argument must be a non-empty structure ..."

Rik <rik5>
Group administrator
Mon 21 Nov 2011 10:05:02 PM UTC, comment #7: 

I think the error makes sense as you are trying to index an object but given the empty struct, you have not told subsref what type of indexing you are attempting or what the values of the index are.  There is no equivalent for this in the syntax of the scripting language itself, so why should it not be an error here as well?

I guess you are trying to avoid having to check to see whether the subscript argument is empty in your code?

But by avoiding the error and simply returning the original, I think it would be easy to hide actual errors and silently return incorrect results.  I'm not sure that possibility is worth the added convenience.  At least, I think we should discuss this on the maintainers list before making such a change.

Until then, I think something like Rik's patch should be applied to avoid the crash.  But maybe it should just check the number of elements of the indexing struct directly?

John W. Eaton <jwe>
Group administrator
Mon 21 Nov 2011 09:45:49 PM UTC, comment #6: 

Oops, copy-paste error.

>> a = 1:4

b.type = '()'
b.subs = 1:2
subsref (a, b(2:end))

a =

     1     2     3     4


b =

    type: '()'


b =

    type: '()'
    subs: [1 2]

Error using subsref
Subscript argument to SUBSREF and SUBSASGN must not be empty.
 

>>

Lukas Reichlin <paramaniac>
Mon 21 Nov 2011 09:42:13 PM UTC, comment #5: 

Yes, Octave does the same for the valid case.  What about the original one with the empty struct?


a = 1:4
b.type = "()"
b.subs = 1:2
subsref (a, b(2:end))


John W. Eaton <jwe>
Group administrator
Mon 21 Nov 2011 09:35:14 PM UTC, comment #4: 

R2011b:

>> a = 1:4;
b.type = "()";
b.subs = {1:2};
subsref (a,b)
 b.type = "()";
         |
Error: The input character is not valid in MATLAB statements or expressions.

>> a = 1:4;
b.type = '()';
b.subs = {1:2};
subsref (a,b)

ans =

     1     2

>>


Lukas Reichlin <paramaniac>
Mon 21 Nov 2011 09:31:01 PM UTC, comment #3: 

What does Matlab do in this situation?

John W. Eaton <jwe>
Group administrator
Mon 21 Nov 2011 08:58:54 PM UTC, comment #2: 

Thanks for the quick reply. Regarding the patch, the behaviour I expect would be not to raise an error. I would prefer that subsref (a, b) with empty b struct (but "type" and "subs" present) just returns argument "a" without modifications. This would fit my expectations and code better:
http://octave.svn.sourceforge.net/viewvc/octave/trunk/octave-forge/extra/quaternion_oo/inst/%40quaternion/subsref.m?revision=9108&view=markup




Lukas Reichlin <paramaniac>
Mon 21 Nov 2011 06:58:54 PM UTC, comment #1: 

This is a wonderful core dump.  It occurs on all Octave versions that I have tried including 3.0, 3.2.x, 3.4.x, and the development branch.  And it is not platform specific since I run Linux and you reported this on Mac OS.

First, subsref wants a character or cell array of indices for the second argument, not an ordinary matrix.  To have this work, even in the ordinary case, one would need to make subs a cell array.  For example:


a = 1:4;
b.type = "()";
b.subs = {1:2};
subsref (a,b)
=> 1:2


The problem seems to be that b(2:end) is a 1x0 empty struct array.  It passes the input validation tests of subsref in the function decode_subscripts in utils.cc because that function only checks whether the two fields "type" and "subs" exist.  It doesn't bother to check whether the fields are empty.

I just put in a simple test to see whether the fields are empty and now your original code passes.  See the patch attached to this bug report.  If that seems like the correct behavior I can commit it to the development branch.


(file #24416)

Rik <rik5>
Group administrator
Sun 20 Nov 2011 05:01:20 PM UTC, original submission:  

I noticed that subsref can crash in some cases. A simple example I could find is the following:


a = 1:4
b.type = "()"
b.subs = 1:2
subsref (a, b(2:end))

panic: impossible state reached in file `ov-range.cc' at line 116
panic: Abort trap: 6 -- stopping myself...
attempting to save variables to `octave-core'...
save to `octave-core' complete
Abort trap: 6


The problem originates from my upcoming quaternion package:
http://octave.svn.sourceforge.net/viewvc/octave/trunk/octave-forge/extra/quaternion_oo/inst/%40quaternion/subsref.m?revision=9108&view=markup

The problem is described in the m-file above. It crashes as well, but the panic
message is slightly different:

panic: impossible state reached in file `ov-base-mat.cc' at line 60


Lukas Reichlin <paramaniac>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #24432:  diffs.txt added by jwe (7KiB - text/plain)
file #24416:  subsref.patch added by rik5 (427B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-11-23 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2011-11-23 jwe Attached File- Added diffs.txt, #24432
    2011-11-21 rik5 Attached File- Added subsref.patch, #24416
        StatusNone Patch Submitted
    2011-11-20 paramaniac Carbon-Copy- Added paramaniac

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code