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

 
 

bug #34868: subsref crashes octave

Submitted by:  Lukas Reichlin <paramaniac>
Submitted on:  Sun 20 Nov 2011 05:01:20 PM UTC  
 
Category: InterpreterSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Segfault, Bus Error, etc.
Status: FixedAssigned to: None
Originator Name: paramaniacOpen/Closed: Closed
Release: 3.4.3Operating System: Mac OS

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 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>
Project 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

is not right. It is just

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

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

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

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

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

John W. Eaton <jwe>
Project 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.

Any thoughts?

Rik <rik5>
Project 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>
Project 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.

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

Rik <rik5>
Project 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>
Project 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.

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,

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>
Project 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

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>
Project 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

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>
Project 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>
Project 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?

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

R2011b:

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>
Project 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:

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>
Project 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:

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:

Lukas Reichlin <paramaniac>

 

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

Attach File(s):
   
   
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
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by rik5 (Updated the item)
  • -unavailable- added by paramaniac (Submitted the item)
  • -unavailable- added by paramaniac
  •  

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

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 23 Nov 2011 08:06:34 PM UTCrik5StatusPatch Submitted=>Fixed
      Open/ClosedOpen=>Closed
    Wed 23 Nov 2011 07:38:58 AM UTCjweAttached File-=>Added diffs.txt, #24432
    Mon 21 Nov 2011 06:58:54 PM UTCrik5Attached File-=>Added subsref.patch, #24416
      StatusNone=>Patch Submitted
    Sun 20 Nov 2011 05:01:21 PM UTCparamaniacCarbon-Copy-=>Added paramaniac

    Back to the top


    Powered by Savane 3.1-cleanup1