bugGNU Octave - Bugs: bug #54781, Matrix creation functions (eye,...

 
 

bug #54781: Matrix creation functions (eye, rand, zeros, ...) allow invalid non-integer dimensions

Submitter:  Kai Torben Ohlhus <siko1056>
Submitted:  Thu 04 Oct 2018 02:19:30 PM UTC
   
 
Category:  Interpreter Severity:  4 - Important
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  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

Fri 12 Oct 2018 08:12:15 AM UTC, comment #10: 

Awesome =)  In the spirit of your changes Rik, I added a small change for "eye()" in cset https://hg.savannah.gnu.org/hgweb/octave/rev/a533a7c4d2cc .

Regarding comment #8 for Matlab R2018b:


>> x = zeros ([2, 1.1, 2])
Error using zeros
Size inputs must be integers.


Same for Octave now (with your patches applied) =)

I really consider this item to be fixed/closed as well.  Thank you Rik and jwe for your help!

Kai Torben Ohlhus <siko1056>
Group Member
Thu 11 Oct 2018 10:49:33 PM UTC, comment #9: 

I pushed another changeset which fixes the remaining issue (https://hg.savannah.gnu.org/hgweb/octave/rev/d6581134daaa).  This revealed that Octave was doing an inappropriate conversion in rand() of a dimension vector in to an int type rather than an octave_idx_type.

Marking bug as fixed and closing report.

Rik <rik5>
Group administrator
Thu 11 Oct 2018 09:13:02 PM UTC, comment #8: 

For compatibility, what does Matlab return for


x = zeros ([2, 1.1, 2])


???

Rik <rik5>
Group administrator
Thu 11 Oct 2018 09:05:57 PM UTC, comment #7: 

I pushed a changeset that fixes all of the utility matrices like eye, ones, etc. as well as rand.  See https://hg.savannah.gnu.org/hgweb/octave/rev/373fe1608f7c.  I also added some BIST tests while I was at it. 

The remaining issue is a dimension vector such as


x = zeros ([2, 1.1, 2])


which still passes.

Rik <rik5>
Group administrator
Thu 11 Oct 2018 07:14:17 PM UTC, comment #6: 

Rik, thanks for taking care of this.  I applied file #45170 and can confirm the outputs of comment #5.

It improves the situation for the creation matrices, except for "rand".  I think it should be applied to all four overloaded versions of libinterp/corefcn/data.cc (fill_matrix) and we leave this report open.

Kai Torben Ohlhus <siko1056>
Group Member
Tue 09 Oct 2018 05:38:14 PM UTC, comment #5: 

Attached is a quick diff that works for me.  The error message is different now, but maybe that is okay?

Previously,


octave:1> x = zeros (2, {1}, 2)
error: octave_base_value::double_value (): wrong type argument 'cell'
error: octave_base_value::int64_value (): wrong type argument 'cell'
error: zeros: dimension arguments must be scalar integers


Now,


octave:2> x = zeros (2, 1.1, 2)
error: zeros: conversion of 1.1 to int64_t value failed


It doesn't mention that the dimension must be a scalar integer.  On the other hand, it identifies the argument that is in error far better.

The diff is trivial


diff -r 2d5f48a39b7e libinterp/corefcn/data.cc
--- a/libinterp/corefcn/data.cc Mon Oct 08 23:04:24 2018 +0200
+++ b/libinterp/corefcn/data.cc Tue Oct 09 10:33:09 2018 -0700
@@ -3931,8 +3931,12 @@ fill_matrix (const octave_value_list& ar
         dims.resize (nargin);

         for (int i = 0; i < nargin; i++)
-          dims(i) = (args(i).isempty ()
-                     ? 0 : args(i).xidx_type_value ("%s: dimension arguments must be scalar integers", fcn));
+          {
+            if (args(i).isempty ())
+              dims(i) = 0;
+            else
+              dims(i) = args(i).idx_type_value (true);
+          }
       }
       break;
     }


However, this still doesn't error out if a dimension vector is used.


octave:1> x = zeros ([2, 1.1, 2])
x =

ans(:,:,1) =

   0
   0

ans(:,:,2) =

   0
   0


That is a different codepath in the function fill_matrix in data.cc.



(file #45170)

Rik <rik5>
Group administrator
Tue 09 Oct 2018 05:09:04 PM UTC, comment #4: 

Agreed.  This was done for Matlab compatibility, but I never liked it either because I prefer strict input validation.  Marking bug as confirmed.

Rik <rik5>
Group administrator
Thu 04 Oct 2018 04:09:12 PM UTC, comment #3: 

OK, I'd be glad to require dimension arguments to be integers.  I never liked the rounding behavior anyway.  It was only done in Octave for compatibility, as far as I know.

John W. Eaton <jwe>
Group administrator
Thu 04 Oct 2018 03:04:24 PM UTC, comment #2: 

Okay, my Matlab experience refers to a much shorter timespan ;-)

At least R2018a errors in all cases with a hint to use integer dimensions only.

Kai Torben Ohlhus <siko1056>
Group Member
Thu 04 Oct 2018 02:36:17 PM UTC, comment #1: 

Originally, Matlab rounded (I think) non-integer values passed as dimensions to functions like eye, ones, and zeros.  So Octave did the same for compatibility.  What does modern Matlab do?

John W. Eaton <jwe>
Group administrator
Thu 04 Oct 2018 02:19:30 PM UTC, original submission:  

While fixing another bug, I received errors, because of potentially erroneous test cases (maybe copy&paste errors from "sprand")
https://hg.savannah.gnu.org/hgweb/octave/file/20e294a607af/test/diag-perm.tst#l80


A = rand (n-3, n, .5);


Additionally, Octave does not error on the following statements, and silently creates nonsense:


>> A = eye (0.5), size(A)
A = [](0x0)
ans =
   0   0

>> A = eye ([2, 0.5]), size(A)
A = [](2x0)
ans =
   2   0

>> A = eye (2, 0.5), size(A) # works!
error: eye: conversion of 0.5 to int64_t value failed


Similar for "zeros" and "ones" (only the first as example given below:


>> A = zeros (0.5), size(A)
A = [](0x0)
ans =
   0   0

>> A = zeros (2, 0.5), size(A)
A = [](2x0)
ans =
   2   0

>> A = zeros (2, 0.5, 4), size(A)
A = [](2x0x4)
ans =
   2   0   4


Kai Torben Ohlhus <siko1056>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #45170:  intidx.diff added by rik5 (659B - 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 rik5 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by siko1056 (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-10-11 rik5 StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2018-10-11 rik5 StatusPatch Submitted Confirmed
    2018-10-09 rik5 Attached File- Added intidx.diff, #45170
        StatusConfirmed Patch Submitted
    2018-10-09 rik5 StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code