bugGNU Octave - Bugs: bug #46660, object array writing into class...

 
 

bug #46660: object array writing into class member alters another class member

Submitter:  None
Submitted:  Thu 10 Dec 2015 05:59:20 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  lachlan
Originator Name:  Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * dev
Operating System:  * Any Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 05 Apr 2017 11:31:02 AM UTC, comment #8: 

@Lachlan: your patch seemed pretty useful for me. Markus and I reviewed it, the test case of this item was added to your patch and finally got pushed: https://hg.savannah.gnu.org/hgweb/octave/rev/0b6810085ed3

Closing this item.

Kai Torben Ohlhus <siko1056>
Group Member
Fri 06 Jan 2017 08:43:01 PM UTC, comment #7: 

just for information:
i have downloaded the current mercurial snapshot
version
ans = 4.3.0+
and i did perform a
hg patch --no-commit bug_46660_classdef_array_v2.cset

i made a thorough testing and everything looks great!

thanks and best regards!

Hg200 <hg200>
Tue 03 Jan 2017 09:48:23 AM UTC, comment #6: 

I'm terribly sorry about the buggy patch last time, and for whatever went wrong with my testing process.  This new patch Should Work(tm).

The previous patch identified cases where the index  ival  had only one dimension, and padded it on the right with "1".  In principle, that avoids the bug described in comment #2, but has two bugs.

1. I was using index 1 as the base instead of index 0, so that an assignment to (2) was incorrectly being assigned to (2,2).

2. Creating a(2) creates a(1,2) rather than a(2,1) as I had assumed.  That means the padding should be on the left in that case, instead of on the right.  The new patch checks whether to pad on the left or right, based on whether or not the first dimension is a singleton.


(file #39354)

Lachlan Andrew <lachlan>
Fri 05 Aug 2016 08:02:48 AM UTC, comment #5: 

well i think now we have to many fields.

waiting for the fix i have compiled the current mercurial snapshot with the bugfix present. after running the code i get an testclass object with dimension 2x2 (four integer fields for x and four integer fields for y):

>> a(1).x

ans = [](0x0)

>> a(2).x

ans = [](0x0)

>> a(3).x

ans = [](0x0)

>> a(4).x

ans =  123

>> a(1).y

ans = [](0x0)

>> a(2).y

ans = [](0x0)

>> a(3).y

ans = [](0x0)

>> a(4).y

ans =  321

as a comparison to the version 4.0.1 which comes with fedora the same code results in an testclassobject of 1x2:

>> a(1).x

ans = [](0x0)

>> a(2).x

ans = [](0x0)

>> a(3).x

error: A(I): index out of bounds; value 3 out of bound 2

>> a(1).y

ans = [](0x0)

>> a(2).y

ans =  321

>> a(3).y

error: A(I): index out of bounds; value 3 out of bound 2

note1: i am running the code from script (!) and the error is still present as you can see a(2).x equals zero but should be 123! so i can't confirm that running from script will protect you from the problem.

note2: the correct definition is 1x2 because testclass contains only two integers (x and y). in the code an array is created with two testclass members. hence we can only have four integers in total counting x and y.

best regards and thanks to the team!

Anonymous
Tue 26 Jul 2016 12:24:39 AM UTC, comment #4: 

I pushed this changeset here:

  http://hg.savannah.gnu.org/hgweb/octave/rev/eb8667f2faac

then I started doing some tests (I know, backwards).

Before the patch, I confirm the reported buggy behavior.

With the patch, I'm seeing [](0x0) returned when displaying a(2).x or a(2).y in all cases:


octave:1> clear all;
octave:2> clear classes;
octave:3> a(1)=testclass;
octave:4> a(2)=testclass;
octave:5> a(2).x = 123;
octave:6> display(a(2).x);
[](0x0)

octave:7> display(a(2).y);
[](0x0)

octave:8> a(2).y = 321;
octave:9> display(a(2).x);
[](0x0)

octave:10> display(a(2).y);
[](0x0)


Not sure what's happened here.

John W. Eaton <jwe>
Group administrator
Fri 01 Jul 2016 10:21:28 AM UTC, comment #3: 

The previous patch missed the "username" field.

(file #37644)

Lachlan Andrew <lachlan>
Tue 28 Jun 2016 08:53:07 AM UTC, comment #2: 

The attached patch seems to fix the problem.

The issue was that Array<T>::index (Array<idx_vector>&, bool resize_ok,...) was being called with a an array of size 1, causing it to assume that the array had to be extended.  This caused it to create a new element, overwriting the existing one.

Since the original code seemed to work when it was part of a script (I still don't know why), I'm not sure if it is worth writing a BIST for this.

(file #37608)

Lachlan Andrew <lachlan>
Mon 27 Jun 2016 11:43:09 AM UTC, comment #1: 

For those wishing to repeat this, you can cut-and-paste the following.


clear all;
clear classes;
a(1)=testclass;
a(2)=testclass;
a(2).x = 123;
display(a(2).x);

%123

display(a(2).y);

%[](0x0)

a(2).y = 321;
display(a(2).x);

%[](0x0) <----- writing into a(2).y has altered a(2).x

display(a(2).y);


Note that if this is placed into a script and executed, then it works correctly:


octave:14> testclasstest
 123

[](0x0)

 123

 321


The fact that it works in a script makes the severity of this problem much less than it would otherwise be.

Lachlan Andrew <lachlan>
Thu 10 Dec 2015 05:59:20 PM UTC, original submission:  

create a simple class with two integer attributes:

classdef testclass
   properties
    x
    y
   end
end

now create an object array of two of those objects and write an integer into x and after that into y. once you have written into y the integer previously stored in x is gone:

>> clear all;
>> clear classes;
>> a(1)=testclass;
>> a(2)=testclass;
>> a(2).x = 123;
>> display(a(2).x);

 123

>> display(a(2).y);

[](0x0)

>> a(2).y = 321;
>> display(a(2).x);

[](0x0)  <----- writing into a(2).y has altered a(2).x

>> display(a(2).y);

 321

if the statement is repeated with a non array everything is fine:

>> clear all;
>> clear classes;
>> b=testclass;
>> b.x = 123;
>> b.y = 321;
>> display(b.x)

 123 <----- writing into b.y hasn't altered b.x

>> display(b.y)

 321

the behaviour is different to matlab - and i suppose writing an attribute member should no alter or reset any other attribute members.


Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #39354:  bug_46660_classdef_array_v2.cset added by lachlan (2KiB - application/octet-stream)

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Updated the item)
  • -email is unavailable- added by hg200 (Posted a comment)
  • -email is unavailable- added by hg200 (i am the anonymous)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by lachlan (Updated the item)
  • -email is unavailable- added by mtmiller (Updated the item)
  • -email is unavailable- added by siko1056 (Updated 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 17 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-04-05 siko1056 StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2017-04-04 mmuetzel StatusPatch Submitted Patch Reviewed
    2017-03-29 rik5 Dependencies- bugs #50671 is dependent
    2017-03-29 mmuetzel Dependencies- bugs #47241 is dependent
    2017-01-03 lachlan Attached File- Added bug_46660_classdef_array_v2.cset, #39354
    2016-08-05 hg200 Carbon-Copy- Added hg200
    2016-07-01 lachlan Attached File- Added bug_46660_classdef_array.cset, #37644
    2016-06-28 lachlan Attached File- Added bug_46660_classdef_array.cset, #37608
        StatusConfirmed Patch Submitted
        Assigned toNone lachlan
    2016-06-27 lachlan Severity4 - Important 3 - Normal
    2016-04-14 lachlan Severity3 - Normal 4 - Important
    2015-12-22 mtmiller CategoryNone Interpreter
        Operating SystemGNU/Linux Any
    2015-12-21 siko1056 StatusNone Confirmed
        Release4.0.0 dev

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code